Broken assumptions can lead to the inability to seize RSR

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L424

Vulnerability details

Description

The seizeRSR function takes RSR from the staking contract when BackingManager wants to sell RSR, but it does not have enough. In this case, the stakers can lose a portion of their stake in order to keep the system healthy. However, an issue arises from broken assumptions about stakeRSR and totalStakes, which will make the contract unable to seize due to revert.

Proof of concept

Let's assume the system becomes unhealthy, and the stakers will start unstaking their tokens in fear of an upcoming reset/seize. Then, the expected event comes, and the BackingManager tries to seize RSR from the staking contract.

This action is frontran however, with the last staker who:

1. unstakes everything
2. stakes one token
3. stakes one token again

Let's examine how does this break seizeRSR.

1. During the unstake call:
   • _payoutRewards is called. Since totalStakes is still above 1e18, stakeRSR can be increased and become greater than totalStakes. Let's assume that totalStakes would be 1000e18 and stakeRSR would end at 1001e18 after this step.
   • Now, the stakeRate would be updated. Since stakeRSR is not zero yet, and so is not totalStakes, it would be set to

     uint192((totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR)

     which translates to

     uint192((1000e18 * 1e18 + (1001e18 - 1)) / 1001e18)

     resulting in 999000999000999001

   • totalStakes would be decreased in _burn by 1000e18 to zero, which would result in stakeRSR being set to zero
2. During the stake call:
   • payoutRewards is skipped because it was already called in this block
   • newTotalStakes in mintStakes is calculated as (stakeRate * newStakeRSR) / FIX_ONE, which translates to (stakeRate * (stakeRSR + rsrAmount)) / FIX_ONE which translates to (999000999000999001 * (0 + 1)) / 1e18 resulting in 0, meaning 0 tokens are minted and totalStakes remains 0
   • stakeRSR is increased by 1
3. The second stake call will have the same effects
   • after this call, the stakeRate is 999000999000999001, totalStakes is 0, and stakeRSR is 2 (note that this breaks this assumption)
4. Now we examine the important parts of the seizeRSR call
   • stakeRSRToTake is set to (2 * rsrAmount + (rsrBalance - 1)) / rsrBalance, which will result in 1 every time the we are not seizing more than half of the total balance of the contract
   • stakeRSR is therefore set to 1, and since it is non-zero, stakeRate is updated to uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR), which translates to uint192((1e18 * 0 + (1 - 1)) / 1) which comes to 0
   • the rest of the call is not important until it comes to

     emit ExchangeRateSet(initRate, exchangeRate());

   • Here, we can see that the call returns

     return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate;

   • since stakeRate was updated to 0 before, the call will revert here

Impact and likelihood

The likelihood of this issue is LOW. The impact seems somewhere between HIGH and MEDIUM since a necessary rebalance action can be DoSed until it is noticed and an action is taken. Considering that the issue is possible due to a broken invariant, the severity should be judged as MEDIUM.

Recommendation

One way to fix the issue would be to enforce the invariant if totalStakes == 0, then stakeRSR == 0. This could be done by assuring that amount is not 0 in _mint.

    function _mint(address account, uint256 amount) internal virtual {
        _notZero(account);
+       _notZero(amount);
        assert(totalStakes + amount < type(uint224).max);

        stakes[era][account] += amount;
        totalStakes += amount;

        emit Transfer(address(0), account, amount);
        _afterTokenTransfer(address(0), account, amount);
    }

Another mitigation could be to update the seizeRSR function to update the stakeRate only if both stakeRSR and totalStakes are non-zero or update the stakeRate to FIX_ONE if either of these two is zero.

Assessed type

DoS

[tbrent]

This is a plausible issue. We would like to request a code PoC from the warden, since it is about a very specific numeric case.

[thereksfour]

Since the initial judging time is not enough to verify it, consider it valid first and keep open, and wait for the POC from warden. And may invalidate it without further proof.

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report

[akshatmittal]

Just noting that we currently think this is not possible but might be plausible in super specific circumstances which is why we're requesting the PoC.

[coreggon11]

Hi @akshatmittal @tbrent

The PoC in the issue is wrong, however the issue exists, below I attach the updated PoC with a test proving the issue. Under those conditions seizeRSR will be dosed due to a broken protocol invariant.

Let's assume the system becomes unhealthy, and the stakers will start unstaking their tokens in fear of an upcoming reset/seize. Then the following chain of action happens:

1. The last staker unstakes everything but one token so _payoutRewards updates stakeRSR
2. Stake rate is below 5e17 - this can happen natuarally or forcefully (in our scenario everybody unstaked so we can influence it in some way)
3. BackingManager tries to seize RSR from the staking contract, but this transaction is frontran with another transaction in which
   1. The last staker unstakes the last token
   2. The last staker stakes 2 wei

What will happen is that

1. During the unstake call:
   • _payoutRewards is called. Since totalStakes is still above 1e18, stakeRSR can be increased and become greater than totalStakes.
   • This will influence the new stakeRate. If this becomes less than 5e17, the problem arises
2. During the stake call:
   • payoutRewards is skipped because it was already called in this block
   • newTotalStakes in mintStakes is calculated as (stakeRate * newStakeRSR) / FIX_ONE, which translates to (stakeRate * (stakeRSR + rsrAmount)) / FIX_ONE which translates to (499999999999999999 * (0 + 2)) / 1e18 resulting in 0, meaning 0 tokens are minted and totalStakes remains 0
   • stakeRSR is increased by 2
   • Note that 499999999999999999 is the greatest possible value of stakeRate when the issue arises
   • in the end we can see that totalStakes is 0, and stakeRSR is 2 (note that this breaks this assumption)
3. Now we examine the important parts of the seizeRSR call
   • stakeRSRToTake is set to (2 * rsrAmount + (rsrBalance - 1)) / rsrBalance, which will result in 1 every time the we are not seizing more than half of the total balance of the contract
   • stakeRSR is therefore set to 1, and since it is non-zero, stakeRate is updated to uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR), which translates to uint192((1e18 * 0 + (1 - 1)) / 1) which comes to 0
   • the rest of the call is not important until it comes to

     emit ExchangeRateSet(initRate, exchangeRate());

   • Here, we can see that the call returns

     return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate;

   • since stakeRate was updated to 0 before, the call will revert here

The following test showcases the issue with different numbers:

it.only("Unable to seize RSR", async () => {
  const stakeAmt: BigNumber = bn("5000e18");
  const unstakeAmt: BigNumber = bn("4999e18");
  const one: BigNumber = bn("1e18");
  const rewards: BigNumber = bn("100e18");
  const seizeAmount: BigNumber = bn("2499e18");

  // 1. Stake
  await rsr.connect(addr1).approve(stRSR.address, stakeAmt);
  await stRSR.connect(addr1).stake(stakeAmt);

  // 2. Decrease stakeRate to ~5e17 - 1
  await rsr.connect(owner).mint(stRSR.address, rewards);
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 1);
  await stRSR.payoutRewards();
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 86400);
  await stRSR.payoutRewards();

  // 3. Unstake everything but 1e18
  await stRSR.connect(addr1).unstake(unstakeAmt);
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 172800);

  // 4. Unstake the last token then stake 2 wei

  // everything must happen in 1 tx thats why we deploy `SeizeAttacker`
  const SeizeAttackerFactory = await ethers.getContractFactory("SeizeAttacker");
  let seizeAttacker = await SeizeAttackerFactory.deploy();

  // transfer stRSR to the seize attacker so it can unstake
  await stRSR.connect(addr1).transfer(seizeAttacker.address, one);
  await rsr.connect(owner).mint(seizeAttacker.address, 2);
  await seizeAttacker.doIt(stRSR.address, rsr.address, one);

  // 5. seize rsr fails
  await setStorageAt(stRSR.address, 256, addr1.address); // set addr1 as backing manager so we can call seize rsr easily
  await stRSR.connect(addr1).seizeRSR(seizeAmount);
});

We will see that the test will fail with the following output

  Error: VM Exception while processing transaction: reverted with panic code 0x12 (Division or modulo division by zero)

[akshatmittal]

Hey @coreggon11, thanks for that. Can you please share the code for SeizeAttacker contract and replace the last call with impersonating BackingManager instead? (So I'm able to reproduce this)

[coreggon11]

@akshatmittal mb, forgot about that

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

interface IStRSR {
    function stake(uint256 rsrAmount) external;
    function unstake(uint256 stakeAmount) external;
}

interface RSR {
    function approve(address who, uint256 amount) external;
}

contract SeizeAttacker {

    function doIt(address stRSR,address rsr, uint amount) external {
        // unstake
        IStRSR(stRSR).unstake(amount);
        // approve
        RSR(rsr).approve(stRSR, 2);
        // stake 1
        IStRSR(stRSR).stake(2);
    }

}

In the test we set addr1 as backing manager so addr1 can call the method, the test should work)

[akshatmittal]

Accepted.

Specifically, want to point out that following must be true:

1. Last staker in the pool.
2. stakeRate is < 5e17 - 1
3. Alone has > minTradeVolume worth of RSR, or total drafts > minTradeVolume. (Acceptable Values)
4. Able to frontrun the seize call.

[jorgect207]

Hey @thereksfour; thank so much for your time.

Since to this issue be possible attacker have to as the warden explain:

1. unstakes everything
2. stakes one token
3. stakes one token again

This is what im talking about in my report #79 validation repo that why i consider my report is duplicate of this or at least partial 50.

[coreggon11]

@akshatmittal the volume can be cumulated with other stakers who already unstaked) The main problem here is that we are breaking a protocol variant, as shown in the poc, hence the code behaves unexpectedly.