Reserve contest - HollaDieWaldfee's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L201

Vulnerability details

Impact

This report deals with how the recollateralization algorithm underestimates the number of baskets it can hold (by calculating unnecessary costs for collateral that does not need to be traded).

When this number of baskets it thinks it can hold is reached - which is less than what it can actually hold - it compromises its baskets (i.e. reduces the number of baskets needed for full collateralization).

This causes RToken holders to lose value as the excess assets are not used to only appreciate RToken value but is also processed in part as rsr revenue.

Proof of Concept

In order to understand this issue it is necessary to understand the recollateralization algorithm.

Here is how recollateralization works and how the algorithm under-estimates the baskets it can acquire:

1. RecollateralizationLib.prepareRecollateralizationTrade is called to determine the next trade (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L62-L113)

2. In this function, the basket range is calculated with the basketRange function (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L88)

3. The recollateralization algorithm tries to recollateralize the RToken up to the bottom of the basket range (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L380). Therefore it is important that this bottom of the range is correct. As we will see it is underestimated.

4. The bottom of the range is calculated as assetsLow.div(basketPriceHigh) (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L201)

5. assetsLow is calculated by the totalAssetValue function which calculates the sum of all collateral valued with their low prices (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/mixins/RecollateralizationLib.sol#L264).

6. basketPriceHigh is calculated by the BasketHandler._price function which values all collaterals in a basket with their high prices (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BasketHandler.sol#L337-L347)

7. The issue with this becomes clear when thinking about an example. Assume the BackingManager holds 10 WBTC and it holds so many baskets that 7 WBTC are used up. These 7 WBTC can be considered to be "good" and need not be traded for other collateral.
   The question should now be how to divide the ADDITIONAL 3 WBTC across collateral to increase the number of baskets held. However the recollateralization algorithm calculates as though it would need to sell those 7 WBTC at the low price and buy them again at the high price. E.g. sell them at $19000 and buy again at $20000.
   Thereby the recollateralization algorithm ends up with a bottom basket range that underestimates how many baskets can actually be aquired.

Tools Used

VSCode

Recommended Mitigation Steps

It is clear that the recollateralization algorithm must be changed to not devalue collateral that will not be traded.

There are two possibilities for this:

1. assetsLow can be calculated with two different prices for each asset. The part that will not be traded anymore is valued with the high price, the part that might be traded is valued with the low price. Thereby when dividing assetsLow by basketPriceHigh, the collateral that is held and will not be traded is not devalued.

2. Perform the recollateralization calculations on the part of the assets that might be traded only. Thereby leaving out the "good" collateral entirely. This would require more changes to the code but could be a cleaner solution.

I encourage the sponsor to investigate the above two potential solutions. The recollateralization algorithm is the most math-heavy part of the protocol. So any changes must be carefully considered and tested.

Lines of code

https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BackingManager.sol#L173-L179

Vulnerability details

Impact

The BackingManager.handoutExcessAssets function sends all rsr that the BackingManager holds to the rsrTrader (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BackingManager.sol#L173-L179).

The purpose of this is that rsr which can be held by the BackingManager due to seizure from the StRSR contract is sent back entirely to the StRSR contract and not - as would happen later in the function (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BackingManager.sol#L221-L242) - shared across rsrTrader and rTokenTrader.

The rsrTrader then sends the rsr to the Distributor (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/RevenueTrader.sol#L59-L65).

So far so good. However the Distributor does not necessarily send all of the rsr to the StRSR contract. Instead it distributes the rsr according to its distribution table. I.e. there can be multiple destinations each receiving a share of the rsr (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/Distributor.sol#L108-L136).

In economic terms, rsr that is thereby not sent to StRSR but to other destinations, is a transfer of funds from stakers to these destinations, i.e. a loss to stakers.

Stakers should only pay for recollateralization of the RToken, not however send revenue to rsr revenue destinations.

Proof of Concept

Assume the following situation:

• A seizure of rsr from the StRSR contract occurred because the RToken was under-collateralized.

• A trade occurred which restored collateralization. However not all rsr was sold by the trade and was returned to the BackingManager.

Now BackingManager.manageTokens is called which due to the full collateralization calls BackingManager.handoutExcessAssets (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BackingManager.sol#L118).

This sends rsr to the rsrTrader (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/BackingManager.sol#L173-L179).

Then the rsr is sent to the Distributor (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/RevenueTrader.sol#L59-L65).

There it is distributed across all rsr destinations (https://github.com/reserve-protocol/protocol/blob/b30ab2068dddf111744b8feed0dd94925e10d947/contracts/p1/Distributor.sol#L108-L136).

Tools Used

VSCode

Recommended Mitigation Steps

rsr should be sent from the BackingManager directly to StRSR without the need to go through rsrTrader and Distributor. Thereby it won't be sent to other rsr revenue destinations.

Fix:

diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol
index 431e0796..eb506004 100644
--- a/contracts/p1/BackingManager.sol
+++ b/contracts/p1/BackingManager.sol
@@ -173,7 +173,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
         if (rsr.balanceOf(address(this)) > 0) {
             // For CEI, this is an interaction "within our system" even though RSR is already live
             IERC20Upgradeable(address(rsr)).safeTransfer(
-                address(rsrTrader),
+                address(stRSR),
                 rsr.balanceOf(address(this))
             );
         }

There is a caveat to this however:

It is possible for rsr to be a reward token for a collateral of the RToken.
Neither the current implementation nor the proposed fix addresses this and instead sends the rewards to StRSR.

In principal, rsr that was rewarded should have a share that goes to the rTokenTrader as well as include all rsr revenue destinations.
However there is no easy way to differentiate where the rsr came from.

Therefore I think it is reasonable to send all rsr to StRSR and make it clear to developers and users that rsr rewards cannot be paid out to rToken holders.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186-L340 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L344-L370 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L406-L418 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L72

Vulnerability details

Impact

rToken can be issued via the RToken.issue function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186-L340).

There is a limit to how many rToken can be minted within one block (a percentage of the rToken supply).

If RToken.issue is called but the rToken cannot be minted immediately, the issuance is added to a queue.

The time when the issuance can be vested (i.e. when the rToken can be actually minted) is determined by the RToken.whenFinished function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L344-L370) which is called internally by RToken.issue (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L243).

If there is an issuance in the queue it can be cancelled by the recipient of the issuance by calling RToken.cancel (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L406-L418).

All the tokens from the basket are then refunded.

So when a user calls RToken.issue and the issuance is queued and immediately after that he calls RToken.cancel, there is no cost to the user except the Gas.

The issue is that by performing this issue -> cancel roundtrip, the allVestAt variable (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L72) is increased (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L369). This means that other users that now call RToken.issue need now to wait longer to vest their rToken.

So an attacker can repeatedly call issue and cancel to essentially increase the allVestAt to a very large block number (maybe years in the future).

The amount of blocks by which allVestAt is increased depends on the capital the attacker uses to perform one issue -> cancel roundtrip (this capital is not lost, just used for some amount of time) and the number of roundtrips (which costs a little bit of Gas).

Assume the attacker uses enough capital to increase allVestAt by 5 blocks per roundtrip (i.e. approximately 60 seconds) and one roundtrip costs $1 in Gas.

It then costs $525,600 to increase allVestAt by 1 year:

1 year is 31,536,000 seconds

31,536,000 seconds / 12 seconds per block = 2,628,000 blocks

2,628,000 blocks / 5 blocks per roundtrip = 525,600 roundtrips

525,600 roundtrips * $1 per roundtrip = $525,600

This is definitely within reach of a determined attacker. Also Gas costs might decrease in the future which makes this attack even more dangerous.

The vision of Reserve is to provide a protocol to create currencies that are used worldwide with a TVL of possibly trillions of dollars.

The attack described above requires little capital in comparison to the TVL and can be very effective in denying the protocol to operate.

Thereby this attack, although it does not pose a risk to the users' assets, makes it impossible for the protocol to satisfy its ambitions. I therefore rate this issue to be of "High" severity.

Proof of Concept

1. Assume allVestAt is a block number in the future. This means that by calling RToken.issue, the rToken are not issued immediately but added to the issuance queue instead. So the issuance can be canceled.
   This is not much of a constraint however. In the case that allVestAt is not a block number in the future, the attacker must issue some amount of rToken immediately. This amount can the be redeemed again. So there is just an added need for capital to perform the attack.
2. The attacker calls RToken.issue to increase allVestAt and immediately after that he calls RToken.cancel. All collateral tokens are returned to him again.
3. The attacker repeats step 2 until allVestAt is as large as he wants.
4. rToken that are issued from now on can only be vested when block.timestamp has reached allVestAt.

Tools Used

VSCode

Recommended Mitigation Steps

There is no easy fix for this. Any change to the issuance mechanism changes the economic incentives of the protocol.

A possible solution can be to introduce a cancellation fee such that the attack becomes too expensive or to "recharge" the issuance capacity when an issuance is cancelled.

However the sponsor should explore possible mechanisms and make sure they don't introduce any bad economic incentives.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L275

Vulnerability details

Impact

The RecollateralizationLib.basketRange function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L152-L202) internally calls the RecollateralizationLib.totalAssetValue function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L226-L281).

I will show in this report that the RecollateralizationLib.totalAssetValue function returns a value for assetsLow that is too low.

This in turn causes the range.bottom value (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L201) that the RecollateralizationLib.basketRange function returns to be too low.

Before showing why the assetsLow value is underestimated however I will explain the impact of the range.bottom variable being too low.

There are two places where this value is used:

1. RecollateralizationLib.prepareRecollateralizationTrade function

This function passes the range to the RecollateralizationLib.nextTradePair function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L88-L91)

Since range.bottom is too low, the needed amount is too low (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L380).

This causes the if statement to not be executed in some cases when it otherwise would be executed (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L381-L396).

And the amtShort is smaller than it should be (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L391).

In the end this causes recollateralization trades to not buy as much assets as they could buy. This is because the amount of assets is underestimated so the protocol can actually hold more baskets than it thinks it can.

Therefore underestimating assetsLow causes a direct loss to RToken holders because the protocol will not recollateralize the RToken to the level that it can and should.

2. Price calculations of RTokenAsset

A RTokenAsset uses the RecollateralizationLib.basketRange function to calculate its value:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/RTokenAsset.sol#L156

The RTokenAsset therefore underestimates its low and lotLow prices:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/RTokenAsset.sol#L58

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/RTokenAsset.sol#L99

This then can lead to issues in any places where the prices of RTokenAssets are used.

Proof of Concept

Here is the affected line:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L275

potentialDustLoss = potentialDustLoss.plus(rules.minTradeVolume);

This line is executed for every asset in the AssetRegistry (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L242).

So for every asset in the AssetRegistry a potential dust loss of minTradeVolume is added.

The following scenario shows why this is wrong:

assume minTradeVolume = $50

assume further the following:
asset1 with low value $1
asset2 with low value $1
asset3 with low value $1
asset4 with low value $200

Currently potentialDustLoss will be 4*minTradeVolume = $200.
So assetsLow = $203 - $200 = $3.

Dust loss should not be calculated with $50 for the first 3 assets.
Dust loss for an asset should be capped at its low value.
So dust loss alltogether should be $1 + $1 + $1 + $50 = $53.

So assetsLow should be $1+$1+$1+$200 - $53 = $150.

Tools Used

VSCode

Recommended Mitigation Steps

I suggest that an asset can only incur as much dust loss as its balance is.
If the protocol only holds $5 of asset A then this should not cause a dust loss of say $10.

The fix first saves the assetLow value which should be saved to memory because it is now needed two times then it caps the dust loss of an asset at its low value:

diff --git a/contracts/p1/mixins/RecollateralizationLib.sol b/contracts/p1/mixins/RecollateralizationLib.sol
index 648d1813..b5b86cac 100644
--- a/contracts/p1/mixins/RecollateralizationLib.sol
+++ b/contracts/p1/mixins/RecollateralizationLib.sol
@@ -261,7 +261,8 @@ library RecollateralizationLibP1 {
 
             // Intentionally include value of IFFY/DISABLED collateral when low is nonzero
             // {UoA} = {UoA} + {UoA/tok} * {tok}
-            assetsLow += low.mul(bal, FLOOR);
+            uint192 assetLow = low.mul(bal,FLOOR);
+            assetsLow += assetLow;
             // += is same as Fix.plus
 
             // assetsHigh += high.mul(bal, CEIL), where assetsHigh is [0, FIX_MAX]
@@ -272,7 +273,7 @@ library RecollateralizationLibP1 {
             // += is same as Fix.plus
 
             // Accumulate potential losses to dust
-            potentialDustLoss = potentialDustLoss.plus(rules.minTradeVolume);
+            potentialDustLoss = potentialDustLoss.plus(fixMin(rules.minTradeVolume, assetLow));
         }
 
         // Account for all the places dust could get stuck

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L374-L422 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L596-L598 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L496-L530

Vulnerability details

Impact

If a RToken is under-collateralized, the BackingManager can call the StRSR.seizeRSR function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L141).

This sends some amount of rsr held by the StRSR contract to the BackingManager which can then be traded for other tokens in order to recollateralize the RToken.

There are 3 pools of rsr in the StRSR contract that StRSR.seizeRSR claims rsr from.

1. stakeRSR (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L386-L398)
2. draftRSR (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L401-L414)
3. rewards (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L417)

The rsr taken from the rewards is what is interesting in this report.

The issue is that the StRSR._payoutRewards function (which is used to pay rsr rewards to stakers over time) keeps track of the available rewards to distribute in the rsrRewardsAtLastPayout variable (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L517).

When the StRSR.seizeRSR function is called (taking away rewards and sending them to the BackingManager) and after that StRSR._payoutRewards is called, StRSR._payoutRewards uses the rsrRewardsAtLastPayout variable that was set before the seizure (the actual amount of rewards is smaller after the seizure).

Thereby the amount by which StRSR.stakeRSR is increased (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L513) when rewards are paid out can be greater than the actual rewards that are available.

Proof of Concept and further assessment of Impact

The fact that the rsrRewardsAtLastPayout variable is too big after a call to StRSR.seizeRSR has two consequences when StRSR._payoutRewards is called:

1. stakeRSR is increased by an amount that is larger than it should be (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L513)
2. stakeRate (which uses division by stakeRSR when calculated) is smaller than it should be (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L524-L526)

Both affected variables can in principle be off by a large amount. In practice this is not likely because the rewards paid out will be small in comparison to stakeRSR.

Also after a second call to StRSR._payoutRewards all variables are in sync again and the problem has solved itself. The excess payouts are then accounted for by the StRSR.rsrRewards function.

So there is a small amount of time for any real issue to occur and there does not always occur an issue when StRSR.seizeRSR is called.

That being said, the behavior described so far can cause a temporary DOS:

In StRSR._payoutRewards, stakeRSR is increased (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L513), then StRSR.rsrRewards is called which calculates rsr.balanceOf(address(this)) - stakeRSR - draftRSR (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L596-L598).

The falsely paid out amount of rewards can increase StRSR.stakeRSR so much that this line reverts due to underflow.

This can cause DOS when StRSR.seizeRSR is called again because it internally calls StRSR.rsrRewards.

This will solve itself when more rsr accumulates in the contract due to revenue which makes the balance increase or someone can just send rsr and thereby increase the balance.

The DOS occurs also in all functions that internally call StRSR._payoutRewards (StRSR.stake and StRSR.unstake):

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L215

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L262

Overall the impact of this on the average RToken is quite limited but as explained above it can definitely cause issues.

Tools Used

VSCode

Recommended Mitigation Steps

When StRSR.seizeRSR is called, the rsrRewardsAtLastPayout variable should be set to the rewards that are available after the seizure:

diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol
index 8fe1c3e7..4f9ea736 100644
--- a/contracts/p1/StRSR.sol
+++ b/contracts/p1/StRSR.sol
@@ -419,6 +419,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab
         // Transfer RSR to caller
         emit ExchangeRateSet(initRate, exchangeRate());
         IERC20Upgradeable(address(rsr)).safeTransfer(_msgSender(), seizedRSR);
+        rsrRewardsAtLastPayout = rsrRewards();
     }

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L514 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L448 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BasketHandler.sol#L183-L192

Vulnerability details

Impact

The Reserve protocol allows redemption of rToken even when the protocol is paused.

The docs/system-design.md documentation describes the paused state as:

all interactions disabled EXCEPT RToken.redeem + RToken.cancel + ERC20 functions + StRSR.stake

Redemption of rToken should only ever be prohibited when the protocol is in the frozen state.

You can see that the RToken.redeem function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L514) has the notFrozen modifier so it can be called when the protocol is in the paused state.

The issue is that this function relies on the BasketHandler.status() to not be DISABLED (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L448).

The BasketHandler.refreshBasket function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BasketHandler.sol#L183-L192) however, which must be called to get the basket out of the DISABLED state, cannot be called by any user when the protocol is paused.

When the protocol is paused it can only be called by the governance (OWNER) address.

So in case the basket is DISABLED and the protocol is paused, it is the governance that must call refreshBasket to allow redemption of rToken.

This is dangerous because redemption of rToken should not rely on governance to perform any actions such that users can get out of the protocol when there is something wrong with the governance technically or if the governance behaves badly.

Proof of Concept

The RToken.redeem function has the notFrozen modifier so it can be called when the protocol is paused (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439).

The BasketHandler.refreshBasket function can only be called by the governance when the protocol is paused:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BasketHandler.sol#L186-L190

require(
    main.hasRole(OWNER, _msgSender()) ||
        (status() == CollateralStatus.DISABLED && !main.pausedOrFrozen()),
    "basket unrefreshable"
);

Therefore the situation exists where rToken redemption should be possible but it is blocked by the BasketHandler.refreshBasket function.

Tools Used

VSCode

Recommended Mitigation Steps

The BasketHandler.refreshBasket function should be callable by anyone when the status() is DISABLED and the protocol is paused.

So the above require statement can be changed like this:

diff --git a/contracts/p1/BasketHandler.sol b/contracts/p1/BasketHandler.sol
index f74155b1..963e29de 100644
--- a/contracts/p1/BasketHandler.sol
+++ b/contracts/p1/BasketHandler.sol
@@ -185,7 +185,7 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler {
 
         require(
             main.hasRole(OWNER, _msgSender()) ||
-                (status() == CollateralStatus.DISABLED && !main.pausedOrFrozen()),
+                (status() == CollateralStatus.DISABLED && !main.frozen()),
             "basket unrefreshable"
         );
         _switchBasket();

It was discussed with the sponsor that they might even allow rToken redemption when the basket is DISABLED.

In other words only disallow it when the protocol is frozen.

This however needs further consideration by the sponsor as it might negatively affect other aspects of the protocol that are beyond the scope of this report.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L514 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L72-L77

Vulnerability details

Impact

The Reserve protocol allows redemption of rToken even when the protocol is paused.

The docs/system-design.md documentation describes the paused state as:

all interactions disabled EXCEPT RToken.redeem + RToken.cancel + ERC20 functions + StRSR.stake

Redemption of rToken should only ever be prohibited when the protocol is in the frozen state.

The issue is that the RToken.redeem function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L514) relies on the BackingManager.grantRTokenAllowance function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L72-L77) to be called before redemption.

Also the only function that relies on BackingManager.grantRTokenAllowance to be called before is RToken.redeem.

Therefore BackingManager.grantRTokenAllowance can be called at any time before a specific ERC20 needs first be transferred from the BackingManager for the purpose of redemption of rToken.

The issue is that the BackingManager.grantRTokenAllowance function has the notPausedOrFrozen modifier. This means it cannot (in contrast to RToken.redeem) be called when the protocol is paused.

Therefore if rToken is for the first time redeemed for a specific ERC20 in a paused protocol state, BackingManager.grantRTokenAllowance might not have been called before.

This effectively disables redemption of rToken as long as the protocol is paused and is clearly against the usability / economic considerations to allow redemption in the paused state.

Proof of Concept

For simplicity assume there is an rToken backed by a single ERC20 called AToken

1. rToken is issued and AToken is transferred to the BackingManager.
2. The protocol goes into the paused state before any redemptions have occurred. So the BackingManager.grantRTokenAllowance function might not have been called at this point.
3. Now the protocol is paused which should allow redemption of rToken but it is not possible because the AToken allowance cannot be granted since the BackingManager.grantRTokenAllowance function cannot be called in the paused state.

Another scenario is when the basket of a RToken is changed to include an ERC20 that was not included in the basket before. If the protocol now goes into the paused state without BackingManager.grantRTokenAllowance being called before, redemption is not possible.

Tools Used

VSCode

Recommended Mitigation Steps

The BackingManager.grantRTokenAllowance function should use the notFrozen modifier instead of the notPausedOrFrozen modifier such that allowance can be granted in the paused state:

diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol
index 431e0796..7dfa29e9 100644
--- a/contracts/p1/BackingManager.sol
+++ b/contracts/p1/BackingManager.sol
@@ -69,7 +69,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
     // checks: erc20 in assetRegistry
     // action: set allowance on erc20 for rToken to UINT_MAX
     // Using two safeApprove calls instead of safeIncreaseAllowance to support USDT
-    function grantRTokenAllowance(IERC20 erc20) external notPausedOrFrozen {
+    function grantRTokenAllowance(IERC20 erc20) external notFrozen {
         require(assetRegistry.isRegistered(erc20), "erc20 unregistered");
         // == Interaction ==
         IERC20Upgradeable(address(erc20)).safeApprove(address(main.rToken()), 0);

Reserve Low Risk and Non-Critical Issues

Summary

[L-01] Check that longFreeze >= shortFreeze

The docs/system-design.md documentation states that:

The LONG_FREEZER role should be assigned to an address that will highly optimize for no false positives. It is much longer than the short freeze. It exists so that in the case of a zero-day exploit, governance can act before the system unfreezes and resumes functioning.

This makes it clear that the longFreeze duration must not be shorter than the shortFreeze duration. Maybe the longFreeze should even be strictly longer than the shortFreeze.

If longFreeze is shorter than shortFreeze it fails to fulfill its purpose of extending the shortFreeze.

The issue is that the Auth.setShortFreeze and Auth.setLongFreeze functions do not check that shortFreeze <= longFreeze.

Therefore I recommend adding the following line to Auth.setShortFreeze (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/mixins/Auth.sol#L180-L184):

require(shortFreeze_ <= longFreeze, "long freeze cannot be shorter than short freeze")

And add the following line to Àuth.setLongFreeze (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/mixins/Auth.sol#L187-L191):

require(shortFreeze <= longFreeze_, "long freeze cannot be shorter than short freeze")

In order for the change to work you need to change the order in which you set the short freeze and long freeze values in the Auth.__Auth_init function.

You need to set the long freeze first, then the short freeze:

setLongFreeze(longFreeze_);
setShortFreeze(shortFreeze_);

[L-02] Check that RToken contract does not receive rToken distribution

The Distributor is used to distribute rsr and rToken to destination addresses.

When calling the Distributor.setDistribution function (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Distributor.sol#L61-L65) it should be checked that the destination is not the RToken contract.

This is because the RToken contract will revert if it receives rToken (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L833). So the RToken contract can be mistakenly registered thereby causing DOS to the Distributor.

Also the RToken contract is not able to handle any rsr it receives so you can just forbid registering the RToken contract as destination.

[L-03] rToken issuances can be spammed

The RToken.issue function can be called for any recipient (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186).

So any user can issue rToken for any other user.

This can lead to scenarios where malicious users intentionally issue a very small amount of rToken to other users in order to spam them.

E.g. if there is an application with a GUI, other users' GUIs can be spammed with multiple issuances that they did not initiate.

This can negatively impact user experience.

It was assessed with the sponsor that they need to have this design and the benefits outweigh the risks.

However this should be kept in mind when building applications around the issuance functionality because they need to deal with this potential spamming.

[L-04] Asset architecture makes dangerous use of delegatecall

Assets are registered in the core protocol via the AssetRegistry contract. The core protocol components can then claim rewards for an asset they hold by calling Asset.claimRewards via delegatecall.

For example the claimRewards function in the CTokenFiatCollateral Asset looks like this (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/CTokenFiatCollateral.sol#L52-L57):

function claimRewards() external virtual override {
    IERC20 comp = IERC20(comptroller.getCompAddress());
    uint256 oldBal = comp.balanceOf(address(this));
    comptroller.claimComp(address(this));
    emit RewardsClaimed(comp, comp.balanceOf(address(this)) - oldBal);
}

The claimRewards function is dependent on the comptroller state variable of the CTokenFiatCollateral contract.

The only reason this state variable can be accessed even though the claimRewards function is called via delegateCall is that comptroller is an immutable variable.

So the current plugin implementations are dependent on their claimRewards functions only accessing immutable variables.

If they try to access regular mutable variables, they will operate on the storage of the caller which can lead to various flaws.

There is no documentation about this requirement for immutable variables to be used and the sponsor admitted that this behavior about immutable variables has not been known before and the plugin architecture worked essentially "by accident".

It was discussed with the sponsor that either the architecture of plugins need to be changed or at least the documentation should be improved to make this very clear to plugin developers.

[L-05] Introduce max value for delayUntilDefault

Currently the delayUntilDefault variable in the FiatCollateral contract is only checked to be greater zero:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/FiatCollateral.sol#L75

This is unsafe because if it is a very large value it can be > type(uint48).max after block.timestamp is added to it.

This means _whenDefault is always set to NEVER:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/FiatCollateral.sol#L189

This in turn means that the collateral cannot default.

I suggest adding a reasonable upper limit to the delayUntilDefault variable. Maybe 1 year or 1 month.

[L-06] Only disable basket if new asset is not the same as old asset

In the AssetRegistry.swapRegistered function, the basket is disabled if the quantity of the asset is greater 0:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/AssetRegistry.sol#L78

It is not necessary to disable the basket if the new asset is the same as the old basket.
Not disabling the basket in this case improves the availability of nearly all operations in the protocol since most of them are dependent on a valid basket.

I suggest changing the function to this:

    function swapRegistered(IAsset asset) external governance returns (bool swapped) {
        require(_erc20s.contains(address(asset.erc20())), "no ERC20 collision");

++++    if (assets[asset.erc20()] != asset) {
            uint192 quantity = basketHandler.quantity(asset.erc20());
            if (quantity > 0) basketHandler.disableBasket();
++++    }


        swapped = _registerIgnoringCollisions(asset);
    }

[L-07] Use lotLow price instead of low price to calculate total asset value

Currently the RecollateralizationLib.totalAssetValue function makes use of the low price to calculate the total asset value under management by the BackingManager:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L264

You should consider using the lotLow price instead. This is since the purpose of the lotLow price is to provide a price that decays to zero if there is something wrong with the price feed. The low price however will just be 0 when there is something wrong with the price feed. The lotLow price therefore more accurately reflects the true value of the asset.

[L-08] Round with mode FLOOR instead of CEIL in calculation of range.bottom

In the RecollateralizationLib.basketRange function, range.bottom is calculated like this:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L201

range.bottom = fixMin(assetsLow.div(basketPriceHigh, CEIL), range.top);

The calculation is using rounding mode CEIL. This is wrong because range.bottom represents the low value of the range. Therefore the calculation should round down, i.e. use rounding mode FLOOR.

[L-09] Sending sell or buy tokens to GnosisTrade contract such that reportViolation is not called

The GnosisTrade contract calls Broker.reportViolation when the clearingPrice is less than the worstCasePrice:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/trading/GnosisTrade.sol#L206-L208

This disables the Broker such that no new trades can be opened until the issue is resolved by governance.

In a case where the GnosisTrade contract would call Broker.reportViolation this can be avoided by sending sell tokens or buy tokens (possible a very small amount is sufficient) to the GnosisTrade contract.

This behavior can prevent governance and the protocol from noticing an issue as it emerges (maybe a small amount of funds can manipulate the price sufficiently to keep the protocol going). The governance then only notices the problem as it gets worse.

The sold amount and bought amount should be queried from the EasyAuction contract directly instead of relying on the GnosisTrade contract balance.

[L-10] minBuyAmountPerOrder calculation should be based on asset value not token fraction

The minBuyAmountPerOrder in the GnosisTrade.init function is calculated like this:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/trading/GnosisTrade.sol#L123-L126

uint256 minBuyAmtPerOrder = Math.max(
    minBuyAmount / MAX_ORDERS,
    DEFAULT_MIN_BID.shiftl_toUint(int8(buy.decimals()))
);

So it is at least as big as DEFAULT_MIN_BID.shiftl_toUint(int8(buy.decimals())), and DEFAULT_MIN_BID is one hundredth of a token (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/trading/GnosisTrade.sol#L34).

This amount can cause problems for some tokens like WBTC. Currently one hundredth of one WBTC is worth >$200.

If the assets sold are worth only $100 then nobody is going to buy for $200.

I suggest the minBuyAmountPerOrder to be calculated based on the value of the asset that is sold in case the asset can be priced. Only in the case that the asset is unpriced must there be a fallback mechanism.

[N-01] No need to access components via Main

All components of the protocol are registered in the Main contract. It is not always necessary to access components by calling the Main contract first.

Specifically the p1 implementation follows the style to only access components via Main when the component is not accessible from the local contract.

Here are the instances that should be changed:

Use components.stRSR instead of main.stRSR():
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Deployer.sol#L208

Use furnace instead of main.furnace():
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L452

Use assetRegistry instead of main.assetRegistry():
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L380

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L443

[N-02] Use Solidity units when possible

You should consider using Solidity units (https://docs.soliditylang.org/en/v0.8.17/units-and-global-variables.html) whenever possible to improve readability of the code.

Use 1 weeks instead of 604800:
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Broker.sol#L24

[N-03] Redundant code

Redundant collateral status check

In the RToken.issue function, the collateral status is checked two times:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L217

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L255

The second check is redundant because it will always be true as made sure by the first check.

[N-04] Consider adding functionality to restake drafted withdrawal

Unstaking rsr from the StRSR contract is performed by first calling unstake and then after an unstakingDelay calling withdraw.

This means that when a user calls unstake, he MUST wait the unstakingDelay and then he MUST call withdraw.

A user might decide to restake his rsr at a time when unstakingDelay has not passed yet and thereby continue earning rewards.

It was discussed with the sponsor that they will consider adding such a restake functionality in order to provide more flexibility.

The incentives involved with the staking/unstaking mechanism likely don't change much.

It might make staking even somewhat more attractive because of the added flexibility.

[N-05] Consider implementing asymmetric peg range

Currently the FiatCollateral contract implements a symmetric range in which targetPerRef values are allowed for the collateral to be SOUND. This means the same delta is used to calculate the pegBottom and pegTop sides:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/FiatCollateral.sol#L85-L87

It can be beneficial for some FiatCollateral assets to allow a higher deviation on one side than on the other side.

E.g. the USD/USDT peg might be considered valid in a range from 0.95 to 1.02 with the asymmetric range instead of 0.95 - 1.05 with the symmetric range.

[N-06] Consider leaving the basket DISABLED when it has IFFY collateral and allowing redemption when basket is DISABLED

It was discussed with the Sponsor whether it is correct how the BasketHandler handles IFFY collateral.

I brought up the following concern:

In the BasketHandler._switchBasket function, a collateral with the status IFFY is considered a good collateral and is added to the reference basket. Some operations however like StRSR.withdraw require that there is no IFFY collateral in the basket.

In the discussion with the sponsor it was determined that:

So it might be that leaving the basket DISABLED and enabling redemption on DISABLED baskets makes more sense

There is no vulnerability with the current behavior but it might be beneficial to change the behavior of the protocol as stated above.

The sponsor should therefore further assess the proposed change and implement it if it is beneficial.

[N-07] Use IStRSRVotes instead of IStRSR interface

The Deployer is used to deploy an instance of a RToken.

This is achieved by cloning an implementation contract of each contract that is needed.

One of the contracts is called stRSR and it must implement the IStRSR interface:

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Deployer.sol#L121

This is too permissive though because there are two contracts that implement the IStRSR interface: StRSR and StRSRVotes.

The RToken protocol however requires that the contract is StRSRVotes not StRSR (voting capabilities are needed for governance).

The Deployer contract therefore should use IStRSRVotes instead of IStRSR such that the implementation contract must implement IStRSRVotes.