the sale sequence of assets is incorrect when default

[code423n4]

Lines of code

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

Vulnerability details

According to the risk evaluation from the official app:

Please carefully evaluate the RToken before choosing to stake your RSR here. If any of the various collaterals of this RToken default, then the staked RSR will be the first funds that get auctioned off to make up the difference for RToken holders.

The staked RSR will be the first funds to be sold when RToken default. As recollateralization and Sharing most of the profits, the RSR staker should be slashed preferentially before RToken holders.

When coll has defaulted, the correct sale sequence should be:

defaulted coll -> rsr -> SOUND coll

But because of RecollateralizationLibP1.nextTradePair leaves the rsr calculation at the end of the function, when stake rsr amount can't increase the range.top to the basketsNeeded, the SOUND colls will be sold after the defaulted colls. And the rsr will be sold last.

Impact

Break the promise about rsr overcollateralization, which means the rtoken holders suffer from more risk of default than they should have but dont get more revenues.

Proof of Concept

POC git patch test/Recollateralization.test.ts

diff --git a/test/Recollateralization.test.ts b/test/Recollateralization.test.ts
index 86cd3e88..75aef2c7 100644
--- a/test/Recollateralization.test.ts
+++ b/test/Recollateralization.test.ts
@@ -797,6 +797,100 @@ 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('10e18')
+
+        // 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('C4M6', 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);
+        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, token1.address, backupToken1.address, anyValue, 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(currentToken1B).to.lt(initToken1B);
+        expect(rsrB).to.eq(stakeAmount);
+      })
+    
+    })
+
     context('With very simple Basket - Single stablecoin', function () {
       let issueAmount: BigNumber
       let stakeAmount: BigNumber

RUN test:

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

log:

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

Tools Used

Manual review

Recommended Mitigation Steps

Separate branching for SOUND collateral in the RecollateralizationLibP1.nextTradePair

Assessed type

Context

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[tbrent]

It is preferred to minimize RSR selling and to sell off excess SOUND collateral before RSR in order to prevent double trading. If we were to change the order in the way described, then in the event of a default there would be large RSR selling, possibly followed by a similar amount of SOUND collateral selling for RSR, which would then be treated as reward RSR and dripped out to stakers. This is less good than simply using the SOUND collateral in the first place, even though technically this uses revenue that could be destined for either RToken holders or RSR stakers.

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[5z1punch]

I think there is a misunderstand about he scenario I presented. I agree that "sell off excess SOUND collateral before RSR in order to prevent double trading". But the "excess SOUND collateral" should be the part of collateral hold by BackingManager - rToken.basketsNeeded amount. The current code leads to early selling the collateral which is equal to rToken.basketsNeeded when the rToken is undercollateralized.

Although the effect on the final state is little, this causes users who would have the opportunity to partially redeem their collateral at the rsr auction stage to lose a portion of their principal.

The RSR stakeAmount in the poc test is 10, the sale sequence is: defaulted coll token 0 -> SOUND coll token1 -> rsr The token1 is sold before rsr, even though the baskets needs 50 token1 and there is not any excess SOUND collateral.

But if RSR stakeAmount is changed to 25, the sale sequence will be normal: defaulted coll token 0 -> rsr -> rToken is fullyCollateralized

[0xean]

cc @tbrent

[tbrent]

Hmm this is a good and subtle point. It took me a while to see the argument behind it. In short, if the protocol expects to end up undercollateralized in the best-case, then selling RSR first has no downside and maximizes RToken redeemCustom() value. The only case where this has a downside is when the clearing price of the RSR auction ends up being better than the best-case price (out-of-band outcome), which can cause double-trading of the value gotten from the unexpected gain.

I don't think this should be considered Medium though since it does not impact the ultimate redemption value of the RToken, only the mid-rebalancing value. It is not actually a stated property of the protocol that RToken redemption value during rebalancing must be greedily maximized. It is important that the RToken redemption value during rebalancing be up-only in order to disincentivize bank runs, but I think we have that property regardless of which order of sale is used. If this is not the case please correct me and then I would be happy to accept this as Medium.

Not sure whether we will mitigate yet, so just acknowledging for now.

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[c4-sponsor]

tbrent marked the issue as disagree with severity

[0xean]

@5z1punch care to comment on

It is important that the RToken redemption value during rebalancing be up-only in order to disincentivize bank runs, but I think we have that property regardless of which order of sale is used. If this is not the case please correct me and then I would be happy to accept this as Medium.

before I make a final call here?

[5z1punch]

@0xean Alright, Im sure that now there is not any misunderstand about the issue. And as the comment It is not actually a stated property of the protocol that RToken redemption value during rebalancing must be greedily maximized, I agree that it should be a QA.

[5z1punch]

Just a reminder, the tag unsatisfactory and Med should be removed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)