073.md

Deep Sepia Gazelle

High

The checks that confirm that the sequencer is up in OracleUtils::checkSequencerActive modifier are inefficient for Arbitrum chain

Summary

The protocol will be deployed on three chains: Arbitrum, Base and Ethereum. The protocol uses Chainlink Oracle to get the price and performs checks to ensure that the Sequencer is active. The problem is that these checks are not sufficient for the Arbitrum chain.

Root Cause

In the chainlink docs is written that sequencerUptimeFeed can return 0 value for startedAt on Arbitrum when the Sequencer Uptime contract is not yet initialized:

The sequencerUptimeFeed object returns the following values:

answer: A variable with a value of either 0 or 1
0: The sequencer is up
1: The sequencer is down

startedAt: This timestamp indicates when the sequencer feed changed status. When the sequencer comes back up after an outage, wait for the GRACE_PERIOD_TIME to pass before accepting answers from the data feed. Subtract startedAt from block.timestamp and revert the request if the result is less than the GRACE_PERIOD_TIME.

The startedAt variable returns 0 only on Arbitrum when the Sequencer Uptime contract is not yet initialized. For L2 chains other than Arbitrum, startedAt is set to block.timestamp on construction and startedAt is never 0. After the feed begins rounds, the startedAt timestamp will always indicate when the sequencer feed last changed status.
If the sequencer is up and the GRACE_PERIOD_TIME has passed, the function retrieves the latest answer from the data feed using the dataFeed object.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

We can have a scenario where a round begins with startedAt set to 0 and answer, the initial status, also set to 0. According to the documentation, if answer equals 0, it indicates that the sequencer is up, while a value of 1 indicates that the sequencer is down. However, in this situation, both answer and startedAt can initially be 0. These values will only be updated to reflect the correct status of the sequencer after all data has been retrieved from oracles and the update is confirmed.

The inefficient checks to confirm the correct status of the sequencer leads the OracleUtils::checkSequencerActive modifier to not revert even when the sequencer uptime feed is not updated or it is called in an invalid round.

PoC

The checks in the OracleUtils::checkSequencerActive modifier to ensure that the Sequencer's status is up are the following:

modifier checkSequencerActive() {
    if (sequencerUptimeFeed != address(0)) {
        (
            ,
            /*uint80 roundID*/ int256 answer,
            uint256 startedAt /*uint256 updatedAt*/ /*uint80 answeredInRound*/,
            ,

        ) = AggregatorV2V3Interface(sequencerUptimeFeed).latestRoundData();

        // Answer == 0: Sequencer is up
        // Answer == 1: Sequencer is down
@>      bool isSequencerUp = answer == 0;
@>      if (!isSequencerUp) {
            revert SequencerDown();
        }

        // Make sure the grace period has passed after the
        // sequencer is back up.
@>      uint256 timeSinceUp = block.timestamp - startedAt;
@>      if (timeSinceUp <= GRACE_PERIOD_TIME) {
            revert GracePeriodNotOver();
        }
    }
    _;
}

But the check for the timeSinceUp is inefficient if its called on Arbitrum when the Sequencer Uptime contract is not yet initialized. In that case the startedAt will be 0 and block.timestamp - startedAt (for example: 1722184616 - 0 = 1722184616) will result in a value greater than GRACE_PERIOD_TIME that is 3600 and the code will not revert.

Mitigation

Add a check to ensure that startedAt is not 0:

modifier checkSequencerActive() {
    if (sequencerUptimeFeed != address(0)) {
        (
            ,
            /*uint80 roundID*/ int256 answer,
            uint256 startedAt /*uint256 updatedAt*/ /*uint80 answeredInRound*/,
            ,

        ) = AggregatorV2V3Interface(sequencerUptimeFeed).latestRoundData();

+       bool validRound = startedAt > 0;
+       if (!validRound){
+          revert InvalidUptimeFeedRound();
+       }

        // Answer == 0: Sequencer is up
        // Answer == 1: Sequencer is down
        bool isSequencerUp = answer == 0;
        if (!isSequencerUp) {
            revert SequencerDown();
        }

        // Make sure the grace period has passed after the
        // sequencer is back up.
        uint256 timeSinceUp = block.timestamp - startedAt;
        if (timeSinceUp <= GRACE_PERIOD_TIME) {
            revert GracePeriodNotOver();
        }
    }
    _;
}