Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L319-L416

Vulnerability details

Summary

The move implementation logic allows for a bypass due to break in logic leading to move still being challenged even when its clock has exceeded the MAX_CLOCK_DURATION

Description

According to the natspec:

INVARIANT: A move can never be made once its clock has exceeded MAX_CLOCK_DURATION seconds of time.

However, this invariant does not hold due to the fact that, the check which is meant to explicitly handle this is not being properly implemented.

The current check =>

    if (nextDuration.raw() == MAX_CLOCK_DURATION.raw()) revert ClockTimeExceeded();

The above explicitly checks that if the nextDuration equals the MAX_CLOCK_DURATION, it should revert. But in certain situations, the nextDuration is going to be greater than the MAX_CLOCK_DURATION in which according to the INVARIANT is also meant to revert but wont do so in this case.

It is known that the main reason for the above implementation is to allow for CLOCK_EXTENSION which is used to grant the potential grandchild's clock extensions seconds. This checks if the (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw()). However, if this is'nt the case i.e there is no CLOCK_EXTENSION, a move can still be made when it's clock exceeds MAX_CLOCK_DURATION seconds of time

Impact

Break in logic
Claim can still be made after maximum clock duration has elapsed.
Possibility of a dishonest actor proving a false claim.
Removes the safeguards meant to be in place

Proof of Code

    function move(Claim _disputed, uint256 _challengeIndex, Claim _claim, bool _isAttack) public payable virtual {
        // INVARIANT: Moves cannot be made unless the game is currently in progress.
        if (status != GameStatus.IN_PROGRESS) revert GameNotInProgress();

        ...

        Duration nextDuration = getChallengerDuration(_challengeIndex);

        // INVARIANT: A move can never be made once its clock has exceeded `MAX_CLOCK_DURATION`
        //            seconds of time.
        if (nextDuration.raw() == MAX_CLOCK_DURATION.raw()) revert ClockTimeExceeded(); //@audit should be >=, otherwise a move can still be challenged if its clock has exceeded the MAX_CLOCK_DURATION

        // If the remaining clock time has less than `CLOCK_EXTENSION` seconds remaining, grant the potential
        // grandchild's clock `CLOCK_EXTENSION` seconds. This is to ensure that, even if a player has to inherit another
        // team's clock to counter a freeloader claim, they will always have enough time to to respond. This extension
        // is bounded by the depth of the tree. If the potential grandchild is an execution trace bisection root, the
        // clock extension is doubled. This is to allow for extra time for the off-chain challenge agent to generate
        // the initial instruction trace on the native FPVM.
        if (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw()) {
            // If the potential grandchild is an execution trace bisection root, double the clock extension.
            uint64 extensionPeriod =
                nextPositionDepth == SPLIT_DEPTH - 1 ? CLOCK_EXTENSION.raw() * 2 : CLOCK_EXTENSION.raw();
            nextDuration = Duration.wrap(MAX_CLOCK_DURATION.raw() - extensionPeriod);
        }

        // Construct the next clock with the new duration and the current block timestamp.
        Clock nextClock = LibClock.wrap(nextDuration, Timestamp.wrap(uint64(block.timestamp)));

        ...

        emit Move(_challengeIndex, _claim, msg.sender);
    }

Recommendation

Restructure the logic check to ensure A move can never be made once its clock has exceeded MAX_CLOCK_DURATION seconds of time

Assessed type

Invalid Validation

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

Break in logic implementation of `move` #95