Protocol assumes deposit pause of only 1 cycle and could slash users unfairly

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x172ae8cfaafba6e3509330adda61840f9d9015d94fe3503d701b9f73f674395e Severity: medium

Description: Description\ The StakingServiceBase.sol is the base contract for all staking services and handles the logic regarding track of individual token cycle history, the current cycle and manages deposit/withdrawals and the way they handle future cycles. And interesting aspect of the deposit function _updateAmountStakedDeposit is how it handles periods with no deposits on a tokenId. By the protocols logic, if we stake at cycle N, we can claim rewards at N+2 or later and claim for all later cycles until we withdraw. However the mappings tracking history and deposit amounts per cycle are updated only on user action, which is why the protocol does an interesting check:

/// @dev if this _lastActionCycle is less than the next cycle => it's the first deposit done on this cycle
            if (_lastActionCycle < nextCycle) {
                uint256 currentCycle = nextCycle - 1;
                /// @dev if this _lastActionCycle is less than the current cycle =>
                ///      No deposits occurred on the last cycle & no withdraw on this cycle
                if (_lastActionCycle < currentCycle) {
                    /// @dev we have so to checkpoint the current cycle
                    _stakingHistoryByToken[tokenId].push(currentCycle);
                    /// @dev and to report the amountStaked of the lastActionCycle to the currentCycle
                    _tokenInfoByCycle[currentCycle][tokenId].amountStaked = _tokenInfoByCycle[_lastActionCycle][tokenId].amountStaked; 
                }
                /// @dev checkpoint the next cycle
                _stakingHistoryByToken[tokenId].push(nextCycle);
            }

If the last cycle we interacted in is less than the next one it can either be the: current one or an older one If it is only the current one, we simply push the next cycle. If it is older, the protocol does the mistake of assuming that no deposits occured ONLY on the last cycle.

Attack Scenario\ An example scenario: We start from the first cycle, so cycle = 1 We deposit 100 CVX, we enter _updateAmountStakedDeposit, where:

1. Total staked for the nextCycle=2 becomes 100 more
2. Our tokenId's staked amount becomes 100
3. cycle=2 gets pushed to our history as the last entry of the array (it's the mint of the tokenId position)

Our reward will be claimable at cycle = 3 but we should be able to claim more rewards if we keep our stake for longer and could just wait. At cycle 5 we do another deposit of 100 and the following happens:

1. Total staked for the nextCycle=6 becomes 100 more
2. Our tokenId's staked amount becomes 200
3. We have previous history, so we check when was the last time we interacted with the staking service. It was cycle=2, so the first condition for if (_lastActionCycle < nextCycle) passes. Inside of it we have another one if (_lastActionCycle < currentCycle). Here the protocol assumes no deposits happened only during the previous cycles, but in our case it was for 3 cycles in a row. It does: _tokenInfoByCycle[currentCycle][tokenId].amountStaked = _tokenInfoByCycle[_lastActionCycle][tokenId].amountStaked setting the current cycle's staked amount to the last staked amount. But the cycles where we did not interact with the protocol remain unupdated, even though our stake was inside during those cycles

This means that looking back on the cycles we have: Cycle 1 - nothing since it is the first cycle Cycle 2 - total is x+100, our tokenId's amount is 100 and the stake history holds the value of 2 Cycle 3 - total is x+100, but our tokenId's amount inside _tokenInfoByCycle remains 0 Cycle 4 - same as number 3 Cycle 5 - total is x+100, we deposited 100 and due to the difference between our history and the current cycle, we update _tokenInfoByCycle to 100.

This leaves cycle 3 and 4 with empty amounts, even though we have staked since cycle 1. On attempt to claim those rewards, we would be rewarded only for cycles 2, 5 and 6(6 holds the deposit during 5, 5 holds the same values as 2), even though our stake has been sitting inside the contract for all cycles.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   
   /// @dev Else it's not the mint of the position
       else {
           /// @dev fetches the _lastActionCycle where an action has been performed
           uint256 _lastActionCycle = _stakingHistoryByToken[tokenId][cycleLength - 1];
   
           /// @dev if this _lastActionCycle is less than the next cycle => it's the first deposit done on this cycle
           if (_lastActionCycle < nextCycle) {
               uint256 currentCycle = nextCycle - 1;
   
               /// @dev if this _lastActionCycle is less than the current cycle =>
               ///      No deposits occurred on the last cycle & no withdraw on this cycle
               if (_lastActionCycle < currentCycle) {

• for(uint prevCycle = _lastActionCycle; prevCycle <= currentCycle; ){

• //@dev we iterate over all cycles where no deposit/withdraw actions occured

• // and set their tokenId staked amounts to the last recorded

• // to correctly reflect the earned rewards

• _stakingHistoryByToken[tokenId].push(prevCycle);

• _tokenInfoByCycle[prevCycle][tokenId].amountStaked = _tokenInfoByCycle[_lastActionCycle][tokenId].amountStaked;

• unchecked {prevCycle ++}

• }

• /// @dev we have so to checkpoint the current cycle

• _stakingHistoryByToken[tokenId].push(currentCycle);

• /// @dev and to report the amountStaked of the lastActionCycle to the currentCycle

• _tokenInfoByCycle[currentCycle][tokenId].amountStaked = _tokenInfoByCycle[_lastActionCycle][tokenId].amountStaked; }

          /// @dev checkpoint the next cycle
          _stakingHistoryByToken[tokenId].push(nextCycle);
      }
  }

Recommendation\ Instead of assuming that only the last cycle has no deposits on it, iterate over all cycles between the last one in the history and the current one, updating their _tokenInfoByCycle to the latest recorded one accordingly. Something similiar to the logic inside _stakedAmountEligibleAtCycle

[shalbe-cvg]

Hello,

Thank you for reporting this issue. What you have mentioned in this issue is by design. Values contained in the _tokenInfoByCycle mapping can have an amountStaked of 0 if no action occurs during a/multiple cycle(s) but it will never impact the rewards calculation since the _stakedAmountEligibleAtCycle function will handle this case.

For information, we decided to do it like this to avoid the loop you provided in your PoC to save some gas.

To be more specific, the latter function will fetch the last cycle where an action occurs so the amountStaked properly represents the eligible staked amount for rewards for a specified cycle.

Regards.