Pineapple Staking Contract
Cantina Security Report
Organization
- @pineapple
Engagement Type
Cantina Reviews
Period
-
Repositories
Researchers
Findings
High Risk
1 findings
1 fixed
0 acknowledged
Medium Risk
1 findings
1 fixed
0 acknowledged
Low Risk
2 findings
1 fixed
1 acknowledged
Informational
2 findings
1 fixed
1 acknowledged
High Risk1 finding
Users can enjoy high APR without continue lock
State
Severity
- Severity: High
≈
Likelihood: Medium×
Impact: High Submitted by
cccz
Description
Users can choose the lock period when entering the system, and the reward will be a higher APR during the lock period and a base APR outside of the lock period.
if (rewardDuration > lockPeriod && stake.stakingType != 0) { uint256 lockedReward = (stake.tokenAmount * lockPeriod * stakingAPR[uint8(stake.stakingType)]) / (365 days * BASE); uint256 flexReward = (stake.tokenAmount * (rewardDuration - lockPeriod) * stakingAPR[0]) / (365 days * BASE); return lockedReward + flexReward; } else { // default case return (stake.tokenAmount * rewardDuration * stakingAPR[uint8(stake.stakingType)]) / (365 days * BASE); }
However, if rewardDuration <= lockPeriod, the reward will always be the higher APR.
e.g., if a user locks for 1 year and claims the reward after 1.5 years, the reward will be 15% APR for 1 year and 3% APR for 0.5 years.
however, if the user claims the reward every 0.5 years, the reward will always be 15% APR.
This actually makes the user enjoy the higher APR after the lock period without having to continue lock.
Recommendation
Change to
--- a/contracts/PappleStaking.sol+++ b/contracts/PappleStaking.sol@@ -369,24 +369,25 @@ contract Staking is Stake storage stake = stakes[_id]; if (stake.finished) return 0;- uint256 rewardDuration = block.timestamp - stake.lastClaimTime;- uint256 lockPeriod = stakingPeriod[uint8(stake.stakingType)];-- // if a locked stake has finished it continues to accrue base interest until withdrawn (flexReward)- if (rewardDuration > lockPeriod && stake.stakingType != 0) {- uint256 lockedReward = (stake.tokenAmount *- lockPeriod *- stakingAPR[uint8(stake.stakingType)]) / (365 days * BASE);- uint256 flexReward = (stake.tokenAmount *- (rewardDuration - lockPeriod) *- stakingAPR[0]) / (365 days * BASE);- return lockedReward + flexReward;- } else {- // default case- return- (stake.tokenAmount *- rewardDuration *+ uint256 highAPRTime = stake.startTime + stakingPeriod[uint8(stake.stakingType)];+ if(block.timestamp > highAPRTime) {+ if(highAPRTime > stake.lastClaimTime) {+ uint256 lockedReward = (stake.tokenAmount *+ (highAPRTime - stake.lastClaimTime) * stakingAPR[uint8(stake.stakingType)]) / (365 days * BASE);+ uint256 flexReward = (stake.tokenAmount *+ (block.timestamp - highAPRTime) *+ stakingAPR[0]) / (365 days * BASE);+ return lockedReward + flexReward;+ } else {+ return (stake.tokenAmount *+ (block.timestamp - stake.lastClaimTime) *+ stakingAPR[0]) / (365 days * BASE);+ }+ } else {+ return (stake.tokenAmount *+ (block.timestamp - stake.lastClaimTime) *+ stakingAPR[uint8(stake.stakingType)]) / (365 days * BASE); } }
PineappleDEX: Fixed in commit 3e537348
Cantina: Fix verified.
Medium Risk1 finding
Updates to stakingMultiplier and BASE_SCORE_VALUE may prevent users from withdrawal
State
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
cccz
Description
When the user stakes and withdraws,
userTotalScore
is updated based on the result of_calculateStakeScore()
.userTotalScore[msg.sender] += _calculateStakeScore( _amount, _stakingType ); ... userTotalScore[stake.user] -= _calculateStakeScore( stake.tokenAmount, stake.stakingType );
The result of
_calculateStakeScore()
is affected bystakingMultiplier
andBASE_SCORE_VALUE
, and they may be changed by the owner.return (_amount * stakingMultiplier[uint8(_stakingType)] * BASE_SCORE_VALUE) / denominator; } ... function setStakingMultiplier( uint8 tier, uint256 mult_ ) external onlyOwner nonReentrant whenNotPaused { require(tier < maxStakingTypes, "Invalid tier"); uint256 oldMultiplier = stakingMultiplier[tier]; stakingMultiplier[tier] = mult_; emit StakingMultiplierSet(tier, oldMultiplier, mult_); } ... function setBaseScoreValue( uint256 _newValue ) external onlyOwner nonReentrant whenNotPaused { uint256 oldValue = BASE_SCORE_VALUE; BASE_SCORE_VALUE = _newValue; emit BaseScoreValueSet(oldValue, _newValue); }
If
stakingMultiplier
andBASE_SCORE_VALUE
increase, due to the increase of_calculateStakeScore()
during the stake period, this may cause the update ofuserTotalScore
to underflow when the user withdraws.userTotalScore[stake.user] -= _calculateStakeScore( stake.tokenAmount, stake.stakingType );
For example, when a user stakes,
userTotalScore
increases by 100. During the stake period, thestakingMultiplier
increases. When the user withdraws,userTotalScore
decreases by 110, resulting in an underflow.Recommendation
Option 1: When reducing
userTotalScore
, if the reduced value is greater thanuserTotalScore
, directly setuserTotalScore
to 0. This does not completely get rid of the issue, because ifstakingMultiplier
andBASE_SCORE_VALUE
are reduced, the user will have some extra points.Option 2: Add a
score
field to theStake
structure, when the user stakes, save thescore
and increaseuserTotalScore
, when the user withdraws, subtract the value of thescore
field directly fromuserTotalScore
.PineappleDEX: Fixed in commit 2aa5a274 and 3e537348
Cantina: Fix verified.
Low Risk2 findings
restakeStake() is missing system status check
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: Low Submitted by
cccz
Description
When
isOpen
is false ordepositsPaused
is true, the protocol prohibits users from callingstakeTokens()
to enter the system, howeverrestakeStake()
lacks these checks, allowing users to bypass these status checks and enter the system.function stakeTokens( uint256 _amount, uint256 _stakingType ) external nonReentrant whenNotPaused { require(isOpen, "Staking is not available"); require(!depositsPaused, "Deposits are paused");
Recommendation
Change to
function restakeStake(uint256 _id) external nonReentrant whenNotPaused { Stake storage oldStake = stakes[_id];+ require(isOpen, "Staking is not available");+ require(!depositsPaused, "Deposits are paused"); require(!oldStake.finished, "Stake already withdrawn"); require(oldStake.user == msg.sender, "Not stake owner"); uint256 stakingType = oldStake.stakingType; uint256 stakingDuration = block.timestamp - oldStake.startTime;
PineappleDEX: Fixed in commit 2aa5a274
Cantina: Fix verified.
Reward race issue
State
- Acknowledged
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: Medium Submitted by
cccz
Description
In
withdrawStake()
andrestakeStake()
, if there are not enough rewards in the system, the system will send the remaining rewards to the user and finish the user's stake.uint256 balance = stakingToken.balanceOf(address(this)); // Contract must at least hold all staked principal require( balance >= totalAmountOfStakes, "Insufficient principal balance" ); // Only the surplus over *all* live principal is usable as reward uint256 availableReward = balance - totalAmountOfStakes; uint256 paidReward = rewardAmount <= availableReward ? rewardAmount : availableReward;
One problem is that if multiple users withdraw at the same time, the rewards only satisfy some users, but all users think the rewards are enough. Then when the users' transactions are executed, some users will get little or no rewards due to the reward race issue.
Recommendation
It is recommended to provide a
minRewardOut
parameter inwithdrawStake()
andrestakeStake()
to allow users to control the slippage of the rewards they receive.PineappleDEX : Acknowledged, we will ensure that the contract is always sufficiently funded.
Informational2 findings
Redundant Check
Severity
- Severity: Informational
Submitted by
cccz
Description
The following check in
withdrawStake()
is incorrect.- require (balance + paidReward >= totalAmountOfStakes, "Insufficient reward liquidity");+ require (balance >= totalAmountOfStakes + paidReward, "Insufficient reward liquidity");
And it is redundant because the previous check and calculation ensures that it is always true.
require( balance >= totalAmountOfStakes, "Insufficient principal balance" ); // Only the surplus over *all* live principal is usable as reward uint256 availableReward = balance - totalAmountOfStakes; uint256 paidReward = rewardAmount <= availableReward ? rewardAmount : availableReward; // Contract must at least hold all staked principal + paid reward require (balance + paidReward >= totalAmountOfStakes, "Insufficient reward liquidity");
Recommendation
It is recommended to remove this check.
PineappleDEX: Fixed in commit 2aa5a274
Cantina: Fix verified.
Centralization risk
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
cccz
Description
The
setStakingPeriod()
andsetStakingAPR()
functions allow the owner to call to change the stake period and APR, and their changes may cause the user's funds to be locked for longer periods of time or to get a lower APR.function setStakingPeriod( uint8 tier, uint256 seconds_ ) external onlyOwner nonReentrant whenNotPaused { require(tier < maxStakingTypes, "Invalid tier"); uint256 oldPeriod = stakingPeriod[tier]; stakingPeriod[tier] = seconds_; emit StakingPeriodSet(tier, oldPeriod, seconds_); } function setStakingAPR( uint8 tier, uint256 apr_ ) external onlyOwner nonReentrant whenNotPaused { require(tier < maxStakingTypes, "Invalid tier"); uint256 oldAPR = stakingAPR[tier]; stakingAPR[tier] = apr_; emit StakingAPRSet(tier, oldAPR, apr_); }
Recommendation
It is recommended to document this issue to inform the user.
PineappleDEX : Acknowledged, we will add a small disclaimer to the site.