Oracle timeout at rebalance will result in a sell-off of all RSRs at 0 price

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/mixins/RecollateralizationLib.sol#L394-L413

Vulnerability details

When creating the trade for rebalance, the RecollateralizationLibP1.nextTradePair uses (uint192 low, uint192 high) = rsrAsset.price(); // {UoA/tok} to get the rsr sell price. And the rsr assert is a pure Assert contract, which price() function will just return (0, FIX_MAX) if oracle is timeout:

function price() public view virtual returns (uint192, uint192) {
    try this.tryPrice() returns (uint192 low, uint192 high, uint192) {
        assert(low <= high);
        return (low, high);
    } catch (bytes memory errData) {
        ...
        return (0, FIX_MAX);
    }
}

The trade.sellAmount will be all the rsr in the BackingManager and stRSR:

uint192 rsrAvailable = rsrAsset.bal(address(ctx.bm)).plus(
    rsrAsset.bal(address(ctx.stRSR))
);
trade.sellAmount = rsrAvailable;

It will be cut down to a normal amount fit for buying UoA amount in the trade.prepareTradeToCoverDeficit function.

But if the rsr oracle is timeout and returns a 0 low price. The trade req will be made by trade.prepareTradeSell, which will sell all the available rsr at 0 price.

Note that the SOUND colls won't be affected by the issue because the sell amount has already been cut down by basketsNeeded.

Impact

Loss huge amount of rsr in the auction. When huge amounts of assets are auctioned off at zero, panic and insufficient liquidity make the outcome unpredictable.

Proof of Concept

POC git diff test/Recollateralization.test.ts

diff --git a/test/Recollateralization.test.ts b/test/Recollateralization.test.ts
index 86cd3e88..15639916 100644
--- a/test/Recollateralization.test.ts
+++ b/test/Recollateralization.test.ts
@@ -51,7 +51,7 @@ import {
 import snapshotGasCost from './utils/snapshotGasCost'
 import { expectTrade, getTrade, dutchBuyAmount } from './utils/trades'
 import { withinQuad } from './utils/matchers'
-import { expectRTokenPrice, setOraclePrice } from './utils/oracles'
+import { expectRTokenPrice, setInvalidOracleTimestamp, setOraclePrice } from './utils/oracles'
 import { useEnv } from '#/utils/env'
 import { mintCollaterals } from './utils/tokens'
 
@@ -797,6 +797,166 @@ describe(`Recollateralization - P${IMPLEMENTATION}`, () => {
   })
 
   describe('Recollateralization', function () {
+    context('With simple Basket - Two stablecoins', function () {
+      let issueAmount: BigNumber
+      let stakeAmount: BigNumber
+
+      beforeEach(async function () {
+        // Issue some RTokens to user
+        issueAmount = bn('100e18')
+        stakeAmount = bn('10000e18')
+
+        // Setup new basket with token0 & token1
+        await basketHandler.connect(owner).setPrimeBasket([token0.address, token1.address], [fp('0.5'), fp('0.5')])
+        await basketHandler.connect(owner).refreshBasket()
+
+        // Provide approvals
+        await token0.connect(addr1).approve(rToken.address, initialBal)
+        await token1.connect(addr1).approve(rToken.address, initialBal)
+
+        // Issue rTokens
+        await rToken.connect(addr1).issue(issueAmount)
+
+        // Stake some RSR
+        await rsr.connect(owner).mint(addr1.address, initialBal)
+        await rsr.connect(addr1).approve(stRSR.address, stakeAmount)
+        await stRSR.connect(addr1).stake(stakeAmount)
+      })
+      
+      it('C4M7', async () => {
+        // Register Collateral
+        await assetRegistry.connect(owner).register(backupCollateral1.address)
+  
+        // Set backup configuration - USDT as backup
+        await basketHandler
+          .connect(owner)
+          .setBackupConfig(ethers.utils.formatBytes32String('USD'), bn(1), [backupToken1.address])
+        
+        // Set Token0 to default - 50% price reduction
+        await setOraclePrice(collateral0.address, bn('0.5e8'))
+  
+        // Mark default as probable
+        await assetRegistry.refresh()
+        // Advance time post collateral's default delay
+        await advanceTime((await collateral0.delayUntilDefault()).toString())
+  
+        // Confirm default and trigger basket switch
+        await basketHandler.refreshBasket()
+  
+        // Advance time post warmup period - SOUND just regained
+        await advanceTime(Number(config.warmupPeriod) + 1)
+  
+        const initToken1B = await token1.balanceOf(backingManager.address);
+        // rebalance
+        const token1Decimal = 6;
+        const sellAmt: BigNumber = await token0.balanceOf(backingManager.address)
+        const buyAmt: BigNumber = sellAmt.div(2)
+        await facadeTest.runAuctionsForAllTraders(rToken.address);
+        // bid
+        await backupToken1.connect(addr1).approve(gnosis.address, sellAmt)
+        await gnosis.placeBid(0, {
+          bidder: addr1.address,
+          sellAmount: sellAmt,
+          buyAmount: buyAmt,
+        })
+        await advanceTime(config.batchAuctionLength.add(100).toString())
+        // await facadeTest.runAuctionsForAllTraders(rToken.address);
+        const rsrAssert = await assetRegistry.callStatic.toAsset(rsr.address);
+        await setInvalidOracleTimestamp(rsrAssert);
+        await expectEvents(facadeTest.runAuctionsForAllTraders(rToken.address), [
+          {
+            contract: backingManager,
+            name: 'TradeSettled',
+            args: [anyValue, token0.address, backupToken1.address, sellAmt, buyAmt],
+            emitted: true,
+          },
+          {
+            contract: backingManager,
+            name: 'TradeStarted',
+            args: [anyValue, rsr.address, backupToken1.address, stakeAmount, anyValue], // sell 25762677277828792981
+            emitted: true,
+          },
+        ])
+  
+        // check
+        console.log(await token0.balanceOf(backingManager.address));
+        const currentToken1B = await token1.balanceOf(backingManager.address);
+        console.log(currentToken1B);
+        console.log(await backupToken1.balanceOf(backingManager.address));
+        const rsrB = await rsr.balanceOf(stRSR.address);
+        console.log(rsrB);
+  
+        // expect
+        expect(rsrB).to.eq(0);
+      })
+    })
+
     context('With very simple Basket - Single stablecoin', function () {
       let issueAmount: BigNumber
       let stakeAmount: BigNumber

run test:

PROTO_IMPL=1 npx hardhat test --grep 'C4M7' test/Recollateralization.test.ts

log:

  Recollateralization - P1
    Recollateralization
      With simple Basket - Two stablecoins
BigNumber { value: "0" }
BigNumber { value: "50000000" }
BigNumber { value: "25000000000000000000" }
BigNumber { value: "0" }

Tools Used

Manual review

Recommended Mitigation Steps

Using lotPrice or just revert for rsr oracle timeout might be a good idea.

Assessed type

Context

[tbrent]

Hmm, interesting case.

There are two types of auctions that can occur: batch auctions via GnosisTrade, and dutch auctions via DutchTrade.

Batch auctions via GnosisTrade are good at discovering prices when price is unknown. It would require self-interested parties to be offline for the entire duration of the batch auction (default: 15 minutes) in order for someone to get away with buying the RSR for close to 0.

Dutch auctions via DutchTrade do not have this problem because of an assert that reverts at the top of the contract.

I'm inclined to dispute validity, but I also agree it might be strictly better to use the lotPrice(). When trading out backing collateral it is important to sell it quickly and not have to wait for lotPrice() to decay sufficiently, but this is not true with RSR. For RSR it might be fine to wait as long as a week for the lotPrice() to fall to near 0.

This would then allow dutch auctions via DutchTrade to be used when RSR's oracle is offline.

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory