RSR holders could get less staked stRSR than expected

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L502 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L621 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L230

Vulnerability details

Impact

The stake() function allows RSR holders to stake their RSR token for stRSR token at an exchange rate of that current time. However, the lack of slippage protection can result in users receiving less than expected when staking RSR tokens. This occurs because the stakeRate (i.e. exchangeRate) is updated within the stake functions by the _payoutRewards function. However, users typically check the exchangeRate using the exchangeRate() function before staking to understand the expected amount they will receive. Since the exchangeRate() function does not trigger _payoutRewards, it returns a value that may change if a new stakeRate is claculated (based on new added rewards), which leads to changes in the exchange rate when the actual staking occurs. This lack of a slippage protection mechanism can result in users receiving fewer stRSR tokens than anticipated.

Proof of Concept

• Payout Reward Function: This function payouts rewards (adds to stakeRSR) and updates the stakeRate. https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L593-L627

  function _payoutRewards() internal {
      if (block.timestamp < payoutLastPaid + 1) return;
      uint48 numPeriods = uint48(block.timestamp) - payoutLastPaid;
  
      uint192 initRate = exchangeRate();
      uint256 payout;
  
      // Do an actual payout if and only if enough RSR is staked!
      if (totalStakes >= FIX_ONE) {
          // Paying out the ratio r, N times, equals paying out the ratio (1 - (1-r)^N) 1 time.
          // Apply payout to RSR backing
          // payoutRatio: D18 = FIX_ONE: D18 - FixLib.powu(): D18
          // Both uses of uint192(-) are fine, as it's equivalent to FixLib.sub().
          uint192 payoutRatio = FIX_ONE - FixLib.powu(FIX_ONE - rewardRatio, numPeriods);
  
          // payout: {qRSR} = D18{1} * {qRSR} / D18
          payout = (payoutRatio * rsrRewardsAtLastPayout) / FIX_ONE;
          stakeRSR += payout;
      }
  
      payoutLastPaid += numPeriods;
      rsrRewardsAtLastPayout = rsrRewards();
  
      // stakeRate else case: D18{qStRSR/qRSR} = {qStRSR} * D18 / {qRSR}
      // downcast is safe: it's at most 1e38 * 1e18 = 1e56
      // untestable:
      //      the second half of the OR comparison is untestable because of the invariant:
      //      if totalStakes == 0, then stakeRSR == 0
      @> stakeRate = (stakeRSR == 0 || totalStakes == 0)
          ? FIX_ONE
          : uint192((totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR);
  
      emit RewardsPaid(payout);
      emit ExchangeRateSet(initRate, exchangeRate());
  }

• Exchange Rate: When a user wants to stake or unstake RSR, they will typically check the exchangeRate() function to calculate the expected stRSR:

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L502

function exchangeRate() public view returns (uint192) {
    // D18{qRSR/qStRSR} = D18 * D18 / D18{qStRSR/qRSR}
    @>return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate; // ROUND method
}

However, the exchangeRate function does not call the _payoutRewards function, which updates the stakeRate to a new value (in case of new added RSR rewards).

• Stake Function: When a user stakes RSR tokens, the stake() function calls _payoutRewards before minting new stRSR tokens:

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L227

function stake(uint256 rsrAmount) public {
    _notZero(rsrAmount);
    @> _payoutRewards();
    mintStakes(_msgSender(), rsrAmount);
    IERC20Upgradeable(address(rsr)).safeTransferFrom(_msgSender(), address(this), rsrAmount);
}

This can change the stakeRate after the user has checked the exchangeRate, leading to them receiving fewer stRSR tokens than expected.

• Example: As a simple example, suppose:

1. stakeRate is set as FIX_ONE (i.e. 1e18 - 1:1), totalStakes = 500 and stakeRSR = 500
2. Alice wants to stake 50 RSR, calls exchangeRate() function to get the exchange rate between RSR and StRSR (note: exchangeRate() returns FIX_ONE i.e. 1e18 therefore, 50 RSR = 50 StRSR).
3. Alice calls stake() function. Assume there are new reward to distribute in the contract (200 RSR), the _payoutRewards() function re-calculates a new stakeRate. new stakeRate = (totalStakes(500) * FIX_ONE_256(1e18) + (stakeRSR(700) - 1)) / stakeRSR(700) = 7.14285714285714E+17 (note: now less than initial stakeRate of FIX_ONE (1e18))
4. With the new stakeRate, Alice now gets StRSR amount of = stakeRate(7.14285714285714E+17) * stakeAmount(50) = ~ 35 StRSR (i.e. 15 StRSR less than expected)

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Either implement a slippage protection mechanism in the stake function or update the exchangeRate function to call _payoutRewards

Reference

https://github.com/code-423n4/2023-12-revolutionprotocol-findings/issues/397#issue-2051784043

Assessed type

Other

[tbrent]

This lack of a slippage protection mechanism can result in users receiving fewer stRSR tokens than anticipated.

The exchangeRate() function is a view function that can be useful from off-chain or from other contracts. It has nothing to do with how much StRSR tokens stakers get during staking. In that case, _payoutRewards() is called correctly: https://github.com/reserve-protocol/protocol/blob/72fc1f6e41da01e733c0a7e96cdb8ebb45bf1065/contracts/p1/StRSR.sol#L230

[thereksfour]

Warden's point is that exchangeRate() returns stale exchange rates. But I think this is intentional, as exchangeRate() is a view function and cannot call _payoutRewards() to always get the latest exchange rate. So adding slippage protection would be a better approach, but it is OK without it because the exchange rate fluctuations are mild by design. Regarding severity, unlike the findings cited by warden, the exchange rate here does not rise or fall significantly, and users can unstake to withdraw RSR. Will consider downgrading to Low

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-b