lemonmon - A duplicate dispute game may be created for the same output proposal due to an issue inside `FaultDisputeGame.initialize()`

[sherlock-admin3]

lemonmon

medium

A duplicate dispute game may be created for the same output proposal due to an issue inside FaultDisputeGame.initialize()

Summary

Due to an issue with a revert condition inside the function FaultDisputeGame.initialize(), a duplicate dispute game for the same output proposal may be created.

Vulnerability Detail

Inside the function FaultDisputeGame.initialize(), the calldatasize() is checked to not be greater than 0x66 (line 546-550 in FaultDisputeGame.sol).

According to the comment on line 541-545 (FaultDisputeGame.sol), this is done to prevent the creation of multiple games for the same output proposal.

However, as shown in this POC test_game_with_same_data() which can be added to DisputeGameFactory.t.sol, a duplicate game for the same output proposal may be created.

POC:

// DisputeGameFactory.t.sol
function test_game_with_same_data() public {
    uint256 _value = disputeGameFactory.initBonds(GAME_TYPE);
    vm.deal(address(this), _value);

    assertEq(address(gameProxy).balance, 0);

    // The first game
    uint256 l2BlockNum = uint(bytes32(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00));
    bytes memory _extraData = abi.encodePacked(l2BlockNum);

    (bool success, bytes memory resData) = address(disputeGameFactory).call{value: _value}(abi.encodeWithSelector(disputeGameFactory.create.selector, GAME_TYPE, ROOT_CLAIM, _extraData));
    assertTrue(success);
    FaultDisputeGame gameProxy1 = FaultDisputeGame(abi.decode(resData, (address)));
    (IDisputeGame proxy1,) = disputeGameFactory.games(GAME_TYPE, ROOT_CLAIM, _extraData);
    assertEq(address(gameProxy1), address(proxy1));

    // The second game
    // using shorter _extraData (1 bytes shorter)
    _extraData = hex"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff";
    (success, resData) = address(disputeGameFactory).call{value: _value}(abi.encodeWithSelector(disputeGameFactory.create.selector, GAME_TYPE, ROOT_CLAIM, _extraData));
    assertTrue(success);
    FaultDisputeGame gameProxy2 = FaultDisputeGame(abi.decode(resData, (address)));
    (IDisputeGame proxy2,) = disputeGameFactory.games(GAME_TYPE, ROOT_CLAIM, _extraData);
    assertEq(address(gameProxy2), address(proxy2));

    assertEq(gameProxy1.rootClaim().raw(), gameProxy2.rootClaim().raw());
    assertEq(gameProxy1.l1Head().raw(), gameProxy2.l1Head().raw());
    assertEq(gameProxy1.l2BlockNumber(), gameProxy2.l2BlockNumber());
}

• In the POC, a game is created with _extraData 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00 by calling the DisputeGameFactory.create() function via the disputeGameFactory.create.selector. (the ff can be anything as long as it matches with the _extraData below)
• Then a second game is created with _extraData ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which is 1 byte shorter than the _extraData from the first game, missing the 00 at the end. The extra data from the second game is identical to the extra data from the first game, except the missing last byte.
• Then as a proof it is shown that rootClaim, l1Head and l2BlockNumber of both games are identical in the last 3 lines of the POC.

The POC shows that a duplicate game can be created for the same output proposal, where the games have the same combination of gametype, rootclaim and l2BlockNumber.

The reason is that when creating a new game with shorter _extraData, the missing byte is "filled" with zeros upon creation.

Impact

1. A malicious proposer can create a duplicate game for the same output proposal at the same l2BlockNumber.
2. Challengers have to challenge both games.

A duplicate game for the same proposal may create confusion and may make it difficult to follow the duplicated game. Also, if automated monitoring is done using properly formed extraData (= l2BlockNumber) only, these monitoring may miss malformed cases like above, resulting in untimely invalidation or even fail to invalidate those games.

There should only be one game per proposal to maintain the integrity and trust in the dispute resolution process. Otherwise this issue could undermine the fairness and reliability of the dispute resolution process.

Also the risk for malicious actors may decrease since they have more than one chance to have their proposal accepted.

Code Snippet

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

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L84-L106

Tool used

Manual Review

Recommendation

Consider checking the call data size to be exactly 0x66, which is the expected length.

// FaultDisputeGame.sol
546        assembly {
+547            if iszero(eq(calldatasize(), 0x66)) {
548                // Store the selector for `ExtraDataTooLong()` & revert
549                mstore(0x00, 0xc407e025)
550                revert(0x1C, 0x04)
551            }
552        }

Duplicate of #203

[sherlock-admin2]

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