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

6 stars 4 forks source link

Use of `Payable(msg.sender).transfer` in `WETH98.withdraw` may revert and cause problems with withdrawal #234

Closed sherlock-admin3 closed 7 months ago

sherlock-admin3 commented 7 months ago

Use of Payable(msg.sender).transfer in WETH98.withdraw may revert and cause problems with withdrawal

Low/Info issue submitted by forgebyola

Summary

The use of payable(msg.sender).transfer method of sending ETH to a recipient can cause DOS to that user and call withdrawal may revert.

Vulnerability Details

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

Impact

A user's attempt to withdraw from the WETH98 or DELAYEDWETH contracts may fail due to the use of payable(msg.sender).transfer. This is especially true if the caller is a smart contract or a user with a smart account wallet.

Code Snippet

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

Proof of Concept

For lack of time I would not spend time discussing a proof as I believe this is straightforward.

Tool used

Manual Review

Recommendation

  1. It is generally recommended and best practice to use the builtin call functionality when transferring ETH to a recipient. This is actually as correctly implemented in the FaultDisputeGame.claimCredits function
function withdraw(uint256 wad) public virtual {
        require(balanceOf[msg.sender] >= wad);
        balanceOf[msg.sender] -= wad;
-       payable(msg.sender).transfer(wad);
+       (bool success, ) = msg.sender.call{value: wad}();
+       if(!success) {revert()};
        emit Withdrawal(msg.sender, wad);
    }