rsr can be unregistered from AssetRegistry to prevent seizure of stRSR holdings

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L108-L121

Vulnerability details

Impact

rsr held by stRSR contract can no longer be seized to cover rToken backing shortfall

Proof of Concept

Although stRSR governance is outlined as trusted in the readme I intend to show why this situation invalidate the underlying assumptions for this trust

First an explanation of the issue. A basic feature of the system is that when there is a shortfall in the backing for an rToken, rsr held by the rToken specific stRSR holders is seized to cover it. This process is carried out in RecollateralizationLibP1#prepareRecollateralizationTrade via BackingManager#rebalance.

BackingManager.sol#L148-L171

    (TradingContext memory ctx, Registry memory reg) = tradingContext(basketsHeld);
    (
        bool doTrade,
        TradeRequest memory req,
        TradePrices memory prices
    ) = RecollateralizationLibP1.prepareRecollateralizationTrade(ctx, reg);

BackingManager.sol#L273-L300

function tradingContext(BasketRange memory basketsHeld)
    public
    view
    returns (TradingContext memory ctx, Registry memory reg)
{
    reg = assetRegistry.getRegistry();

    ...
}

Above we see that reg (which contains the list of ERC20 tokens) is pulled directly from the assetRegistry and passed to the RecollateralizationLib.

RecollateralizationLib.sol#L274-L376

function nextTradePair(
    TradingContext memory ctx,
    Registry memory reg,
    BasketRange memory range
) private view returns (TradeInfo memory trade) {
    // assert(tradesOpen == 0); // guaranteed by BackingManager.rebalance()

    MaxSurplusDeficit memory maxes;
    maxes.surplusStatus = CollateralStatus.IFFY; // least-desirable sell status

    uint256 rsrIndex = reg.erc20s.length; // invalid index, to-start

    for (uint256 i = 0; i < reg.erc20s.length; ++i) {
        ...
    }

    if (address(trade.sell) == address(0) && address(trade.buy) != address(0)) {
        (uint192 low, uint192 high) = reg.assets[rsrIndex].price(); <- Calls will revert if rsr isn't present in the ERC20 array from asset registry

        if (
            high != 0 &&
            TradeLib.isEnoughToSell(
                reg.assets[rsrIndex],
                ctx.bals[rsrIndex],
                low,
                ctx.minTradeVolume
            )
        ) {
            trade.sell = reg.assets[rsrIndex];
            trade.sellAmount = ctx.bals[rsrIndex];
            trade.prices.sellLow = low;
            trade.prices.sellHigh = high;
        }
    }
}

The RecollateralizationLib assumes that rsr will be present in the ERC20 list and if it isn't then all calls to rebalance that attempt to use rsr as the sell asset (through seizing it) will revert.

AssetRegistry.sol#L108-L121

function unregister(IAsset asset) external governance {
    require(_erc20s.contains(address(asset.erc20())), "no asset to unregister");
    require(assets[asset.erc20()] == asset, "asset not found");

    try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
        if (quantity != 0) basketHandler.disableBasket(); // not an interaction
    } catch {
        basketHandler.disableBasket();
    }

    _erc20s.remove(address(asset.erc20()));
    assets[asset.erc20()] = IAsset(address(0));
    emit AssetUnregistered(asset.erc20(), asset);
}

Above is the function which allows governance to remove assets from the asset list. During the creation of a rToken, rsr is added to the asset list. However with the above function, nothing prevents rsr from being removed from the ERC20 list. In the event that it is removed as shown above, it now becomes impossible to seize the rsr from stRSR holders. With the how covered, we move onto the why.

The Exploit

Assume that the backing of an rToken is compromised and rsr is will now be seized from the stRSR holders and sold for backing. stRSR holders can create and pass a proposal to remove rsr from the assetRegistry to prevent the seizure.

stRSR should not be trusted with this power for the following reasons which I will summary below:

1. rsr is essential to the rToken and should never be removed
2. stRSR holders are highly incentivized to act in this way
3. Time favors the stRSR holders
4. Other rsr holders are disincentivized to involve themselves and remedy the situation
5. A situation like this causes immense reputation damage to the protocol

The first point is that removing rsr from the asset registry should never be allowed to begin with. The over-collateralization of rTokens is a core feature of the Reserve protocol. As shown above, key features completely break when it is removed. There is no valid use case for removing it.

Second, stRSR holders have a very strong incentive to act in this way. A key reason why governance is typically trusted is that any malicious behavior would lead to losses (depreciation, loss of revenue, etc.) greater than any gain that could be made. Here it is not the case. The voters lose everything if they allow all their rsr to be seized. They have no consideration as to how this impacts the price of rsr or future revenue generated by the rToken since the alternative outcome leaves them with no interest in the protocol. Additionally, by freezing the seizure the situation can potentially be resolved with no losses to them, which leads into my next point.

Time favors stRSR holders. The underlying tokens of an rToken are typically yield bearing which means the undercollateralization literally fixes itself over time. Although stRSR holders cannot withdraw, rToken holders can via custom redemption. For each rToken holder who gives up during the standoff and custom redeems, the deficit is reduced which favors the stRSR holders.

Other rsr holders are disincentivized to involve themselves and remedy the situation. If other rsr holders stake their rsr to gain voting power and reenable rsr as an asset they are directly losing their own rsr stake since their rsr will also be seized and sold afterwards. This highlights another reason why governance cannot be trusted in this scenario. The model assumes that other rsr holders can step in to defeat malicious proposals however doing so would result in catastrophic losses for them.

Reputational damage to the protocol. Although this doesn't directly affect other rTokens, it will no doubt cause depositors to pull out of those tokens due to the perceived risk of it happening again. This would cause large loss of TVL across the protocols as well as devaluation of the rsr asset.

I am aware that governance proposals have a voting and execution delay. However as stated in deployment-variables.md donations can delay this auction:

Setting this too low may allow griefers to delay important auctions. The variable should be set such that 
donations of size minTradeVolume would be worth delaying trading batchAuctionLength seconds in
the event of urgent recollateralization.

Assuming an auction length of 30 minutes and \$100 min trade size, the seizure of rsr could be delayed for $4,800 per day worth of donations allowing any reasonable delay to be bypassed without seizure.

Tools Used

Manual Review

Recommended Mitigation Steps

As simple require statement should be added to assetRegistry#unregister:

function unregister(IAsset asset) external governance {
    require(_erc20s.contains(address(asset.erc20())), "no asset to unregister");
    require(assets[asset.erc20()] == asset, "asset not found");
++  require(asset.erc20() != main.rsr(), "rsr cannot be unregistered");
    try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
        if (quantity != 0) basketHandler.disableBasket(); // not an interaction
    } catch {
        basketHandler.disableBasket();
    }

    _erc20s.remove(address(asset.erc20()));
    assets[asset.erc20()] = IAsset(address(0));
    emit AssetUnregistered(asset.erc20(), asset);
}

Assessed type

Governance