An oracle deprecation might lead the protocol to sell assets for a low price

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L87-L96

Vulnerability details

During a Dutch Auction, if a user places a bid, the trade is settled in the same transaction. As part of this process, the backing manager tries to call the rebalance() function again.
The call to rebalance() is wrapped in a try-catch block, if an error occurs and the error data is empty, the function will revert.

The assumption is that if the error data is empty that means it was due to an out-of-gas error, this assumption isn't always true as mentioned in a previous issue (that wasn't mitigated).
In the case of this issue, this can result in a case where users can't bid on an auction for some time, ending up selling an asset for a price lower than the market price.

Impact

Protocol's assets will be auctioned for a price lower than the market price

Proof of Concept

Consider the following scenario:

• Chainlink announces that an oracle will get deprecated
• Governance passes a proposal to update the asset registry with a new oracle
• A re-balancing is required and executed with a Dutch Auction
• The oracle deprecation happens before the auction price reaches a reasonable value
• Any bid while the oracle is deprecated will revert
• Right before the auction ends the proposal to update the asset becomes available for execution (after the timelock delay has passed). Somebody executes it, bids, and enjoys the low price of the auction.

Recommended Mitigation Steps

On top of checking that the error data is empty, compare the gas before and after to ensure this is an out-of-gas error.

Assessed type

Error

[0xean]

On the fence on this one, it is based off a known issue from a previous contest but does show a new problem stemming from the same problem of oracle deprecation.

Look forward to sponsor comment.

[tbrent]

The PoC does not function as specified. Specifically, bidding on an auction does not involve the price at the time of the tx. The price is set at the beginning of the dutch auction in the init() function. Therefore, it is the starting of new auctions that will revert while the oracle is deprecated, while bids will succeed and simply fail to start the next auction.

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[c4-judge]

0xean marked the issue as unsatisfactory:
Invalid

[0xA5DF]

Therefore, it is the starting of new auctions that will revert while the oracle is deprecated, while bids will succeed and simply fail to start the next auction.

Hey @tbrent
Didn't quite understand the dispute here, if starting the next auction will fail/revert then the bid will revert too.
bid() calls origin.settleTrade() and settleTrade() calls rebalance()
If rebalance() reverts due to a deprecated oracle then settleTrade() will revert too (rebalance() will revert with empty data, and therefore the catch block will trigger a revert here)

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[tbrent]

Ah, understood now. Agree this is Medium and think it should be counted as a new finding since the consequence (dutch auction economics break) is novel.

[c4-judge]

0xean marked the issue as satisfactory

[tbrent]

Hey @0xA5DF -- we're having some confusion around exactly what happens when a chainlink oracle is deprecated. Do you have details to share about what this ends up looking like?

We're having trouble finding documentation on this, and it feels like the aggregator contract should just stay there and return a stale value. Is that not right? Has this happened in the past or has Chainlink committed to a particular approach for deprecating?

[0xA5DF]

Hey
It's a bit difficult to track deprecated Chainlink oracles since Chainlink removes the announcement once they're deprecated.
I was able to track one Oracle that was deprecated during the first contest, from the original issue this seems to be this one.
It seems that what happens is that Chainlink sets the aggregator address to the zero address, which makes the call to latestRoundData() to revert without any data (I guess this is due to the way Solidity handles calls to a non-contract address).
See also the PoC in the original issue in the January contest

[tbrent]

Got it, checks out. Thanks!