Lack of sequencer uptime checks can lead to dutch auctions executing at bad prices or failing

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Proof of Concept

First, per the readMe, protocol is to be deployed on multiple chains, optimistic L2s included, see https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/README.md#L140

| Chains the protocol will be deployed on | Ethereum,Arbitrum,Base,Optimism |

The protocol implements Dutch auction trades via DutchTrade.sol. The auction mechanism is designed with a specific price decay structure over time: see the docs for a short map on how these auctions should work.

// A dutch auction in 4 parts:
//   1.  0% -  20%: Geometric decay from 1000x the bestPrice to ~1.5x the bestPrice
//   2. 20% -  45%: Linear decay from ~1.5x the bestPrice to the bestPrice
//   3. 45% -  95%: Linear decay from the bestPrice to the worstPrice
//   4. 95% - 100%: Constant at the worstPrice

However, the contract does not include any checks for sequencer uptime. This can be problematic because network outages and reorgs happen quite often with optimistic L2s.

Now to place bids we have https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/plugins/trading/DutchTrade.sol#L199-L205

    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 //@audit
..snip
    }

Take a look at the _price() function, which calculates the auction price based on the current timestamp:

function _price(uint48 timestamp) private view returns (uint192) {
  uint48 _startTime = startTime; // {s} gas savings
  uint48 _endTime = endTime; // {s} gas savings
  require(timestamp >= _startTime, 'auction not started');
  require(timestamp <= _endTime, 'auction over');

  uint192 progression = divuu(timestamp - _startTime, _endTime - _startTime); // @audit

  // ...snip
}

Now, if there's a sequencer downtime, which we should agree the likelihood to be substantial considering the various chains protocol is to deploy to the timestamp used for price calculation could be significantly off, leading to incorrect pricing.

Impact

Auctions could execute at unfavorable prices if there's a significant delay between when a bid is submitted and when it's processed due to sequencer downtime, even worse these auctions could fail entirely if they end during a period of sequencer downtime, as no bids would be able to be placed.

Since "sequencer downtime" can be seen as a twin window to reorg issues, would be key to note that since create2 is not being used for these deployments of the auctions then we have issues where users might bid on the wrong auction due to a swap of the addresses since the reorg could switch addresses for 2 different auctions. Another subtle scenario is say a user would like to mint right after the RToken was deployed, a reorg might cause for the deployment of a different RToken and trap the users' funds in it.

Tool used

• Manual review
• Previous finding

Recommended Mitigation Steps

Consider implementing a check for sequencer uptime using Chainlink's L2 Sequencer Uptime Feed. This can be used to ensure that the sequencer was online during the entire duration of an auction and then add a mechanism to extend or restart auctions if there was significant sequencer downtime during the auction period, and for the switch in address case, using create2 should sort this out.

Assessed type

Context

[akshatmittal]

It's a known issue, the Solidified audit already mentions it.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Out of scope

[Bauchibred]

Hi @thereksfour, thanks for judging. I'd appreciate if you could re-evaluate this report as in scope, this is because during the course of the audit, I went through all previous audit reports and in no instance was this issue mentioned, per the sponsor's comment here, I assume he's quoting the second issue from the Solidified 3.4.0 report since this is the only instance where there is a mention of the optimistic sequencer in the Solidified series, the linked issue however does not make this finding OOS considering the only thing covered in that report was the classic window of checking the seqeuncer to ensure Chainlink relays the right data. And if we go to the scope of that audit we find that the Reserve's OracleLib.sol contract was in scope, which is why the issue was raised, since in the implementation of OracleLib.sol we can see that prices are queried using Chainlink's latestRoundData(), see OracleLib.sol#L24-L59 from the current public Reserve git repo. In this report however the window is quite different with a higher impact considering this directly affects the nature as to which dutch auctions are going to be finalized.

[0xEVom]

@thereksfour agree, this is not a known issue. Here's the Solidified finding:

The finding points out that using Chainlink oracles on L2s without also checking for the L2 Sequencer Uptime feeds can be unreliable, which is a very specific issue related to Chainlink itself as documented in their docs.

This issue concerns Dutch auctions. The root cause is very different, and the issues are unrelated. It does not follow from the one that there is an issue in the other.

For reference, an equivalent finding was previously accepted here.

[akshatmittal]

Here's a past report from our previous C4 contest: https://github.com/code-423n4/2023-01-reserve-findings/issues/432

The Solidified comment was just an example, we have discussed this several times.

[thereksfour]

I think the root cause of them is the same, just the impact is different due to different scenarios, I still consider it a duplicate.