`RevenueTrader` can sell unpriced assets for near zero value

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RevenueTrader.sol#L109

Vulnerability details

Description

The RevenueTrader contract trades excess balances of its assets registered in the asset registry for either RSR or RToken. The assets are sold via batch or Dutch auction, in batch auction to the buyer who bids the highest amount of buy assets for the selling assets, and in the Dutch auction to the buyer who bids first, with the price going down over time. However, the revenue trader can open trades even for unpriced assets, i.e., the assets that return (0, FIX_MAX) on price() (for example, when the oracle had a long timeout).

Proof of concept

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RevenueTrader.sol#L109

Let's assume that RevenueTrader holds an asset A. The oracle for this asset has been in timeout longer than the decayDelay + priceTimeout. When we can call manageTokens providing this asset, the contract will load its price, which can result in (0, FIX_MAX) given the conditions mentioned above. When it comes to preparing the trade, the inline docs say

Whether dust or not, trade the non-target asset for the target asset Any asset with a broken price feed will trigger a revert here

This is not true, however, since a price feed with a long timeout, as in our example, should be considered broken, and no revert is triggered. This leads to the minBuyAmount being 0. Although the trade would be regarded as dust at this point, the RevenueTrader does not care about dust trades, and since it has assets to sell, it will try to open the trade. Note that we can only open a GnosisTrade, as DutchTrade would revert on zero price.

Note that even if the trade launched is the batch auction, which allows other users to move the final price higher while the auction is ongoing, the trade is started via manageTokens, which is a permissionless call. This means that if a user spots such an opportunity and starts the trade themselves, they might be the only one with their eyes on the trade. This would mean that a potentially unprofitable trade would be opened, selling potentially valuable assets for near-zero value.

Impact and likelihood

The likelihood should be considered LOW as it depends on a timeout of an oracle longer than the specified decay delay + price timeout. There is however still a The impact should be considered HIGH as it results in a loss of value for the respective RToken system. Therefore, the severity of the issue should be considered MEDIUM.

Recommendation

Consider reverting in prepareTradeSell if the sellLow price is 0. This should be fine for reasonable assets expected to be used in the protocol, as a reasonable price feed should exist for any valid asset. However, this would eliminate a potential loss of value.

Assessed type

Invalid Validation

[thereksfour]

already checked it.

        (uint192 buyLow, uint192 buyHigh) = assetToBuy.price(); // {UoA/tok}
        require(buyHigh != 0 && buyHigh != FIX_MAX, "buy asset price unknown");

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[coreggon11]

Hi @thereksfour, this check checks the price of the asset that we are buying (rsr / rtoken), not the asset we are selling. Let me give more context to this issue.

Here we get the price of the sell token, not validated anywhere: https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RevenueTrader.sol#L161-L177

The price of an asset can indeed be (0, FIX_MAX) if the oracle has been out for some time, as we can see for example here: https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/assets/Asset.sol#L144-L146

Then in the TradeLib.prepareTradeSell function which again only checks for buy price: https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/mixins/TradeLib.sol#L48-L52

So all the checks pass and trade will be launched: https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RevenueTrader.sol#L181

Note that only gnosis trade can be launched since in Dutch trade the sell price must not be zero, but such trades should not happen IMO. If an asset is unpriced, there must be an issue with the oracle. If a price feed does not exist for an asset, it would technically mean that whoever bids the highest amount gets the assets. But such assets should be listed in some kind of whitelist or something like that. Combined with any other issue found regarding the trades, at least some attention should be given to this issue by the team.

[thereksfour]

@akshatmittal and @tbrent , please take a look at this, thanks!

[tbrent]

I would say that such trades should happen, and the current behavior is acceptable.

1. A [0, FIX_MAX] price does not happen all of a sudden: it first goes decays over the priceTimeout period (1 week in deployment scripts). Hence the asset has many other chances to get sold first. The fact that no sale has occurred indicates something about the price. It is permissionless to start auctions.
2. Even if this were not the case, one advantage of a batch auction from a mechanistic behavior is that you do not need to know anything about the relative prices of the two assets being traded in order to discover a good clearing price.
3. Finally, the protocol only performs revenue trades for [0, FIX_MAX]-priced assets. The BackingManager will skip over [0, FIX_MAX]-priced assets. So the stakes are lower.