search

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

5 stars 4 forks source link

Dutch auction participants can lose funds in case the auction creation transaction is part of a chain reorg #24

Closed c4-bot-10 closed 3 months ago

c4-bot-10 commented 3 months ago

Lines of code

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

Vulnerability details

The Broker contract creates DutchTrade instances using OZ's clone factory:

File: Broker.sol
227:     function newDutchAuction(
---
231:     ) private returns (ITrade) {
---
242:         DutchTrade trade = DutchTrade(Clones.clone(address(dutchTradeImplementation)));
---
252:         return trade;
---
253:     }

Because the clone factory uses the EVM CREATE opcode, it has has an unsafe outcome because the created address depends on the ordering of creations.

Reorgs of up to 7 slots have taken place post-Merge on mainnet, which is longer than the entire minimum duration of an auction. Hence a reorg from the moment an auction is deployed to the moment a bid is submitted is a low likelihood, but not unrealistic scenario.

When a chain reorg happens, it is possible that users submitting a bid for a given DutchAuction end up having their bid sent to another DutchAuction, which may present much less favorable conditions at the time of bidding.

Impact

Users participating to Dutch auctions are at risk of losing significant funds when the transactions that created the Dutch auctions are part of a chain reorg.

Proof of Concept

We start with a RToken where one of the RevenueTraders has a few tokens ready to be sold - let's say for RSR in this example.

Now a reorg happens, and transactions are ordered 2, 1, 3. The same transaction 3 is still valid, and if the RSR amount Ashley committed for it is enough, it can be executed successfully, and Ashley would receive a different token. This token is not only different from what she intended to bid for, but may also be bought at a price of up to 1000x the fair price if her transaction is executed shortly after transaction 1.

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider using the OZ factory Clones.cloneDeterministic instead of Clones.clone, possibly with a salt that includes the caller and req.sell token hashed.

Assessed type

Other

akshatmittal commented 3 months ago

We know that, and has been a part of a previous audit. In case of a reorg, it's technically also possible for transfer bidding transaction to come before the starting transaction. The reorg also changes the timing and price of the auction.

We expect bidders to do their own profitability calculation per their expectation.

tbrent commented 3 months ago

See: https://github.com/reserve-protocol/protocol/blob/72fc1f6e41da01e733c0a7e96cdb8ebb45bf1065/audits/Code4rena%20-%20Reserve%20Audit%20Report%20-%20Release%203.0.0%20(core).md#03-reorg-attack

c4-judge commented 3 months ago

thereksfour marked the issue as unsatisfactory: Invalid