RSR Unstake Denial of Service Due to Underflow Error

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L648-L649
https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L324-L332
https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L463

Vulnerability details

Impact

Unstaking in the RSR contract would fail under certain circumstances when withdraw(...) and seizeRSR(...) functions have been previously called causing Denial of Service to innocent users who would like to unstake Due to Underflow Error in unstake(...) function.

Proof of Concept

 function pushDraft(address account, uint256 rsrAmount)
        internal
        returns (uint256 index, uint64 availableAt)
    {
        // draftAmount: how many drafts to create and assign to the user
        // pick draftAmount as big as we can such that (newTotalDrafts <= newDraftRSR * draftRate)
        draftRSR += rsrAmount;
        // newTotalDrafts: {qDrafts} = D18{qDrafts/qRSR} * {qRSR} / D18
>>>        uint256 newTotalDrafts = (draftRate * draftRSR) / FIX_ONE;
>>>        uint256 draftAmount = newTotalDrafts - totalDrafts;
        totalDrafts = newTotalDrafts;

        // Push drafts into account's draft queue
        CumulativeDraft[] storage queue = draftQueues[draftEra][account];
        index = queue.length;

        uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0;
        uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0;
        availableAt = uint64(block.timestamp) + unstakingDelay;
        if (lastAvailableAt > availableAt) {
            availableAt = lastAvailableAt;
        }

        queue.push(CumulativeDraft(uint176(oldDrafts + draftAmount), availableAt));
    }

The code above shows how pushDraft(...) is implemented before it is called during unstaking at L279 of the StRSR contract. it can be noted from the two pointers above that newTotalDrafts is derived from the multiplicative operation of draftRate & draftRSR, the protocol assumes newTotalDrafts would definitely be greater than previous totalDrafts as the draftRSR used to perform this multiplication is increased with incoming rsrAmount. This assumption is the reason behind the subtraction operation noted in the second pointer which assumes newTotalDrafts would definitely be greater than previous totalDrafts, this assumption is wrong as there is a flaw which can make draftRate lower than normal thereby making the increase of draftRSR irrelevant to the overall calculation of newTotalDrafts.
The next series of text in this report would try to break down why draftRate can be lower than usual.

 function withdraw(address account, uint256 endId) external {
   ...
        // ==== Compute RSR amount
        uint256 newTotalDrafts = totalDrafts - draftAmount;
        // newDraftRSR: {qRSR} = {qDrafts} * D18 / D18{qDrafts/qRSR}
>>>        uint256 newDraftRSR = (newTotalDrafts * FIX_ONE_256 + (draftRate - 1)) / draftRate;
        uint256 rsrAmount = draftRSR - newDraftRSR;

        if (rsrAmount == 0) return;

        // ==== Transfer RSR from the draft pool
        totalDrafts = newTotalDrafts;
        draftRSR = newDraftRSR;
  ...
}

The code above shows how there is reduction in Total draft and draftRSR when withdrawal is called but draftRSR is not completely reduced but slightly inflated by 1 i.e ...(draftRate - 1)) / draftRate; as noted in the pointer, it pushes up the value for the purpose of precision.

 function seizeRSR(uint256 rsrAmount) external {
...
        // update draftRate, possibly beginning a new draft era
        if (draftRSR != 0) {
            // Downcast is safe: totalDrafts is 1e38 at most so expression maximum value is 1e56
>>>            draftRate = uint192((FIX_ONE_256 * totalDrafts + (draftRSR - 1)) / draftRSR);
        }
...

The implication of this slight increase of draftRSR would mean a decrease in the value of draftRate (i.e dividing by increased denominator of draftRSR), the protocol wrongly assumes the addition of (draftRSR - 1) to the numerator in the code above would coverup for the inflation in the denominator(i.e draftRSR ) but this would no longer be the case due to the multiplicative effect of the formula.

scenario

if
totalDrafts = 101
from the withdraw(...) function assuming draftRSR was suppose to be 4 for example it would be inflated to 5 so
draftRSR = 5
Therefore based on the seizeRSR(...) function
draftRate = (101+(5-1))/5 = 21
however (5-1) would not be enough to cover for the math discrepancies as 101/4 is suppose to be 25 which is completely far from 21 that the protocol formular would derive.
The effect of this would be felt in the pushDraft(...) function where protocol wrongly assumes that newTotalDrafts can never be lower than the previous totalDrafts thereby attempting to carry out a substration operation under this false assumption without any validations i.e

   uint256 newTotalDrafts = (draftRate * draftRSR) / FIX_ONE;
   uint256 draftAmount = newTotalDrafts - totalDrafts;

when in reality the increase of draftRSR in the formular would not always be enough to cover for the excessive decrease of draftRate which would make newTotalDrafts lower than old totalDrafts causing Dos when a user want to unstake at L279 of the contract

POC code

describe('Withdrawals/Unstaking', () => {
...
 it('Should Dos unstake in RSR', async () => {
      // Perform stake
      const amount1: BigNumber = bn('1e18')
      const amount2: BigNumber = bn('2e18')
      const amount3: BigNumber = bn('3000e18')

      // Approve transfers
      await rsr.connect(addr1).approve(stRSR.address, amount1.add(amount2))
      await rsr.connect(addr2).approve(stRSR.address, amount3)

      // Stake
      await stRSR.connect(addr1).stake(amount1)
      await stRSR.connect(addr1).stake(amount2)
      await stRSR.connect(addr2).stake(amount3)

       let availableAt = (await getLatestBlockTimestamp()) + config.unstakingDelay.toNumber() + 1

       // Set next block timestamp - for deterministic result
       await setNextBlockTimestamp((await getLatestBlockTimestamp()) + 1)

       // Unstake - Create withdrawal
       await expect(stRSR.connect(addr1).unstake(amount1))
         .emit(stRSR, 'UnstakingStarted')
         .withArgs(0, 1, addr1.address, amount1, amount1, availableAt)

       await expectWithdrawal(addr1.address, 0, { rsrAmount: amount1 })

       // Check draftQueueLen
       await expectDraftQueue(1, addr1.address, 1)

       // All staked funds withdrawn upfront
       expect(await stRSR.balanceOf(addr1.address)).to.equal(amount2)

 
 await whileImpersonating(backingManager.address, async (signer) => {
  const x: BigNumber = bn('1e17')
  await stRSR.connect(signer).seizeRSR(x)
})

       availableAt = (await getLatestBlockTimestamp()) + config.unstakingDelay.toNumber() + 1

       // Set next block timestamp - for deterministic result
       await setNextBlockTimestamp((await getLatestBlockTimestamp()) + 1)
       const amount3b: BigNumber = bn('2.999999999999999900e18')
      // Unstake again with different user
      await expect(stRSR.connect(addr2).unstake(amount3b))
        .emit(stRSR, 'UnstakingStarted')
        .withArgs(0, 1, addr2.address, amount3b, amount3b, availableAt)

     
      // All staked funds withdrawn upfront
      expect(await stRSR.balanceOf(addr2.address)).to.equal(0)
    })
...
})

The test code should be added to the Withdrawals/Unstaking section of the ZZStRSR.test.ts test file.
The test would fail causing Dos when the second address wants to unstake.

Tools Used

Manual Review

Recommended Mitigation Steps

The solution to this bug is a little tricky as the addition of precision to draftRSR has it roles, so protocol could consider updating its documentation to cover this bug or make adjustment as provided in the pushDraft function below which puts the possibility of of lower new total drafts into consideration.

 function pushDraft(address account, uint256 rsrAmount)
        internal
        returns (uint256 index, uint64 availableAt)
    {
        // draftAmount: how many drafts to create and assign to the user
        // pick draftAmount as big as we can such that (newTotalDrafts <= newDraftRSR * draftRate)
        draftRSR += rsrAmount;
        // newTotalDrafts: {qDrafts} = D18{qDrafts/qRSR} * {qRSR} / D18
        uint256 newTotalDrafts = (draftRate * draftRSR) / FIX_ONE;
+++     uint256 draftAmount = newTotalDrafts > totalDrafts ? newTotalDrafts - totalDrafts: totalDrafts - newTotalDrafts;
---        uint256 draftAmount = newTotalDrafts - totalDrafts;
        totalDrafts = newTotalDrafts;

        // Push drafts into account's draft queue
        CumulativeDraft[] storage queue = draftQueues[draftEra][account];
        index = queue.length;

        uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0;
        uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0;
        availableAt = uint64(block.timestamp) + unstakingDelay;
        if (lastAvailableAt > availableAt) {
            availableAt = lastAvailableAt;
        }

---        queue.push(CumulativeDraft(uint176(oldDrafts + draftAmount), availableAt));
+++        queue.push(CumulativeDraft(uint176(newTotalDrafts > totalDrafts ? oldDrafts + draftAmount: oldDrafts - draftAmount), availableAt));
    }

Assessed type

DoS