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

6 stars 4 forks source link

nuthan2x - Users cannot create a new game or proposal if the old game is blacklisted #178

Closed sherlock-admin4 closed 7 months ago

sherlock-admin4 commented 7 months ago

nuthan2x

high

Users cannot create a new game or proposal if the old game is blacklisted

Summary

Users cannot create a new game or proposal if the old game is blacklisted. The revert happens on this line as both old and new games have the same UUID

Vulnerability Detail

If a game is blacklisted, the UUID will remain the same because _gameType, _rootClaim, _extraData are still the same. So when creating a new proposal or game, the same UUID from the old game will be used.

From DisputeGameFactory::create

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

From FaultDisputeGame::initialize 


    // 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)

From the comments and revert lines shown below, we can understand that the protocol allows users to create a new game or proposal if the old game is blacklisted. So, if a user tries to call DisputeGameFactory::create to prove the withdrawal again, the revert line triggers because the uuid is the same and it points to the gameID of the previous game and it is != bytes32(0)

So the revert triggers, and users can never prove or resolve that withdrawal transaction. And a new withdrawal transaction with different nonce has to be created, and again, 7 days of challenging period awaits.  

From OptimismPortal2::proveWithdrawalTransaction

        // We generally want to prevent users from proving the same withdrawal multiple times
        // because each successive proof will update the timestamp. A malicious user can take
        // advantage of this to prevent other users from finalizing their withdrawal. However,
        // in the case that an honest user proves their withdrawal against a dispute game that
@>      // resolves against the root claim, or the dispute game is blacklisted, we allow
        // re-proving the withdrawal against a new proposal.
        IDisputeGame oldGame = provenWithdrawal.disputeGameProxy;
        require(
            provenWithdrawal.timestamp == 0 || oldGame.status() == GameStatus.CHALLENGER_WINS
                || disputeGameBlacklist[oldGame] || oldGame.gameType().raw() != respectedGameType.raw(),
            "OptimismPortal: withdrawal hash has already been proven, and the old dispute game is not invalid"
        );

From DisputeGameFactory::create

        // If a dispute game with the same UUID already exists, revert.
        if (GameId.unwrap(_disputeGames[uuid]) != bytes32(0)) revert GameAlreadyExists(uuid); 

Impact

Users can never prove or resolve the game of a withdrawal transaction if the old game with the same uuid is blacklisted. And a new withdrawal transaction with different nonce has to be created, and again, 7 days of challenging period awaits. The invariant or feature is missing here. That is the issue.

Code Snippet

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

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L280-L291

Tool used

Manual Review

Recommendation

Before reverting here check if the game with the same UUID is blacklisted or not. Skip the revert if the game is blacklisted.

nevillehuang commented 7 months ago

Invalid, as noted in the submission, this creation of dispute game with the same UUID is not possible due to this check. So when proving a withdrawal, the oldgame and new game created will always be unique. This issue misses the root cause presented in #203