Open code423n4 opened 1 year ago
0xean
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago tbrent marked the issue as sponsor disputed
c4-judge
commented
1 year ago 0xean marked the issue as unsatisfactory: Invalid
0xA5DF
commented
1 year ago 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
commented
1 year ago tbrent marked the issue as sponsor confirmed
tbrent
commented
1 year ago 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
commented
1 year ago 0xean marked the issue as satisfactory
tbrent
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago Got it, checks out. Thanks!
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 torebalance()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:
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