FastTiger - Incorrect check for withdrawal delay period in function `DelayedWETH.sol#withdraw()`

[sherlock-admin2]

FastTiger

high

Incorrect check for withdrawal delay period in function DelayedWETH.sol#withdraw()

Summary

If the game is resolved incorrectly due to an incorrect check on the withdrawal timestamp of the DelayedWETH.sol#withdraw() function, a loss of funds may occur when the DelayedWETH.sol#recover() function is performed.

Vulnerability Detail

The DelayedWETH.sol#recover() function is used to allows the owner to recover from error cases by pulling ETH out of the contract. The owner would likely place the WETH into another contract to be distributed properly, or the owner could distribute it correctly manually to prevent the game contract from being able to withdraw funds when it resolves incorrectly.

    function recover(uint256 _wad) external {
        require(msg.sender == owner(), "DelayedWETH: not owner");
        uint256 amount = _wad < address(this).balance ? _wad : address(this).balance;
        payable(msg.sender).transfer(amount);
    }

However, funds may be withdrawn incorrectly due to incorrect confirmation of the withdrawal time period in the DelayedWETH.sol#withdraw() function. The DelayedWETH.sol#withdraw() function is as follows.

    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");
79:     require(wd.timestamp + DELAY_SECONDS <= block.timestamp, "DelayedWETH: withdrawal delay not met");
        wd.amount -= _wad;
        super.withdraw(_wad);
    }

As you can see, the withdrawal time is determined by wd.timestamp, which is the time when the funds were unlocked.

    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];
64:     wd.timestamp = block.timestamp;
        wd.amount += _wad;
    }

Therefore, funds may be withdrawn before recover() is performed. As a result, if the game is solved incorrectly, incorrectly distributed funds may be withdrawn, resulting in loss of funds.

Impact

If the game is solved incorrectly, incorrectly distributed funds may be withdrawn

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol#L74 https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol#L57 https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol#L57

Tool used

Manual Review

Recommendation

It is a good idea to base your withdrawal timing on when the game is processed, not when the funds are unlocked. Even so, the protocol's purpose of technically distributing bonds while the game is in progress will be accurately carried out.

[nevillehuang]

Invalid, admin action to recover WETH is not dependent on withdrawal timestamp