Spoofing multiple dispute games for same proposal due to one sided calldata length check

Summary

The FaultDisputeGame's initialize function intends to prevent multiple dispute games for the same output proposal from being created, but the check is insufficient. Calldata that is shorter than 0x66 bytes can be used to spoof additional games for some L2 block numbers, potentially leading to malicious withdrawals and denial-of-service attacks.

Vulnerability Detail

The initialize function includes an assembly block that reverts if the calldatasize() is longer than 0x66 bytes. The documented intent of this check is to prevent the creation of multiple games for the same proposal. However, if the calldata is shorter than 0x66 bytes, it can still allow for multiple dispute games to be created for the same output proposal, undermining the validation check.

This is because extraData can be shorter than 0x20 bytes in some cases, and still result in the same game contract behavior. Specifically, if the l2BlockNumber's hex representation (which is passed in extraData) ends with bytes like 0x..00, 0x..0060, 0x..5f0000, 0x..5e000000, 0x..5d00000000, and so on, it allows for extraData to be shorter than 0x20 bytes.

When _getArgUint256 is called to load extraData, it uses calldataload to load 0x20 bytes regardless of the actual length of extraData passed to the function. If extraData is shorter than 0x20 bytes, the loaded data will include bytes from the 2 CWIA length bytes at the end of the calldata, followed by zeros loaded by calldataload for out of bounds range.

For example, suppose the actual l2BlockNumber is 0x..0100, and extraData is initially passed as 0x..01 with a length of 31 bytes instead of the expected 32 bytes. Due to the 2 CWIA bytes at the end of the calldata (0x0061, which is 1 less than the usual expected 0x0062), the _getArgUint256 function will still load the correct value of 0x..0100 (last byte coming from the CWIA bytes) for extraData. As a result, the game contract will use the same value internally, but the factory will have used a different value to initially hash extraData.

Consequently, the factory will allow the creation of these games because the UUID will be different due to the different encoded extraData, even though the game contract's behavior remains the same. Other than the UUID used internally and that is neither emitted not accessible via views, the resulting dispute games will be indistinguishable by their external behavior.

Impact

The vulnerability violates the core assumption of having only a single unique game per proposal, leading to several severe consequences:

Proving forged withdrawals (loss of funds): By proposing the same malicious proposal twice with spoofed games, an attacker can expect that only one of the games will be countered by fault provers. This is because off-chain monitoring systems will detect the malicious rootClaim (that was emitted in DisputeGameCreated), and use findLatestGames to scan for matching games. Upon finding the matching game (the spoofed game), they will counter it. When the other game, for the same claim, will remain uncountered (masked by the spoofed game) and resolve in favor of the attacker, they will proceed with a malicious withdrawal, effectively stealing funds from the bridge.
Valid proposal spoofing (DoS): A valid proposal can be spoofed by backrunning an honest proposal. Bridge front-ends are likely to use findLatestGames to determine the correct game to prove a user's withdrawal against. This will cause users or bridge front-ends to prove withdrawals against the wrong game, neglecting to prove against the correctly created game. As the spoofed game will not be resolvable and require blacklisting (as is currently the case), users will need to re-prove and wait a second duration of a proof delay (7 days). By repeatedly creating spoofed games, an attacker can cause significant disruptions to the withdrawal process.
It breaks the intended core invariant of having only a single game per proposal that the calldata length check is trying to enforce.
Off-chain systems disruption: monitoring systems and front-ends will rely on findLatestGames to identify the correct dispute game for a given proposal. However, spoofed games will be returned alongside genuine games, with identical extraData and other parameters. This can lead to incorrect processing of challenges and withdrawal proofs.

Likelihood

The vulnerability can be exploited for a significant number of L2 blocks:

At least one in every 256 blocks can be spoofed since the block number will end in 0x..00.
For some blocks, multiple spoofed games can be created. For example, a block number ending in 0x..5f0000 also ends in 0x..00, allowing for the creation of two spoofed games.

Code Snippet

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

POC

// test added into FaultDisputeGame.t.sol

    function test_initialize_extraDataTooShort() public {
        Claim claim = _dummyClaim();

        uint l2block = 0x0100;

        bytes memory _extraData1 = new bytes(32);
        _extraData1[30] = 0x01; // set the 31st byte (one before last)
        FaultDisputeGame gameProxy1 = FaultDisputeGame(payable(address(disputeGameFactory.create(GAME_TYPE, claim, _extraData1))));
        // check l2 block number is as expected
        assertEq(gameProxy1.l2BlockNumber(), l2block);

        // try again, should fail, because should not be able to create same game
        vm.expectRevert(abi.encodeWithSelector(GameAlreadyExists.selector, 0x233fa717c43db2587f80a392c9cab043e3a51e7fd93b1b1dc03978d2d59ebfcc));
        FaultDisputeGame(payable(address(disputeGameFactory.create(GAME_TYPE, claim, _extraData1))));

        // now spoof with shorter extraData
        bytes memory _extraData2 = new bytes(31); // 31 instead of 32
        _extraData2[30] = 0x01; // set last byte to 0x01

        // this succeeds and creates a spoofed identical game
        FaultDisputeGame gameProxy2 = FaultDisputeGame(payable(address(disputeGameFactory.create(GAME_TYPE, claim, _extraData2))));
        // check l2 number is as expected
        assertEq(gameProxy2.l2BlockNumber(), l2block);
        // and same as first game
        assertEq(gameProxy2.l2BlockNumber(), gameProxy1.l2BlockNumber());
        // and extra data is also returned as "the same"
        assertEq(gameProxy2.extraData(), gameProxy1.extraData());
    }

Tool used

Manual Review

Recommendation

To fix this vulnerability, the initialize function should restrict the calldata length to the exact expected length. Additionally, Solidity's msg.data.length can be used to check the calldata length instead of using assembly.

-assembly {
-   if gt(calldatasize(), 0x66) {
-       // Store the selector for `ExtraDataTooLong()` & revert
-       mstore(0x00, 0xc407e025)
-       revert(0x1C, 0x04)
-   }
+if (msg.data.length != 0x66) revert ExtraDataLengthError();
}

Duplicate of #203

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

guhu95 - Spoofing multiple dispute games for same proposal due to one sided calldata length check #139