search

code-423n4 / 2024-04-revert-mitigation-findings

1 stars 1 forks source link

Wrong logic in L2 sequencer check #5

Open c4-bot-9 opened 4 months ago

c4-bot-9 commented 4 months ago

Lines of code

https://github.com/revert-finance/lend/blob/audit/src/V3Oracle.sol#L360-L362

Vulnerability details

C4 issue

ADD-02: Missing L2 sequencer checks for Chainlink oracle

Impact

Proof of concept

The original issue is fixed by PR #27 The mitigation code adds sequencer check as follows:

// sequencer check on chains where needed
        if (sequencerUptimeFeed != address(0)) {
            (, int256 sequencerAnswer, uint256 startedAt,,) =
                AggregatorV3Interface(sequencerUptimeFeed).latestRoundData();

            // Answer == 0: Sequencer is up
            // Answer == 1: Sequencer is down
            if (sequencerAnswer == 0) {
                revert SequencerDown();
            }

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

However, as you can see in the comment sequencerAnswer == 0 indicates that the sequencer is up, yet in that case the code reverts with SequencerDown error (wrong logic). This logic is also stated in the docs: The message calls the updateStatus function in the ArbitrumSequencerUptimeFeed contract and updates the latest sequencer status to 0 if the sequencer is up and 1 if it is down.

Since most of the time the the sequencer is up (sequencerAnswer == 0), the function will revert most of the time and affect many other functions/contracts.

Recommended Mitigation Steps

Change if (sequencerAnswer == 0) to if (sequencerAnswer == 1).

Assessed type

Invalid Validation

c4-sponsor commented 4 months ago

kalinbas (sponsor) confirmed

BogoCvetkov commented 4 months ago

Some additional info for the judges: Me and @thank_you have marked this issue directly as Unmitigated, instead of submitting it as a separate finding

jhsagd76 commented 4 months ago

Some additional info for the judges: Me and @thank_you have marked this issue directly as Unmitigated, instead of submitting it as a separate finding

This issue is a mitigation error instead of the state of Unmitigated.

So I will upgrade both of these issues to the dup for this issue

c4-judge commented 4 months ago

jhsagd76 marked the issue as satisfactory

c4-judge commented 4 months ago

jhsagd76 marked the issue as selected for report

c4-judge commented 4 months ago

jhsagd76 marked the issue as primary issue