Malicios user can put unlimited number of duplicate entries into a proofSubmitters array when a dispute game is blacklisted

Summary

The proveWithdrawalTransaction() is supposed to only allow a transaction to be proven once, this is because whenever a transaction is proven in proveWithdrawalTransaction(). Whenever a withdrawal transaction is proven in proveWithdrawalTransaction() an entry is created in the proofSubmitters[withdrawalHash] array. The proofSubmitters[withdrawalHash] array is supposed to contain the addresses of users that have submitted a valid proof for the withdrawal, as such it is expected that the entries in this array are unique.

However, when a withdrawal is proven successfuly for a dispute game and then this game is blacklisted, a flaw in the logic allows a malicios user to re-prove the withdrawal any number of times and thus generate a large number of entries into the proofSubmitters[withdrawalHash] array. This is possible because the proveWithdrawalTransaction() function does not check if the dispute game is blacklisted, and also it allows a user to re-prove a withdrawal whenever the last dispute game was blacklisted.

This unlimited re-proving can only be stopped by a user proving their withdrawal with a dispute game that is not blacklisted.

Furthermore, a single duplicate entry will be created whenever an honest user re-proves his withdrawal

Vulnerability Detail

The vulnerability is caused by a check in the proveWithdrawalTransaction() function. This check is there as a utility to let users reprove their withdrawal transaction whenever the dispute game is blacklisted. Specifically:

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

        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"
        );

Under normal circuimstances this function reverts whenever the timestamp for a normal withdrawal is non-zero, meaning it was already proven. It has additional checks that allow re-proving of a withdrawal in some special cases.

As we can see, one of these special cases is that the require statement evaluates to true if the old dispute game has been blacklisted. Under normal circuimstances an honest user would now try to reprove his withdrawal against a valid dispute game. However a malicios user monitoring the situation can make a call to proveWithdrawalTransaction() to re-prove the same withdrawal using the old blacklisted dispute game. Since there is no check if the dispute game is blacklisted during the execution of the function it won't revert, and since the old game is blacklisted, the above mentioned require() statement evaluates to true.

To further understand the vulnerability, let's look at how the oldGame variable in the above code snippet will evaluate to the old blacklisted game every time. The proven withdrawal is fetched in the same function and used for the checks:

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

    ProvenWithdrawal memory provenWithdrawal = provenWithdrawals[withdrawalHash][msg.sender];

Then, after the validation and proof is done, it is set with a new non-zero timestamp and the dispute game used for the proving:

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

    provenWithdrawals[withdrawalHash][msg.sender] =
        ProvenWithdrawal({ disputeGameProxy: gameProxy, timestamp: uint64(block.timestamp) });

Thus, when a malicios user re-proves a valid withdrawal with a blacklisted dispute game will have the provenWithdrawals[withdrawalHash][msg.sender] set to the old blacklisted game and a non-zero timestamp, which will pass the oldGame checks in the require() statement.

Finally, the a msg.sender entry will be pushed to the proofSubmitters[withdrawalHash] array:

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

    proofSubmitters[withdrawalHash].push(msg.sender);

If abused, this will cause the array for a particular withdrawalHash to become very large. This can cause external contracts that rely on this function to DoS due to going out of gas or other bugs.

Impact

The proofSubmitters arrays can get an abnormally large number of entries. This will also cause the numProofSubmitters() function to return abnormally large integers. This does not lead to direct loss of funds, but can lead to bugs in external contracts that rely on the validity/size of these variables.

Code Snippet

The numProofSubmitters() function which will return an abnormally large number.

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

    function numProofSubmitters(bytes32 _withdrawalHash) external view returns (uint256) {
        return proofSubmitters[_withdrawalHash].length;
    }

Tool used

Manual Review

Recommendation

Take precautions to prevent duplicate entries. If a proofSubmitters[withdrawalHash] array is expected to have duplicate entries, then the mapping name/documentation must be adjusted, same goes for numProofSubmitters(), as it will mislead developers and can lead to bugs in external contracts/integrations.

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

gesha17 - Malicios user can put unlimited number of duplicate entries into a proofSubmitters array when a dispute game is blacklisted #126