StakingServiceBase.sol#_updateAmountStakedWithdraw() - once per cycle, until the first deposit for the cycle, people are unable to withdraw their stake

[hats-bug-reporter[bot]]

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

Description: Description\ The _updateAmountStakedWithdraw() function is meant to, as per the comments, to update the position NFTs total amount staked for the next cycle and take from the current if the next cycle's pending amount does not cover the withdraw. The logic is fine, however the state variable for the next cycle's total staked amount accross all positions may not be initialized at the time of withdrawal.

Attack Scenario\ There are several variables that track every cycle's state:

• _stakingHistoryByToken tracks the last cycle the user did an action
• _tokenInfoByCycle holds the staked and pending amounts for a token each cycle
• _cycleInfo holds general info about the entire cycle: is it processed, how much total stake there is from all positions and allocated rewards.

With these in mind envision this scenario: It is cycle1 and we deposit 100 tokens, so now:

1. _tokenInfoByCycle[2][tokenId] becomes 100 more on both amount and pending
2. _stakingHistoryByToken[tokenId] gets the next cycle pushed, so the last action will be saved as cycle2
3. Finally _cycleInfo[nextCycle].totalStaked gets incremented by the deposited amount. There is only incrementing here, meaning that if this is the first deposit for a cycle, the initial value of _cycleInfo[nextCycle].totalStaked will be 0 and it will go up from there. This happens because inside processStakersRewards when we do _cycleInfo[_cvgStakingCycle + 2].totalStaked = _cycleInfo[_cvgStakingCycle + 1].totalStaked, the _cvgStakingCycle is already incremented, thus for e.g if we deposit at cycle 1, we increment the stake for cycle 2. When we go to cycle 2, we try to set cycle 4 to cycle 3's stake, but it is empty

Point 3 is exactly the issue. Inside _updateAmountStakedWithdraw() we do the following:

1. Get the last history entry and take it's _tokenInfoByCycle.amountStaked
2. Make sure the amount we withdraw is less or equal to our current stake
3. Calculate the new stake amount as _tokenInfoByCycle.amountStaked- amount and set it for the next cycle
4. Finally we do _cycleInfo[nextCycle].totalStaked -= amount

At step 4, if there have not been any deposits for the current cycle, then _cycleInfo[nextCycle].totalStaked would still be 0 and we would revert, even though if we reached this step, this means our history's stake covers the withdrawal. This happens because the cycle info mapping does not keep track of the global stakes, but only per cycle.

The scenario holds even if there have been deposits for the cycles in the edge-case where somebody's withdrawal is greater than the deposit that came before it. E.g:

• Bob deposits 100 as the first person for the cycle, thus _cycleInfo[nextCycle].totalStaked increases to 100
• Alice has 200 staked from previous cycles and decides to withdraw them all. _cycleInfo[nextCycle].totalStaked is less than 200 so we revert

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   
   function processStakersRewards(uint256 amount) external {
       require(msg.sender == address(cvgControlTower.cvgRewards()), "NOT_CVG_REWARDS");

• //@auditor: we bring this at the top, because on deposit

• //we update nextCycle's stake. If we first increase the cycle

• //it was like we tried to skip over a cycle and do

• //nextCycle+2 = nextCycle+1, instead of nextCycle+1=nextCycle

• //which is the correct way

• cycleInfo[_cvgStakingCycle + 2].totalStaked = _cycleInfo[_cvgStakingCycle + 1].totalStaked;

  /// @dev Increments the cvg cycle
  uint256 _cvgStakingCycle = stakingCycle++;
  
  /// @dev Sets the amount computed by the CvgRewards ( related to Gauge weights ) in the triggered cycle.
  _cycleInfo[_cvgStakingCycle].cvgRewardsAmount = amount;

• cycleInfo[_cvgStakingCycle + 2].totalStaked = _cycleInfo[_cvgStakingCycle + 1].totalStaked;

  emit CvgCycleProcess(_cvgStakingCycle, amount);

  }

Recommendation\ The entire issue stems from the fact that processStakersRewards updates the info for the next cycle wrong and tries to access empty cycles. Look at the revised code for the fix

[PlamenTSV]

Sorry, title is kind of misleading. The main issue is inside processStakersRewards since we increment the cycle, before assigning the cycle infos.

[walk-on-me]

In the processStakerRewards, we are reporting the totalStaked of the nextCycle ( future actualCycle ) in the cycle N+2 ( future nextCycle).

Indeed, our staking design oblige user to stay staked for 1 full cycle and so, the only way to modify the totalStakedof a current cycle is to withdraw.

We have so to consider this issue as Invalid as it's the design of our feature.

[PlamenTSV]

It is not design of the feature, the function looks like this:

//Here we increment the cycle. If the cycle is 1, our stored state is in cycle 2, so we go to cycle 2
         uint256 _cvgStakingCycle = stakingCycle++;

        _cycleInfo[_cvgStakingCycle].cvgRewardsAmount = amount; //We set the rewards for cycle 2

//We transfer the total staked of CYCLE 3 to CYCLE 4 not from cycle 2 to cycle 3 like the intentions are
//The issue lies in the fact that we increment "_cvgStakingCycle" before transfering it's stake 
        _cycleInfo[_cvgStakingCycle + 2].totalStaked = _cycleInfo[_cvgStakingCycle + 1].totalStaked;

As it can be seen the cycle gets incremented by 1 to the cycle that holds the currrent state. If we are in cycle 1, we need the state of cycle 2. In the last line instead of setting cycle 3's total to cycle 2's you increment it again, thus you are trying to set cycle 4's stake to cycle 3's. You try to transfer cycle 3's total to cycle 4's, but cycle 2 begins with this function, meaning that cycle 3 and 4 are empty

[walk-on-me]

Lets say that _cvgStakingCycle = 1.

uint256 _cvgStakingCycle = stakingCycle++; => _cvgStakingCycle = 1

to don't mix with :

uint256 _cvgStakingCycle = ++stakingCycle; => _cvgStakingCycle = 2