Reserve Protocol - Invitational - 0xA5DF's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L391-L392

Vulnerability details

quoteCustomRedemption() works under the assumption that the maximum size of the erc20sAll should be assetRegistry.size(), however there can be cases where an asset was unregistered but still exists in an old basket, making the size of the old basket greater than assetRegistry.size(). In that case the function will revert with an index out of bounds error.

Impact

Users might not be able to use redeemCustom when needed.

I think this should be considered high severity, since being able to redeem the token at all time is an essential feature for the protocol that's allowed also while frozen. Not being able to redeem can result in a depeg or in governance becoming malicious and stealing RToken collateral.

Proof of Concept

Consider the following scenario:

• RToken deployed with 0.9 USDC, 0.05 USDT, 0.05 DAI
• Governance passed a vote to change it to 0.9 DAI and 0.1 USDC and un-register USDT
• Trading is paused before execution, so the basket switch occurs but the re-balance can't be executed. Meaning the actual assets that the backing manager holds are in accordance with the old basket
• A user wants to redeem using the old basket, but custom redemption reverts

As for the revert:

• erc20sAll is created here with the length of assetRegistry.size(), which is 2 in our case.
• Then in this loop the function tries to push 3 assets into erc20sAll which will result in an index-out-of-bonds error

(the function doesn't include in the final results assets that aren't registered, but it does push them too into erc20sAll)

Recommended Mitigation Steps

Allow the user to specify the length of the array erc20sAll to avoid this revert

Assessed type

Other

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L441-L444 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L457-L460

Vulnerability details

When RSR seizure occurs the staking and drafting rate is adjusted accordingly, if any of those rates is above some threshold then a new era begins (draft or staking era accordingly), wiping out all of the holdings of the current era. The assumption is that if the rate is above the threshold then there's not much staking or drafts left after the seizure (and therefore it makes sense to begin a new era). However, there might be a case where a previous seizure has increased the staking/draft rate close to the threshold, and then even a small seizure would make it cross this threshold. In that case the total value of staking or drafts can be very high, and they will all be wiped out by starting a new era.

Impact

Stakers will lose their holdings or pending drafts.

Proof of Concept

Consider the following scenario:

• Max stake rate is 1e9
• A seizure occurs and the new rate is now 91e7
• Not much staking is left after the seizure, but as time passes users keep staking bring back the total stakes to a significant value
• A 10% seizure occurs, this causes the staking rate to cross the threshold (getting to 1.01e9) and start a new era

This means the stakings were wiped out despite holding a significant amount of value, causing a loss for the holders.

Recommended Mitigation Steps

This one is a bit difficult to mitigate. One way I can think of is to add a 'migration' feature, where in such cases a new era would be created but users would be able to transfer the funds that they held in the previous era into the new era. But this would require some significant code changes and checking that this doesn't break anything or introduces new bugs.

Assessed type

Other

Lines of code

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

Vulnerability details

This one was reported in the first contest, it was mitigated but a code change that was made since then brings it back again.

Impact

Users who stake while frozen would get a share of the rewards for the period since the last call to payoutRewards().

This means that in a case of a long freeze (e.g. a week or a few weeks) - users who stake right before the freeze ends would get a reward as if they've staked before the freeze started. This comes at the expense of the stakers who have staked before the freezing begun.

Proof of Concept

While frozen the _payoutRewards() isn't called:

    function stake(uint256 rsrAmount) public {
        require(rsrAmount > 0, "Cannot stake zero");

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

Not paying out the rewards meaning that the next time _payoutRewards() would be called the stakers would get a reward as if they've staked at the last time it was called.

Recommended Mitigation Steps

Consider paying out rewards also while frozen

Assessed type

Other

Lines of code

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

Vulnerability details

The new cancelUnstake() function allows users to cancel their unstaking, by taking the user's drafts and minting it again. However, since the _payoutRewards() isn't being called this means that the user would get rewards for the period between the last time _payoutRewards() was called and the time of cancellation.

Impact

Users would get rewards they don't deserve, at the expense of other stakers.

Proof of Concept

cancelUnstake() doesn't call _payoutRewards(), this means that the minting part would be done with the old staking rate. Using an old staking rate means that the user would get a share of the rewards since the last time _payoutRewards() was called.

Recommended Mitigation Steps

Call _payoutRewards() before the minting

Assessed type

Other

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L87-L96

Vulnerability details

During a Dutch Auction, if a user places a bid, the trade is settled in the same transaction. As part of this process, the backing manager tries to call the rebalance() function again. The call to rebalance() is wrapped in a try-catch block, if an error occurs and the error data is empty, the function will revert.

The assumption is that if the error data is empty that means it was due to an out-of-gas error, this assumption isn't always true as mentioned in a previous issue (that wasn't mitigated). In the case of this issue, this can result in a case where users can't bid on an auction for some time, ending up selling an asset for a price lower than the market price.

Impact

Protocol's assets will be auctioned for a price lower than the market price

Proof of Concept

Consider the following scenario:

• Chainlink announces that an oracle will get deprecated
• Governance passes a proposal to update the asset registry with a new oracle
• A re-balancing is required and executed with a Dutch Auction
• The oracle deprecation happens before the auction price reaches a reasonable value
• Any bid while the oracle is deprecated will revert
• Right before the auction ends the proposal to update the asset becomes available for execution (after the timelock delay has passed). Somebody executes it, bids, and enjoys the low price of the auction.

Recommended Mitigation Steps

On top of checking that the error data is empty, compare the gas before and after to ensure this is an out-of-gas error.

Assessed type

Error

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L89-L93 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L106-L110

Vulnerability details

At the mitigation contest there was an issue regarding the basketHandler.quantity() call at the unregistration process taking up all gas. As a mitigation to that issue the devs set aside some gas and use the remaining to do that call. This opens up to a new kind of attack, where a attacker can cause the call to revert by not supplying enough gas to it.

Impact

This can cause the basket to get disabled, which would require a basket refresh. After a basket refresh is done, an additional warmup period has to pass for some functionality to be available again (issuance, rebalancing and forwarding revenue). In some cases this might trigger a basket switch that would require the protocol to rebalance via trading, trading can have some slippage which can cause a loss for the protocol.

Proof of Concept

The quantity() function is being called with the amount of gas that _reserveGas() returns

If an attacker causes the gas to be just right above GAS_TO_RESERVE the function would be called with 1 unit of gas, causing it to revert:

    function _reserveGas() private view returns (uint256) {
        uint256 gas = gasleft();
        require(gas > GAS_TO_RESERVE, "not enough gas to unregister safely");
        return gas - GAS_TO_RESERVE;
    }

Regarding the unnecessary trade, consider the following scenario:

• The basket has USDC as the main asset and DAI as a backup token
• A proposal to replace the backup token with USDT was raised
• A proposal to unregister BUSD (which isn't part of the basket) was raised too
• USDC defaults and DAI kicks in as the backup token
• Both proposals are now ready to execute and the attacker executes the backup proposal first, then the unregister while disabling the basket using the bug in question
• Now, when the basket is refreshed DAI will be replaced with USDT, making the protocol to trade DAI for USDT

The refresh was unnecessary and therefore the trade too.

Recommended Mitigation Steps

Reserve gas for the call as well:

    function _reserveGas() private view returns (uint256) {
        uint256 gas = gasleft();
-        require(gas > GAS_TO_RESERVE, "not enough gas to unregister safely");
+        require(gas >= GAS_TO_RESERVE + MIN_GAS_FOR_EXECUTION, "not enough gas to unregister safely");
        return gas - GAS_TO_RESERVE;
    }

Disclosure: this issue was mentioned in the comments to the issue in the mitigation contest, however since this wasn't noticed by the devs and isn't part of the submission I don't think this should be considered a known issue

Assessed type

Other

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L245-L338 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L437-L440

Vulnerability details

Custom redemption allows to redeem RToken in exchange of a mix of previous baskets (as long as it's not more than the prorata share of the redeemer). The assumption is that previous baskets aren't worth more than the target value of the basket. However, a previous basket can contain a collateral that depegged upwards and is now actually worth more than its target.

Impact

Funds that are supposed to go revenue traders would be taken by an attacker redeeming RToken

Proof of Concept

The following code shows that when a depeg occurs the collateral becomes IFFY (which means it'll be disabled after a certain delay):

            // If the price is below the default-threshold price, default eventually
            // uint192(+/-) is the same as Fix.plus/minus
            if (pegPrice < pegBottom || pegPrice > pegTop || low == 0) {
                markStatus(CollateralStatus.IFFY);
            } else {
                markStatus(CollateralStatus.SOUND);
            }

Consider the following scenario:

• Basket contains token X which is supposed to be pegged to c
• Token X depegs upwards and is now worth 2c
• After delayUntilDefault passes the basket gets disabled
• A basket refresh is executed (can be done by anybody) and token Y kicks in as the backup token
• Half of token X is now traded for the required Y tokens
• The other half should go to revenue traders (rsr trader and Furnace), but before anyone calls ‘forewardRevenue’ the attacker calls custom redemption with half from the current basket and half of the previous one
• The user would get 0.5X+0.5Y per each RToken which is worth 1.5c

Recommended Mitigation Steps

When doing custom redemption check that the collateral used is sound or at least check that the price isn't higher then the peg price.

Assessed type

Other

A redeemer might get 'front-run' by a freezer

Before redemption furnace.melt() is called, which increases a bit the amount of assets the redeemer gets in return. While frozen the melt() call will revert, but since the call is surrounded by a try-catch block the redemption will continue.

This can lead to a case where a user sends out a redeem tx, expecting melt to be called by the redeem function, but before the tx is executed the protocol gets frozen. This means the user wouldn't get the additional value they expected to get from melting (and that they can get if they choose to wait till after the freeze is over).

If the last time melt() was called wasn't long enough then the additional value from melting wouldn't be that significant. But there can be cases where it can be longer - e.g. low activity or after a freeze (meaning there are 2 freezes one after the other and a user is attempting to redeem between them).

As a mitigation allow the user to specify if they're ok with skipping the call to melt(), if they didn't specifically allow it then revert.

Leaky refresh math is incorrect

leakyRefresh() keeps track of the percentage that was withdrawn since last refresh by adding up the current percentage that's being withdrawn each time. However, the current percentage is being calculated as part of the current totalRSR, which doesn't account for the already withdrawn drafts.

This will trigger the withdrawalLeak threshold earlier than expected. E.g. if the threshold is set to 25% and 26 users try to withdraw 1% each - the leaky refresh would be triggered by the 23rd person rather than by the 26th.

Reorg attack

Reorg attacks aren't very common on mainnet (but more common on L2s if the protocol intends to ever launch on them), but they can still happen (there was a 7 blocks reorg on the Beacon chain before the merge). It can be relevant in the following cases (I've reported a med separately, the followings are the ones I consider low):

• RToken deployment - a user might mint right after the RToken was deployed, a reorg might be used to deploy a different RToken and trap the users' funds in it (since the deployer becomes the owner).
• Dutch Auction - a reorg might switch the addresses of 2 auctions, causing the user to bid on the wrong auction
• Gnosis auctions - this can also cause the user to bid on the wrong auction (it's more relevant for the EasyAuction contract which is OOS)

As a mitigation - make sure to deploy all contracts with create2 using a salt that's unique to the features of the contract, that will ensure that even in the case of a reorg it wouldn't be deployed to the same address as before.

distributeTokenToBuy() can be called while paused/frozen

Due to the removal of notPausedOrFrozen from Distributor.distribute() it's now possible to execute RevenueTrader.distributeTokenToBuy() while paused or frozen.

This is relevant when tokensToBuy is sent directly to the revenue trader: RSR sent to RSRTrader or RToken sent directly to RTokenTrader

redeemCustom allows the use of the zero basket

The basket with nonce #0 is an empty basket, redeemCustom allows to specify that basket for redemption, which will result in a loss for the redeemer.

refreshBasket can be called before the first prime basket was set

This will result in an event being emitted but will not impact the contract's state.

MIN_AUCTION_LENGTH seems too low

The current MIN_AUCTION_LENGTH is set to 2 blocks. This seems a bit too low since the price is time-dependant that means there would be only 3 price options for the auctions, and the final price wouldn't necessarily be the optimal price for the protocol.

If a token that yields RSR would be used as collateral then 100% of the yield would go to StRSR

This isn’t very likely to happen (currently the only token that yields RSR is StRSR of another RToken) but it’s worth keeping an eye on it.

Protocol might not be able to compromise basket when needed

Consider the following scenario:

• Protocol suffers from some loss and compromises the basket to a 1.1e9 ratio
• Months pass by and users mint new tokens and increase the TVL
• A small compromise is required (12%), this brings the ratio to below 1e9 and reverts the compromise
• Protocol is now disabled despite holding a significant amount of value, and users can only redeem for their prorata share of the assets,

This might be intended design, but worth taking this scenario into account

At toColl() and toAsset() use the mapping to check if asset exists

Savings: ~2.1K per call Notice this is a function that's being called frequently, and many times per tx. Overall this can save a few thousands of gas units per tx for the most common txs (e.g. issuance, redemption)

Code: https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L120-L132

At toColl() and toAsset() instead of using the EnumberableSet to check that the erc20 is registered just check that the value returned from the mapping isn't zero (this is supposed to be equivalent as long as the governance doesn't register the zero address as an asset contract - maybe add a check for that at register()).

Proposed changes:

    /// Return the Asset registered for erc20; revert if erc20 is not registered.
    // checks: erc20 in assets
    // returns: assets[erc20]
    function toAsset(IERC20 erc20) external view returns (IAsset) {
        IAsset asset = assets[erc20];
        require(asset != IAsset(address(0)), "erc20 unregistered");
        return asset;
    }

    /// Return the Collateral registered for erc20; revert if erc20 is not registered as Collateral
    // checks: erc20 in assets, assets[erc20].isCollateral()
    // returns: assets[erc20]
    function toColl(IERC20 erc20) external view returns (ICollateral) {
        IAsset coll = assets[erc20];
        require(coll != IAsset(address(0)), "erc20 unregistered");
        require(coll.isCollateral(), "erc20 is not collateral");
        return ICollateral(address(coll));
    }

Get targetIndex from mapping instead of iterating

Gas savings: a few thousands (see below)

The following code is used at the BasketLib to find the index of a value inside an EnumerableSet

            for (targetIndex = 0; targetIndex < targetsLength; ++targetIndex) {
                if (targetNames.at(targetIndex) == config.targetNames[config.erc20s[i]]) break;
            }

However the index can be fetched directly from the _indexed mapping:

diff --git a/contracts/p1/mixins/BasketLib.sol b/contracts/p1/mixins/BasketLib.sol
index bc52d1c6..ce56c715 100644
--- a/contracts/p1/mixins/BasketLib.sol
+++ b/contracts/p1/mixins/BasketLib.sol
@@ -192,10 +192,8 @@ library BasketLibP1 {
         // For each prime collateral token:
         for (uint256 i = 0; i < config.erc20s.length; ++i) {
             // Find collateral's targetName index
-            uint256 targetIndex;
-            for (targetIndex = 0; targetIndex < targetsLength; ++targetIndex) {
-                if (targetNames.at(targetIndex) == config.targetNames[config.erc20s[i]]) break;
-            }
+            uint256 targetIndex = targetNames._inner._indexes[config.targetNames[config.erc20s[i]]] -1 ;
+ 
             assert(targetIndex < targetsLength);
             // now, targetNames[targetIndex] == config.targetNames[erc20]

Gas savings:

• The _indexes keys are considered warm since all values were inserted in the current tx
• Total saving is the sum of the index of the target names per each erc20 minus 1
• on average (depends on the location of the target in the set for each erc20): (config.erc20s.length)*(targetsLength-1)/2*100
  • E.g. for target length of 5 and 10 ERC20 that would save on average 10*4/2*100=2K

Use furnace instead of main.furnace()

Gas savings: ~2.6K

Code: https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L184 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L257

At RToken.redeemTo() and redeemCustom() furnace is being called using main.furnace() instead of using the furnace variable.

The call to main.furnace() costs both the cold storage variable read at main.furnace() and an external call to a cold address while using the furnace variable of the current contract costs only the cold storage read. The additional cold call would cost ~2.6K

To my understanding both of the values should be equal at all times so there shouldn't be an issue with the replacement.

Deployer.implementations can be immutable

Gas saved: ~28K per RToken deployment

The struct itself can’t be immutable, but you can save the values of the fields (and fields of the components) as immutable variables, and use an internal function to build the struct out of those immutable variables This would save ~2.1K per field, with 13 fields that brings us to ~28K of units saved

Update lastWithdrawRefresh only if it has changed

Gas saved: ~100

At leakyRefresh() lastWithdrawRefresh gets updated even if didn't change, that costs an additional 100 gas units.

Proposed change:

-        leaked = lastWithdrawRefresh != lastRefresh ? withdrawal : leaked + withdrawal;
-        lastWithdrawRefresh = lastRefresh;
+        if(lastWithdrawRefresh != lastRefresh){
+                leaked =  withdrawal;
+                lastWithdrawRefresh = lastRefresh;
+        } else{
+                leaked = leaked + withdrawal;
+        }

Require array to be sorted and use sortedAndAllUnique at BackingManager.forwardRevenue()

Estimated savings: ~n^2*10 where n is the length of the asset. For example for 20 assets that would save ~4K

Caching storage variable and function calls

This is one of the most common ways to save a nice amount of gas. Every additional read costs 100 gas units (when it comes to mapping or arrays there's additional cost), and each additional function call costs at least 100 gas units (usually much more). I've noticed a few instances where a storage variable read or a view-function call can be cached to memory to save gas, I'm pretty sure there are many more instances that I didn't notice.

BasketLib.nextBasket() refactoring

Gas saved: a few thousands

The following refactoring saves a few thousands of gas mostly by preventing:

1. Double call to goodCollateral
2. The second iteration over the whole backup.erc20s array

diff --git a/contracts/p1/mixins/BasketLib.sol b/contracts/p1/mixins/BasketLib.sol
index bc52d1c6..7ab9c48b 100644
--- a/contracts/p1/mixins/BasketLib.sol
+++ b/contracts/p1/mixins/BasketLib.sol
@@ -192,10 +192,8 @@ library BasketLibP1 {
         // For each prime collateral token:
         for (uint256 i = 0; i < config.erc20s.length; ++i) {
             // Find collateral's targetName index
-            uint256 targetIndex;
-            for (targetIndex = 0; targetIndex < targetsLength; ++targetIndex) {
-                if (targetNames.at(targetIndex) == config.targetNames[config.erc20s[i]]) break;
-            }
+            uint256 targetIndex = targetNames._inner._indexes[config.targetNames[config.erc20s[i]]] -1 ;
+ 
             assert(targetIndex < targetsLength);
             // now, targetNames[targetIndex] == config.targetNames[erc20]
 
@@ -244,32 +242,32 @@ library BasketLibP1 {
             uint256 size = 0; // backup basket size
             BackupConfig storage backup = config.backups[targetNames.at(i)];
 
+            IERC20[] memory backupsToUse = new IERC20[](backup.erc20s.length);
+
             // Find the backup basket size: min(backup.max, # of good backup collateral)
             for (uint256 j = 0; j < backup.erc20s.length && size < backup.max; ++j) {
-                if (goodCollateral(targetNames.at(i), backup.erc20s[j], assetRegistry)) size++;
+                if (goodCollateral(targetNames.at(i), backup.erc20s[j], assetRegistry)) 
+                {
+                    backupsToUse[size] = backup.erc20s[j];
+                    size++;
+                }
             }
 
             // Now, size = len(backups(tgt)). If empty, fail.
             if (size == 0) return false;
 
-            // Set backup basket weights...
-            uint256 assigned = 0;
 
             // Loop: for erc20 in backups(tgt)...
-            for (uint256 j = 0; j < backup.erc20s.length && assigned < size; ++j) {
-                if (goodCollateral(targetNames.at(i), backup.erc20s[j], assetRegistry)) {
-                    // Across this .add(), targetWeight(newBasket',erc20)
-                    // = targetWeight(newBasket,erc20) + unsoundPrimeWt(tgt) / len(backups(tgt))
-                    newBasket.add(
-                        backup.erc20s[j],
-                        totalWeights[i].minus(goodWeights[i]).div(
-                            // this div is safe: targetPerRef > 0: goodCollateral check
-                            assetRegistry.toColl(backup.erc20s[j]).targetPerRef().mulu(size),
-                            CEIL
-                        )
-                    );
-                    assigned++;
-                }
+            for (uint256 j = 0; j < size; ++j) {
+
+                newBasket.add(
+                    backupsToUse[j],
+                    totalWeights[i].minus(goodWeights[i]).div(
+                        // this div is safe: targetPerRef > 0: goodCollateral check
+                        assetRegistry.toColl(backupsToUse[j]).targetPerRef().mulu(size),
+                        CEIL
+                    )
+                );
             }
             // Here, targetWeight(newBasket, e) = primeWt(e) + backupWt(e) for all e targeting tgt
         }

BasketLib.nextBasket() caching

On top of the above refactoring:

• config.erc20s[i] is being read a few times here
• config.erc20s.length and backup.erc20s.length can be cached
• targetNames.at(i) is being read twice in the second loop (3 before the proposed refactoring)

sellAmount at DutchTrade.settle()

sellAmount is read here twice if greater than sellBal

        soldAmt = sellAmount > sellBal ? sellAmount - sellBal : 0;

quoteCustomRedemption() loop

nonce

Savings: 100*basketNonces.length here nonce is being read each iteration. It can be cached outside of the loop

basketNonces.length

Savings: 100*basketNonces.length

b.erc20s.length

Savings: 100*(sum of b.erc20s.length for all baskets)

Use custom errors instead of string errors

This saves gas both for deployment and in case that the revert is triggered.