Pineapple. Dex

Pineapple Staking Contract

Cantina Security Report

Organization

@pineapple

Engagement Type

Cantina Reviews

Period

-

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

  1. Users can enjoy high APR without continue lock

    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

  1. Updates to stakingMultiplier and BASE_SCORE_VALUE may prevent users from withdrawal

    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 by stakingMultiplier and BASE_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 and BASE_SCORE_VALUE increase, due to the increase of _calculateStakeScore() during the stake period, this may cause the update of userTotalScore 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, the stakingMultiplier 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 than userTotalScore, directly set userTotalScore to 0. This does not completely get rid of the issue, because if stakingMultiplier and BASE_SCORE_VALUE are reduced, the user will have some extra points.

    Option 2: Add a score field to the Stake structure, when the user stakes, save the score and increase userTotalScore, when the user withdraws, subtract the value of the score field directly from userTotalScore.

    PineappleDEX: Fixed in commit 2aa5a274 and 3e537348

    Cantina: Fix verified.

Low Risk2 findings

  1. restakeStake() is missing system status check

    Severity

    Severity: Low

    Likelihood: Low

    ×

    Impact: Low

    Submitted by

    cccz


    Description

    When isOpen is false or depositsPaused is true, the protocol prohibits users from calling stakeTokens() to enter the system, however restakeStake() 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.

  2. Reward race issue

    State

    Acknowledged

    Severity

    Severity: Low

    Likelihood: Low

    ×

    Impact: Medium

    Submitted by

    cccz


    Description

    In withdrawStake() and restakeStake(), 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 in withdrawStake() and restakeStake() 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

  1. 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.

  2. Centralization risk

    State

    Acknowledged

    Severity

    Severity: Informational

    Submitted by

    cccz


    Description

    The setStakingPeriod() and setStakingAPR() 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.