RSR Stakers Unintentionally Slashed During Collateral Depegging Despite Sufficient Collateral Backing

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/RecollateralizationLib.sol#L300
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/RecollateralizationLib.sol#L317-L321
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/TradeLib.sol#L163
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/TradeLib.sol#L176

Vulnerability details

Description

A collateral that has depegged, can still be considered SOUND, as long as the current price remains within the boundaries of the ORACLE_ERROR. In other words, if, ORACLE_ERROR is 1% at the peg price of 1e8, a collateral can be SOUND down to 0.99e8 and up to 1.01e8. Because of this, a depegged collateral can be used to rebalance() the BackingManager as long as it is the most-in-surplus.

With this, a call to isEnoughToSell() on TradeLib.sol is used to determine if there is a sufficient amount of collateral in the BackingManager.sol to cover the rebalancing.

TradeLib.isEnoughToSell(
                        reg.assets[i],
                        ctx.bals[i].minus(needed),
                        low,
                        ctx.minTradeVolume
                    )

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/RecollateralizationLib.sol#L317-L322

The problem is that isEnoughToSell() takes the current collateral price (in this case, the depegged price) and uses it as the low parameter.
e.g. At peg of 1e8, the low may be 990000000000000000 at a 1% oracle error, however, at a depegged price of 0.99e8, the low is now 980100000000000000

(uint192 low, uint192 high) = reg.assets[i].price();

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

Because the low parameter is now lower that it would normally be, the minTradeSize, used in isEnoughToSell will actually be inflated due to dividing by a smaller price:
e.g. At-peg, minTradeSize = 101010101010101010101011 [1.01e23]
De-peg, minTradeSize = 102030405060708091011122 [1.02e23]

    function minTradeSize(uint192 minTradeVolume, uint192 price) private pure returns (uint192) {
        // {tok} = {UoA} / {UoA/tok}
        uint192 size = price == 0 ? FIX_MAX : minTradeVolume.div(price, CEIL);
        return size != 0 ? size : 1;
    }

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

Consequently, because isEnoughToSell compares whether the ctx.bals[i].minus(needed) is >= minTradeSize, there are scenarios where at-peg isEnoughToSell returns true, utilizing the collateral to rebalance, but when depegged it returns false, resulting in the usage of staked RSR to rebalance.

Impact

Now, for this to occur, a few things must align:

1. The collateral must be the most-in-surplus
2. The price of the collateral must be depegged but still deemed SOUND
3. The amount of the collateral in the BackingManager must be within the specified range.

Due to the above conditions, the likelihood of this playing out is low, however, the amount of RSR that is slashed from stakers can be substantial as it is bounded to the minTradeVolume.

• If minTradeVolume is 1e22, the amount seized is approximately 1.02e22 (10,200 RSR),
• If minTradeVolume is 1e23, the amount seized is approximately 1.02e23 (102,000 RSR),
• If minTradeVolume is 1e24, the amount seized is approximately 1.02e24 (1,020,000 RSR),

As you can see, the amount seized is almost a 1:1 ratio, therefore, the higher the minTradeVolume, the more is seized. Being as how these scale by a factor of 10, the amount seized grows exponentially larger as minTradeVolume increases.

According to the deployment-variable.md:
https://github.com/code-423n4/2024-07-reserve/blob/main/docs/deployment-variables.md
The reasonable range of minTradeVolume is Reasonable range: 1e19 to 1e23. So in the scenario displayed below in the PoC, we are using the highest reasonable value of 1e23.
Note: minTradeVolume can be set all the way up to 1e29
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/Trading.sol#L22

Proof of Concept

To use this PoC, create a new file in test and paste the following. Make sure to set the basket as reweightable in fixtures.ts:
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/test/fixtures.ts#L459

Additionally, this test is designed to pass when depegged, showcasing that RSR is seized. To verify that RSR is not seized when at-peg, but that the collateral (token2) is used to rebalance, just change the oracle price from 0.99e8 to 1e8.

import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
import { BigNumber, constants } from 'ethers'
import hre, { ethers } from 'hardhat'
import { expect } from 'chai'
import { IConfig, ThrottleParams, MAX_THROTTLE_AMT_RATE } from '../common/configuration'
import { CollateralStatus, MAX_UINT256, ONE_PERIOD, ZERO_ADDRESS, TradeKind } from '../common/constants'
import { expectRTokenPrice, setOraclePrice } from './utils/oracles'
import { bn, fp, toBNDecimals } from '../common/numbers'
import {
  ATokenFiatCollateral,
  CTokenFiatCollateral,
  ERC20Mock,
  FacadeTest,
  IAssetRegistry,
  RTokenAsset,
  StaticATokenMock,
  TestIBackingManager,
  TestIBasketHandler,
  TestIMain,
  TestIRToken,
  USDCMock,
  CTokenMock,
  DutchTradeRouter,
  TestIRevenueTrader,
  TestIStRSR,
  TestIBroker,
} from '../typechain'
import {
  advanceTime,
} from './utils/time'
import {
  Collateral,
  defaultFixture,
  IMPLEMENTATION,
} from './fixtures'
import { mintCollaterals } from './utils/tokens'

console.log('IMPLEMENTATION: ', IMPLEMENTATION)


describe(`RTokenP${IMPLEMENTATION} contract`, () => {
  let owner: SignerWithAddress
  let addr1: SignerWithAddress
  let addr2: SignerWithAddress
  let other: SignerWithAddress

  //Trading
  let rTokenTrader: TestIRevenueTrader
  let rsrTrader: TestIRevenueTrader

  // Tokens and Assets
  let initialBal: BigNumber
  let token0: ERC20Mock
  let token1: USDCMock
  let token2: StaticATokenMock
  let token3: CTokenMock
  let tokens: ERC20Mock[]

  let collateral0: Collateral
  let collateral1: Collateral
  let collateral2: ATokenFiatCollateral
  let collateral3: CTokenFiatCollateral
  let basket: Collateral[]
  let rTokenAsset: RTokenAsset
  let rsr: ERC20Mock

  // Config values
  let config: IConfig

  // Main
  let main: TestIMain
  let rToken: TestIRToken
  let facadeTest: FacadeTest
  let assetRegistry: IAssetRegistry
  let backingManager: TestIBackingManager
  let basketHandler: TestIBasketHandler
  let stRSR: TestIStRSR
  let broker: TestIBroker

  beforeEach(async () => {
    ;[owner, addr1, addr2, other] = await ethers.getSigners()

    // Deploy fixture
    ;({
      assetRegistry,
      backingManager,
      basket,
      basketHandler,
      config,
      facadeTest,
      main,
      rToken,
      rTokenAsset,
      rTokenTrader,
      rsr,
      stRSR,
      rsrTrader,
      broker,
    } = await loadFixture(defaultFixture))

    // Get assets and tokens
    collateral0 = <Collateral>basket[0] //@note this is explicitly setting collateral0 to type Collateral || dai
    collateral1 = <Collateral>basket[1] //@note usdc
    collateral2 = <ATokenFiatCollateral>basket[2] //@note adai
    collateral3 = <CTokenFiatCollateral>basket[3] //@note cdai
    token0 = <ERC20Mock>await ethers.getContractAt('ERC20Mock', await collateral0.erc20())
    token1 = <USDCMock>await ethers.getContractAt('USDCMock', await collateral1.erc20())
    token2 = <StaticATokenMock>(
      await ethers.getContractAt('StaticATokenMock', await collateral2.erc20())
    )
    token3 = <CTokenMock>await ethers.getContractAt('CTokenMock', await collateral3.erc20())
    tokens = [token0, token1, token2, token3]

    // Mint initial balances
    initialBal = bn('10000000e18') // 10x the issuance throttle amount
    await mintCollaterals(owner, [addr1, addr2], initialBal, basket) //@note mints all the collaterals in the basket at the initialBal to addr1 and addr2

    await (await ethers.getContractAt('RevenueTraderP1', rTokenTrader.address)).cacheComponents()
    await (await ethers.getContractAt('RevenueTraderP1', rsrTrader.address)).cacheComponents()

  })

  describe("Rebalancing", function () {
    it('Should allow for rsr stakers to be slashed when price de-pegs', async () => {
        const auctionLength = 1800 // 30 minutes
        await broker.connect(owner).setDutchAuctionLength(auctionLength)

        //Setting the minTradeVolume to the max reasonable amount
        await backingManager.connect(owner).setMinTradeVolume(bn('1e23'));
  
        let router: DutchTradeRouter
        let issueAmount = bn('1e18')
        let stakeAmount = bn('10000e18')
        let initiaRSRBal = bn('1000000e18')
  
        //Ensuring that the oracle price starts at the peg of 1e8
        await setOraclePrice(collateral0.address, bn('1e8'));
  
        router = await (await ethers.getContractFactory('DutchTradeRouter')).deploy()

        //Token approvals
        await token2.connect(addr1).approve(router.address, constants.MaxUint256)
        await rToken.connect(addr1).approve(router.address, constants.MaxUint256)
        await token0.connect(addr1).approve(rToken.address, constants.MaxUint256)
        await token2.connect(addr1).approve(rToken.address, constants.MaxUint256)
        await rsr.connect(owner).mint(addr1.address, initiaRSRBal);
        await rsr.connect(addr1).approve(stRSR.address, stakeAmount.mul(2));
        await stRSR.connect(addr1).stake(stakeAmount.mul(2));
  
        //Minting token2 (essentially the backing buffer) to the backingManager 
        //IMPORTANT: For this to work, the amount of token2 in backingManager must be within a range:
            //Top of the range: De-pegged down 1% at 0.99e8 = 102030405060708091011122 [1.02e23]
            //Bot of the range: At-peg                      = 101010101010101010101011 [1.01e23]
                //e.g. 1.01e23 < 1.011e23 < 1.02e23
        await token2.connect(owner).mint(backingManager.address, bn('1.011e23'))
  
        //Setting the baskets, issuing rTokens, and changing the weights in order to de-collateralize so that rebalancing is possible
        await basketHandler.connect(owner).forceSetPrimeBasket([token0.address], [bn('0.5e18')])
        await basketHandler.connect(owner).refreshBasket()
        await advanceTime(Number(config.warmupPeriod) + 1)
  
        await rToken.connect(addr1).issue(issueAmount);

        expect(await basketHandler.fullyCollateralized()).to.equal(true)

  
        await basketHandler.connect(owner).forceSetPrimeBasket([token0.address], [bn('0.3e18')])
        await basketHandler.connect(owner).refreshBasket()
  
        await rsr.connect(owner).mint(addr1.address, initiaRSRBal);
        await rsr.mint(backingManager.address, initiaRSRBal.mul(10));
        await backingManager.forwardRevenue([rToken.address])
  
        await rToken.connect(addr1).issue(issueAmount.mul(2));

        expect(await basketHandler.fullyCollateralized()).to.equal(true)

  
        await basketHandler.connect(owner).forceSetPrimeBasket([token0.address], [bn('0.4e18')])
        await basketHandler.connect(owner).refreshBasket()

        expect(await basketHandler.fullyCollateralized()).to.equal(false)

        //Change price here to 1e8 to test at-peg case
        //Setting the price to de-peg
        await setOraclePrice(collateral0.address, bn('0.99e8'));


        const rsrBalanceBeforeSeizure = await rsr.balanceOf(stRSR.address)
        const seizedRSR = bn('102030405060708091011125') // [1.02e23] || This is the amount of RSR that will be seized from the stRSR at the current minTradeVolume of 1e23

        console.log("Token2 balance in backingManager before rebalancing: ", await token2.balanceOf(backingManager.address))
        console.log("RSR balance in StRSR before rebalancing: ", await rsr.balanceOf(stRSR.address))
        console.log("******************REBALANCING ********************");
        await backingManager.rebalance([TradeKind.DUTCH_AUCTION])
        console.log("Token2 balance in backingManager after rebalancing: ", await token2.balanceOf(backingManager.address))
        console.log("RSR balance in StRSR after rebalancing: ", await rsr.balanceOf(stRSR.address))
        //This is the expected RSR balance in the stRSR contract after rsr is seized
        const rsrBalanceAfterSeizure = rsrBalanceBeforeSeizure.sub(seizedRSR)

        //This check will fail if the oracle price is at peg at 1e8
        expect (await rsr.balanceOf(stRSR.address)).to.equal(rsrBalanceAfterSeizure)

      })
  })
})

The logs when de-pegged:

Token2 balance in backingManager before rebalancing:  BigNumber { value: "101100000000000000000000" }
RSR balance in StRSR before rebalancing:  BigNumber { value: "10020000000000000000000000" }
******************REBALANCING ********************
Token2 balance in backingManager after rebalancing:  BigNumber { value: "101100000000000000000000" }
RSR balance in StRSR after rebalancing:  BigNumber { value: "9917969594939291908988875" }
      ✔ Should allow for rsr stakers to be slashed when price de-pegs (3854ms)

The logs when at-peg:

Token2 balance in backingManager before rebalancing:  BigNumber { value: "101100000000000000000000" }
RSR balance in StRSR before rebalancing:  BigNumber { value: "10020000000000000000000000" }
******************REBALANCING ********************
Token2 balance in backingManager after rebalancing:  BigNumber { value: "0" }
RSR balance in StRSR after rebalancing:  BigNumber { value: "10020000000000000000000000" }

Tools Used

Manual Analysis/Hardhat

Recommended Mitigation Steps

My recommendation is to make use of the issuancePremium() function that has been added on to BasketHandler.sol to scale the low from the actual low de-pegged price to the target low at-peg price. Ideally, this calculation would be done on the nextTradePair() function, however this will likely result in stack-to-deep errors. The next best option I can think of will be to have the low and the high returned to the nextTradePair() function already scaled.

-(uint192 low, uint192 high) = reg.assets[i].price();
+(uint192 scaledLow, uint192 scaledHigh) = reg.assets[i].scaledPrice();

The scaledPrice() function will start with the reg.assets[i].price() and will utilize a premium to determine if it is under-pegged, similar to the checks here:
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L440-L443
and here:
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L500-L503

If depegged, scaledPrice() will return the scaled values, but if not depegged, it will return the values from reg.assets[i].price()

Assessed type

Error