c4-bot-8 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

In some cases, a team will not be granted enough CLOCK_EXTENSION time to generate the initial instruction trace and perform a counter claim.

Proof of Concept

In FaultDisputeGame::move, game clocks will grant extensions to ensure a team has enough time to counter a potential claim and/or generate an execution trace bisection root claim. A special case is when the potential grandchild is executiontrace bisection root claim ( SPLIT_DEPTH + 1), twice the extension time (2 * CLOCK_EXTENSION.raw()) should be ensured.

The problem is that the current implementation might still cause a team to have less than 2 * CLOCK_EXTENSION.raw() time to generate an execution trace bisection root claim and perform a counterclaim. Specifically, if (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw()) condition check is vulnerable.

//packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol
  function move(
    Claim _disputed,
    uint256 _challengeIndex,
    Claim _claim,
    bool _isAttack
  ) public payable virtual {
...
|>      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);
        }
...

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

Suppose CLOCK_EXTENSION.raw() is 3 hours, MAX_CLOCK_DURATION.raw() is 84 hours (3.5 days).

Case A: At time of move(), nextDuration.raw() is 82 hours. nextPosition is at SPLIT_DEPTH -1.

nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw(). The grandchild is at SPLIT_DEPTH + 1 (execution trace root).

extensionPeriod: 6h (CLOCK_EXTENSION.raw() 2) nextDuration: 78h (MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw() 2)

Case B: At time of move(), nextDuration.raw() is 81 hours. nextPosition is at SPLIT_DEPTH -1.

B-1: nextDuration.raw() == MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw(). The grandchild is at SPLIT_DEPTH + 1 (execution trace root).

The if condition will be bypassed.

extensionPeriod: 0h nextDuration: 81h

B-2: The oppposing team attack.

B-3: The grandchild now has max. 3h to generate execution trace root claim and perform an counter claim. This is half of the intended CLOCK_EXTENSION.raw() * 2.

As seen in caseB, in normal conditions, a team that need to create both executiontrace root claim and perform a counter claim may not get the minimum CLOCK_EXTENSION.raw() * 2 time. A team's clock may run out prematurely.

Tools Used

Manual

Recommended Mitigation Steps

Consider change to the following:

...
uint64 extensionPeriod;
if (nextPositionDepth != SPLIT_DEPTH - 1 && (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - CLOCK_EXTENSION.raw())) {
     extensionPeriod = CLOCK_EXTENSION.raw();
} else if (nextPositionDepth == SPLIT_DEPTH - 1 && (nextDuration.raw() > MAX_CLOCK_DURATION.raw() - 2* CLOCK_EXTENSION.raw())) {
     extensionPeriod = CLOCK_EXTENSION.raw() * 2;
}
nextDuration = Duration.wrap(MAX_CLOCK_DURATION.raw() - extensionPeriod);
...

Assessed type

Other

zobront commented 3 months ago

Reminder that @xuwinnie had this issue in his QA report, so should be dup'd in with this one.

ajsutton commented 3 months ago

This is accurate. There was a fair bit of debate about whether 2 * CLOCK_EXTENSION was actually required at the split depth so this doesn't concern me, though if we extend the limit for one side it seems reasonable to expect it to be extended for the other side as well.

c4-judge commented 3 months ago

zobront marked the issue as satisfactory