peanuts - It is possible for an earlier claim to resolve before a later one

[sherlock-admin3]

peanuts

medium

It is possible for an earlier claim to resolve before a later one

Summary

When resolving claims, the claim at the granular level (at MAX_DEPTH) is resolved first and will perforate upwards until all the claims are resolved. It is possible that an earlier claim can be resolved before a later claim.

Vulnerability Detail

In every subgame apart from the root, there can be an attack and defense claim. Imagine there are 8 claims: This is how the claim is played out. The first claim is the root claim. The second claim is an attack of the root claim. The third and fourth claim is an attack and defence of the second claim respectively. The fifth and sixth claim is the attack and defense of the third attack claim.

claimData[0] = rootClaim claimData[1] = Attack claim, subgame[0] = [1] claimData[2] = Attack claim against claimData[1], subgame[1] = [2] claimData[3] = Defense claim for claimData[1], subgame[1] = [2,3] claimData[4] = Attack claim against claimData[2], subgame[2] = [4] claimData[5] = Defense claim for claimData[2], subgame[2] = [4,5] claimData[6] = Attack claim against claimData[3], subgame[3] = [6] claimData[7] = Defense claim against claimData[3], subgame[3] = [6,7]

There are 8 claims with 4 subgames.

When it comes to resolveClaims(), the later claims should be resolved first. In other words, claimData[1] cannot be resolved unless the other claims have been resolved.

Looking at the resolveClaim() function, it does check that the earlier claims must be resolved first. However, this check does not hold for subgames with both attack and defense claims.

  for (uint256 i = 0; i < challengeIndicesLen; ++i) {
            uint256 challengeIndex = challengeIndices[i];

>           // INVARIANT: Cannot resolve a subgame containing an unresolved claim
>           if (subgames[challengeIndex].length != 0) revert OutOfOrderResolution();

            ClaimData storage claim = claimData[challengeIndex];

            // If the child subgame is uncountered and further left than the current left-most counter,
            // update the parent subgame's `countered` address and the current `leftmostCounter`.
            // The left-most correct counter is preferred in bond payouts in order to discourage attackers
            // from countering invalid subgame roots via an invalid defense position. As such positions
            // cannot be correctly countered.
            // Note that correctly positioned defense, but invalid claimes can still be successfully countered.
            if (claim.counteredBy == address(0) && leftmostCounter.raw() > claim.position.raw()) {
                countered = claim.claimant;
                leftmostCounter = claim.position;
            }
        }

Instead of resolving subgames[3], a player decides to resolve subgames[2] first. Since there are no subgames[4] and subgames[5], subgames[2] is able to resolve before subgames[3]

    uint256[] storage challengeIndices = subgames[_claimIndex];
        uint256 challengeIndicesLen = challengeIndices.length;

• claimIndex = 2
• challengeIndices = subgames[2] = [4,5]
• challengeIndicesLen = 2

        for (uint256 i = 0; i < challengeIndicesLen; ++i) {
            uint256 challengeIndex = challengeIndices[i];

            // INVARIANT: Cannot resolve a subgame containing an unresolved claim
            if (subgames[challengeIndex].length != 0) revert OutOfOrderResolution();

- challengeIndex = challengeIndices[0] = 4
- if (subgames[4].length != 0) revert (doesn't revert as the length of subgames[4] is 0.
- The function continues and ` delete subgames[_claimIndex]` is called

subgame[2] is resolved before subgame[3].

Impact

Breaks the invariant that the later claims must be resolved before the earlier ones

Code Snippet

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

Tool used

Manual Review

Recommendation

Not sure if intended or not, but with only one move() per subgame, later claims will never be able to resolve before earlier ones.

[nevillehuang]

Based on scope details below, any issue related to FDG/FDG subgames resolution logic with root cause stemming from FaultDisputeGame contract will be considered OOS of this contest if airgap and/or delayed WETH mechanism implemented for off-chain review of game results and bond distribution is not shown to be bypassed

https://docs.google.com/document/d/1xjvPwAzD2Zxtx8-P6UE69TuoBwtZPbpwf5zBHAvBJBw/edit

[cryptostaker2]

Hi @nevillehuang, I have no escalation so just going to tag you.

I believe this issue shows that an invariant has been bypassed, and there is no safeguard for this.

[0xjuaan]

yeah this is a dupe of #164

[nevillehuang]

Agree, this is a duplicate of #164, but based on context of agreed upon scope, this should remain invalid and will likely be rewarded separately from the contest pot.

[sherlock-admin2]

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