Creation of multiple FaultDisputeGames for same output root and l2BlockNumber possible

Summary

It is possible to create multiple FaultDisputeGames for the same output proposal and L2BlockNumber pair, even though there is a check meant to prevent that. Under certain assumptions this can cause an incorrectly resolved game to remain unnoticed by the guardian.

Vulnerability Detail

In FaultDisputeGame.initialize there is this check to prevent against the creation of multiple dispute games for the same output proposal:

// 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)
assembly {
    if gt(calldatasize(), 0x66) {

However, this check only checks that calldata is not greater than the expected length. It is still possible to create a FaultDisputeGame with extraData that is shorter than 0x20 bytes but will still yield the same result for the l2BlockNumber call as a game that was created with correct length of extra data.

Example:

l2BlockNumber is 0x[61 zeroes]100
then we can create 2 games for the same root claim and the same l2Blocknumber but different extraData
first game has the full l2BlockNumber as extraData
second game is created with extraData = 0x[61 zeroes]1 (omitting the last byte)
the call to l2BlockNumber will still yield 0x[61 zeroes]100 because the last 2 bytes appended to the calldata will be 0x0061 (the CWIA length)

A minimal POC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import { Clone } from "../src/Clone.sol";
import { ClonesWithImmutableArgs } from "../src/ClonesWithImmutableArgs.sol";

contract TestClone is Clone {
    function getArgDynBytes() public returns (bytes memory) {
        uint256 calldataSize;
        assembly {
            calldataSize := calldatasize()
        }
        console.logUint(calldataSize);

        return _getArgDynBytes(0x40, 0x30);
    }
}

contract TestCloneFactory {
    using ClonesWithImmutableArgs for address;

    TestClone public implementation;

    constructor(TestClone implementation_) {
        implementation = implementation_;
    }

    function createClone(
        bytes32 param1,
        bytes32 param2,
        bytes calldata param3
    ) external returns (TestClone clone) {
        bytes memory data = abi.encodePacked(param1, param2, param3);
        clone = TestClone(address(implementation).clone(data));
    }
}

//@note clones with immutable args
contract CWIATest is Test {

    function testCWIA() public{
        TestClone implementation = new TestClone();
        TestCloneFactory factory = new TestCloneFactory(implementation);
        //@note only 31 bytes as third patam
        TestClone clone = factory.createClone(keccak256("param1"), keccak256("param2"), hex"65004300000000000000000000000000000000000000000000000000000111");
        //@note argDynBytes modified to return 0x30 bytes instead of 0x20 to show the full data
        bytes memory argDynBytes = clone.getArgDynBytes();
        // prints 0x650043000000000000000000000000000000000000000000000000000001110061000000000000000000000000000000
            // where 0x0061 is the length of the extraData
        console.logBytes(argDynBytes);
    }
}

Impact

Having multiple FaultDisputeGames is an unwanted state that has been documented in the code and checked against. Furthermore, given the right conditions it can lead to incorrect behavior of crucial off-chain components such as the guardian.

An example of a scenario with the right conditions: Attacker creates 2 games for the same output root and same effective l2BlockNumber (but different extraData, else creation would be not possible). Both resolve incorrectly. If the guardians off-chain monitoring program were to track the games as pairs of (output root, l2BlockNumber) then he could only track one of the games and would miss the other.

While this is a quite hypothetical scenario it would cause a high impact and is easily mitigated against (see below).

Code Snippet

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

Tool used

Manual Review

Recommendation

Check for unequality of length rather than a greater-than-relation:

if iszero(eq(calldatasize(), 0x66)) {

Duplicate of #203

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

99Crits - Creation of multiple FaultDisputeGames for same output root and l2BlockNumber possible #221