Ward - The presence of `extraData` in the UUID generation extends uniqueness to an unhealthy extent

[sherlock-admin3]

Ward

medium

The presence of extraData in the UUID generation extends uniqueness to an unhealthy extent

Summary

The presence of extraData in the UUID generation greatly expands the uniqueness and allows spamming of dispute games for the same claim with same game type.

Vulnerability Detail

In the FaultDisputeGame contract it is stated that initialize function reverts if we try to get different UUIDs by simply adding extra bytes to the extraData that won't effect the game, as follows:

"Revert if the calldata size is too large, which signals that the extraData contains more than expected. This is to prevent adding extra bytes to the extraData that result in a different game UUID in the factory, but are not used by the game, which would allow for multiple dispute games for the same output proposal to be created. Expected length: 0x66 (0x04 selector + 0x20 root claim + 0x20 l1 head + 0x20 extraData + 0x02 CWIA bytes)"

However, this check only checks that the extraData is not longer than 32 bytes (0x20 in hex representation). This is an insufficient check because the only scenario it prevents is a scenario where we want to "create a game that has the same GameType and rootClaim with a different game whose extraData is exactly 32 bytes long, and we don't want to change the extraData but wanna add extra bytes."

However, it ignores the following cases:

• Spammers may find it okay to give a different extraData.

• extraData is shorter than 32 bytes and can change the remaining bytes even if we do not want to change the filled ones.

• extraData is already empty.

Theoretically in the first and last case,

Up to: $$256^{32} - MeaningfulCombinations$$ dispute games can be created with exactly the same rootClaim and GameType but different UUID, which is a pretty big number.

In the second case,

Up to: $$filledBytes - 256^{remainingBytes}$$ dispute games can be created with exactly the same rootClaim and GameType but different UUID, which is most likely to be a pretty big number.

There may be parameters that are not added to the formulas and may not reflect the truth completely, but they show the situation.

Impact

A spam attack can be made by using different extraData parameter for the same GameType and rootClaim (even using legit ones), creating unnecessary traffic which is very unhealthy for fault-proof mechanism.

Code Snippet

Entry of extraData: https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L87

UUID generation; Different extraData results in different UUID and thus we can create a dispute game with only the extraData parameter being different: https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L110

Length check for extraData. While it's a nice check, it doesn't prevent spam as expected: https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L541-L552

The following test demonstrates that dispute games with different extraData can be created for the same GameType and rootClaim:

function testFuzz_createOnlyExtraDataDifferent_succeeds(
        bytes calldata extraData,
        uint256 _value
    )
        public
    {

        GameType gt = GameType.wrap(0);
        Claim rootClaim = Claim.wrap(bytes32((uint256(1) << 248) | uint256(10)));

        // Set all three implementations to the same `FakeClone` contract.
        for (uint8 i; i < 3; i++) {
            GameType lgt = GameType.wrap(i);
            disputeGameFactory.setImplementation(lgt, IDisputeGame(address(fakeClone)));
            disputeGameFactory.setInitBond(lgt, _value);
        }

        vm.deal(address(this), _value);

        vm.expectEmit(false, true, true, false);
        emit DisputeGameCreated(address(0), gt, rootClaim);
        IDisputeGame proxy = disputeGameFactory.create{ value: _value }(gt, rootClaim, extraData);

        (IDisputeGame game, Timestamp timestamp) = disputeGameFactory.games(gt, rootClaim, extraData);

        // Ensure that the dispute game was assigned to the `disputeGames` mapping.
        assertEq(address(game), address(proxy));
        assertEq(Timestamp.unwrap(timestamp), block.timestamp);
        assertEq(disputeGameFactory.gameCount(), 1);

        (, Timestamp timestamp2, IDisputeGame game2) = disputeGameFactory.gameAtIndex(0);
        assertEq(address(game2), address(proxy));
        assertEq(Timestamp.unwrap(timestamp2), block.timestamp);

        // Ensure that the game proxy received the bonded ETH.
        assertEq(address(proxy).balance, _value);
    }

Tool used

Manual Review

Recommendation

It is obvious that the most important variable, which directly affects the resolution and is even the cornerstone of dispute games, is 'rootClaim' and that 'GameType' is absolutely necessary. But it does not seem necessary in practice for there to be two dispute games that have the same claim and want to achieve the same result, but only their extraData differs. Although it is not a problem for extraData to remain, only rootClaim and GameType should be taken as basis in UUID generation.extraData does not affect the result in any way, and even if it does, the fault dispute game should be concluded accordingly, otherwise we will allow a very wide universe of dispute games with the same rootClaim and GameType because of extraData.

        // Compute the unique identifier for the dispute game.
-        Hash uuid = getGameUUID(_gameType, _rootClaim, _extraData);
+        Hash uuid = getGameUUID(_gameType, _rootClaim);

And please remove extraData's association with UUID in other relevant places as well.

Duplicate of #203