No check for sequencer uptime in `DutchTrade` can lead to dutch auctions failing or executing at bad prices

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/trading/DutchTrade.sol#L199

Vulnerability details

Impact

The DutchTrade contract implements a wholesale dutch auction via a 4-piecewise falling-price mechansim. However, there is no check for sequencer uptime, which could lead to auctions failing or executing at unfavorable prices.

Current deployment parameters set dutchAuctionLength to 15 minutes by default in L2s and accept the "Reasonable range" as 100 to 3600 seconds. This could have serious consequences in the event of a network outage.

Network outages and large reorgs happen with relative frequency. For instance, Arbitrum suffered a 4 hour-long outage 8 months ago.

Proof of Concept

DutchTrade::bid() and settle() are executed completely independent of the sequencer state as follows;

    function bid() external returns (uint256 amountIn) {
        require(bidder == address(0), "bid already received");
        assert(status == TradeStatus.OPEN);

        // {buyTok/sellTok}
        uint192 price = _price(uint48(block.timestamp)); // enforces auction ongoing

        // {qBuyTok}
        amountIn = _bidAmount(price);

        // Mark bidder
        bidder = msg.sender;
        bidType = BidType.TRANSFER;

        // reportViolation if auction cleared in geometric phase
        if (price > bestPrice.mul(ONE_POINT_FIVE, CEIL)) {
            broker.reportViolation();
        }

        // Transfer in buy tokens from bidder
        buy.safeTransferFrom(msg.sender, address(this), amountIn);

        // settle() in core protocol
        origin.settleTrade(sell);

        // confirm .settleTrade() succeeded and .settle() has been called
        assert(status == TradeStatus.CLOSED);
    }

Also take a look at other reports where the same situation occurs; 1 , 2

Tools Used

Neovim, Foundry

Recommended Mitigation Steps

To mitigate this issue, consider integrating an external uptime feed such as Chainlink's L2 Sequencer Feeds. This would allow the contract to invalidate an auction if the sequencer was ever offline during its duration.

Assessed type

Other

[0xpessimist]

I'm aware that the report in the findings repository was marked as OOS for the reason that it was already mentioned in the Solidified audit. However, I still want to do my escalation because the report in the Solidified audit simply suggests a Chainlink Best Practice as informational without establishing any connection to DutchTrade or determining an impact. (See: Solidified Reserve 3.4.0 Audit Report)

My thoughts on the validation decision:

I believe that adding a PoC like:

• Auction starts
• Network bad
• Network good
• Price bad

would only add aesthetic value to the report without contributing any additional reality to it.

The report clearly identifies the issue, describes the unique auction style of DutchTrade, references the default duration of a Dutch auction and the reasonable range. It also references the Arbitrum Outage, which actually happened and was even longer than the reasonable range for Dutch auctions, and shows that there are no checks on bid() and settle() about sequencer uptime. It also recommends the same mitigation as other reports.

My report is not the best among others but I believe it clearly demonstrates the issue and is a valid duplicate.

Additionally, I hope that being the first person (along with Ev_om) to report this issue on Code4rena (during the Ethereum Credit Guild contest) may show that I have a deep understanding of the issue.

[thereksfour]

https://github.com/code-423n4/2024-07-reserve-findings/issues/94#issuecomment-2323392463