The delay set in DelayedWETH is insufficient if resolution is wrong

Summary

The delay in DelayedWETH is insufficient if the game plays until its MAX_DEPTH (or for many claims) as users can withdraw their bonds before the whole game reached a resolved status.

Vulnerability Detail

The point of having a delayed system in DelayedWETH is so that if the resolution of the game is wrong, there is still time for the guardian and owner to change things and not let the bond go into the wrong hands. That is why bonds are first unlocked() before withdrawing() and there is DELAY_SECONDS when withdrawing.

 function unlock(address _guy, uint256 _wad) external {
        // Note that the unlock function can be called by any address, but the actual unlocking capability still only
        // gives the msg.sender the ability to withdraw from the account. As long as the unlock and withdraw functions
        // are called with the proper recipient addresses, this will be safe. Could be made safer by having external
        // accounts execute withdrawals themselves but that would have added extra complexity and made DelayedWETH a
        // leaky abstraction, so we chose this instead.
        WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
>       wd.timestamp = block.timestamp;
        wd.amount += _wad;
    }

    /// @inheritdoc IDelayedWETH
    function withdraw(address _guy, uint256 _wad) public {
        require(!config.paused(), "DelayedWETH: contract is paused");
        WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
        require(wd.amount >= _wad, "DelayedWETH: insufficient unlocked withdrawal");
        require(wd.timestamp > 0, "DelayedWETH: withdrawal not unlocked");
>       require(wd.timestamp + DELAY_SECONDS <= block.timestamp, "DelayedWETH: withdrawal delay not met");
        wd.amount -= _wad;
        super.withdraw(_wad);
    }

The issue is that unlock() starts the withdrawal timestamp and the player can claim credit even before the whole game is resolved since there is no check that the game must reach a final status (challenger/defender wins) before a player can claim their credit.

function claimCredit(address _recipient) external {
        // Remove the credit from the recipient prior to performing the external call.
        uint256 recipientCredit = credit[_recipient];
        credit[_recipient] = 0;

        // Revert if the recipient has no credit to claim.
        if (recipientCredit == 0) {
            revert NoCreditToClaim();
        }

        // Try to withdraw the WETH amount so it can be used here.
        WETH.withdraw(_recipient, recipientCredit);

        // Transfer the credit to the recipient.
        (bool success,) = _recipient.call{ value: recipientCredit }(hex"");
        if (!success) revert BondTransferFailed();
    }

resolveClaim() is the function that invokes _distributeBonds() which starts the unlocking period for the player, to which they can call claimCredit() when the unlocking period ends.

Imagine a game with MAX_DEPTH 73, and about 200 claims in total. Alice is the one that created the 200th claim. As she is the last claim, her claim must be resolved first before other claims can be resolved.

Alice calls resolveClaim() with her claimIndex and _distributeBonds() get invoked, starting her unlocking period.

Since there are so many claims beforehand, it will take awhile for all the claims to be resolved and for resolve() to be finally called. It is also imperative to note that players will unlikely help other players resolve their claim since they will have to pay the gas fee for them to call resolveClaim(), which is a reason why resolving will take a longer period of time.

Before resolve() is called, if the time has passed DELAY_SECONDS, Alice can withdraw her funds, which makes it unfair to those people who are waiting for their unlocking period. Also, if the game is resolved wrongly, Alice has already retrieved her funds, which makes the DELAY_SECONDS check insufficient.

Impact

It will be unfair for other players as the unlocking period is different for everyone. Players can claim before the game is resolved, which, if resolved incorrectly, will cause accounting issues.

Code Snippet

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

Tool used

Manual Review

Recommendation

Recommend setting the delay time to be the same for everybody, and allow claiming only after the game has been resolved. This can be done by having a check in the claimCredit() function and having a uniform unlock period that starts right after resolved() is called.

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

peanuts - The delay set in DelayedWETH is insufficient if resolution is wrong #132