c4-bot-10 commented 3 months ago

Lines of code

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

Vulnerability details

RToken contracts, and in particular AssetRegistry and BasketManager are robust to weird behaviors of collaterals such as reverting on regular ERC20 operations, in the sense that at any point in time it is possible to remove a collateral from the basket without ever calling it.

The same level of protection is however not achieved when a collateral becomes unresponsive after it's been auctioned for recollateralization.

BackingManager.rebalance operations calls can only happen when there are no trades open:

File: BackingManager.sol
107:     function rebalance(TradeKind kind) external nonReentrant {
---
120:         require(tradesOpen == 0, "trade open");

and the only place that brings a non-zero tradesOpen back to 0 is Trading.settleTrade, which in turn requires the trade DutchTrade or GnosisTrade to successfully settle.

If an auctioned (sell) or the raised (buy) token reverts on balanceOf or transfer calls, both DutchTrade and GnosisTrade will revert on settle calls, making it impossible to bring tradesOpen back to 0, therefore preventing BackingManager from resuming normal operation:

File: DutchTrade.sol
File: DutchTrade.sol
274:     function settle()
---
278:     {
---
290:         boughtAmt = buy.balanceOf(address(this)); // {qBuyTok}
291:         buy.safeTransfer(address(origin), boughtAmt); // {qBuyTok}
---
292:         sell.safeTransfer(address(origin), sell.balanceOf(address(this))); // {qSellTok}

File: GnosisTrade.sol
175:     function settle()
---
179:     {
---
194:         uint256 sellBal = sell.balanceOf(address(this));
---
203:         boughtAmt = buy.balanceOf(address(this));
---
205:         if (sellBal != 0) IERC20Upgradeable(address(sell)).safeTransfer(origin, sellBal);
---
206:         if (boughtAmt != 0) IERC20Upgradeable(address(buy)).safeTransfer(origin, boughtAmt);

Impact

Tokens that become nonfunctional, for example after a failed upgrade, while being auctioned or raised by BackingManager, will permanently DoS its recollateralization capabilities.

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider adding a governance action allowing BackingManager to force-settle (essentially writing off) token sales that can't be normally settled.

Assessed type

DoS

tbrent commented 2 months ago

We want to confirm this issue, but not because of the upgrade case -- more clear is the blocklisting case. The protocol is not intended to get stuck in this case and we have tests cases trying to show this, but it fails when trades are open.

Think the recommended mitigation is the best one available.

tbrent commented 2 months ago

We think this is L severity, however. The ERC20 must be upgraded during the auction to censor the trade address, which is a really significant assumption.

c4-judge commented 2 months ago

thereksfour changed the severity to QA (Quality Assurance)

0xEVom commented 2 months ago

@thereksfour we think this is a textbook M severity finding and would kindly ask for it to be reconsidered.

The ERC-20 behaviour in case here (be it an upgrade or the trade address being blocklisted) is low probability but in-scope, as per the README, and the assumption that it must take place during the auction makes it M severity:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Here, the function of the protocol is fatally impacted and there is a "hypothetical attack path with stated assumptions, but external requirements".

This clearly seems like a scenario the protocol would want to prevent from happening no matter what (which as the sponsor says it intends to) and something that will be addressed. We think M severity makes sense here and is the correct categorization as per C4 rules.

thereksfour commented 2 months ago

For issues that have too low likelihood, I'll tend to consider it QA. If the tokens were required to be of a certain type ( in scope ) I would consider it M, but the assumption here goes further, that the behavior must be triggered at a specific time, which to me is QA.

however. The ERC20 must be upgraded during the auction to censor the trade address, which is a really significant assumption.

code-423n4 / 2024-07-reserve-findings