Dutch auctions can fail to settle if any other collateral in the basket behaves unexpectedly

[c4-bot-4]

Lines of code

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

Vulnerability details

When a Dutch auction that originated from the backing manager receives a bid, it calls BackingManager.settleTrade() to settle the auction immediately, which attempts to chain into another rebalance() call. This chaining is implemented using a try-catch block that attempts to catch out-of-gas errors.

However, this pattern is not safe because empty error data does not always indicate an out-of-gas error. Other types of errors also return no data, such as calls to empty addresses casted as contracts and revert / require statements with no error message.

The rebalance() function interacts with multiple external assets and performs several operations that can throw empty errors:

1. In basketsHeldBy(), which calls _quantity(), which in turn calls coll.refPerTok() (this function should in theory never revert, but in case it interacts with the underlying ERC20, its implementation may have been upgraded to one that does).
2. In prepareRecollateralizationTrade(), which calls basketRange(), which also calls _quantity().
3. In tryTrade() if a new rebalancing trade is indeed chained, which calls approve() on the token via AllowanceLib.safeApproveFallbackToMax(). This is a direct interaction with the token and hence cannot be trusted, especially if the possibility of upgradeability is considered.

If any of these operations result in an empty error, the auction settlement will fail. This can lead to the Dutch auction being unable to settle at a fair price.

Note: we have found this finding pointing out the very same issue in a previous audit, but this report highlights a different root cause in where the error originates.

Impact

Dutch auctions may fail to settle at the appropriate price or at all.

Proof of Concept

1. A Dutch auction is initiated for rebalancing collateral.
2. After some time, a bidder attempts to submit a bid at fair market value.
3. BackingManager.settleTrade() is called by the trade contract.
4. The rebalance() function is called within the try-catch block.
5. The underlying ERC-20 of one of the collateral assets in the basket has an empty revert clause that currently throws when one of its functions is called.
6. The catch block receives an empty error and reverts the transaction.

Tools Used

Manual review

Recommended Mitigation Steps

Avoid usage of this pattern to catch OOG errors in any functions that cannot revert and may interact with external contracts. Instead, in such cases always employ the _reserveGas() pattern that was iterated on to mitigate previous findings (1, 2, 3) with a similar root cause. We have found no other instances in which this applies.

Assessed type

DoS

[akshatmittal]

1. This is a known issue.
2. The ERC20 upgrade to return empty revert data on calling any of its functions seems a little far fetched.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[0xEVom]

@thereksfour we do not think this should be considered a known issue either, unless it was accepted in a previous contest or pointed out in an audit.

An empty revert in one function of a collateral asset being characterized as far-fetched is a little surprising, considering findings were accepted in previous Reserve contests for the same situation but the token reverting, consuming all gas and consuming a specific amount of gas.

All those findings concerned the ability to unregister a misbehaving asset, which we found to now be guaranteed. However, we found an asset misbehaving could also have the additional impact of preventing auctions from settling for a different asset. This same impact was accepted as valid for a different root cause here.

Again, an empty revert is nothing unusual and a simple require() with no error message will produce it.

We think this scenario is very much realistic and would like to kindly ask for it to be reassessed.

[thereksfour]

@akshatmittal and @tbrent This seems to be a possible upgrade, please take a look, thanks!

Again, an empty revert is nothing unusual and a simple require() with no error message will produce it.

[akshatmittal]

Looking back at this again @thereksfour.

The first two statements which hinge on refPerTok reverting are not valid since we require refPerTok to not revert. If a collateral plugin does revert on it, it must be fixed and replaced. The third example however, the approve one, is where I can see the token revert causing issues.

I currently can not see any sane ERC20 reverting on an approve case with no message, however you may have better examples than I do. I still consider it highly unlikely, although if you do have examples to share I'll consider them.

And honestly, I currently do not see how to do better. For a little more context on that, we want settle to start a new auction, which is why that revert exists there, and we can't use the _reserveGas pattern here since the gas cost for rebalance is unbound.

RTokens are designed to be governance focused, and we already have the requirement for Governance to only include collaterals they absolutely trust (which is why you'd see all RTokens today use blue chip assets only).

If you absolutely must consider it valid, I'd probably bring it down to L/QA given the requirements here, but also looking for your thoughts.

[thereksfour]

@0xEVom If there's no example, I'll invalidate it because the assumption isn't valid

[0xEVom]

@thereksfour USDT and BNB throw empty errors on reverts within approve(), for instance.

These are the two largest market cap ERC-20 tokens in existence - again, this is not some theoretical esoteric behaviour but a realistic scenario.

There may not be a better approach if the gas cost of rebalance() is unbounded as you say @akshatmittal. But lack of an immediate mitigation does not invalidate the issue/make it QA.

[akshatmittal]

@0xEVom Both of the examples you have mentioned throw on zero, which is a case handled within the code. (Also just saying here, BNB isn't technically a supported token for other reasons)

this is not some theoretical esoteric behaviour but a realistic scenario.

Believe me, I'm not trying to say so. I'm really trying to find a realistic case where an upgrade on the token makes it regress in a basic ERC20 function.

And yeah, I'm also not saying not having a mitigation invalidates the issue, but rather that the protocol has ways of dealing with such specific things like wrapping the tokens, etc. We already wrap tokens that we don't like behaviours of, or tokens that have weird behaviours.

(Talking to cccz to accept this, just trying to get a better idea)

[thereksfour]

Although the likelihood is low, the assumed token satisfies acceptable upgradability, will upgrade it to M.

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report