fix: have DelayedWETH use call instead of transfer (ethereum-optimism…

…#228)

Updates DelayedWETH to use call instead of transfer because
transfer only sends along 2300 gas which will cause the transfer
to fail if the owner contract uses lots of gas in the fallback
function. Adds tests that demonstrate that the function now works
appropriately under any reasonable amount of gas usage. In theory
this means the recovery function can be reentered but this isn't
an issue because the function is authenticated and is recovering
all of the ETH in the contract anyway.

packages/contracts-bedrock/semver-lock.json

@@ -164,8 +164,8 @@
     "sourceCodeHash": "0x769983913a4228c34475cb52286c0bc380495b3be9e401bf46eae3b32286f560"
   },
   "src/dispute/weth/DelayedWETH.sol": {
-    "initCodeHash": "0xb9bbe005874922cd8f499e7a0a092967cfca03e012c1e41912b0c77481c71777",
-    "sourceCodeHash": "0x87d00995773d34cc28e81559f4cc5f25890d924df285ec6e9e01b5277f52a9dc"
+    "initCodeHash": "0x8f9a5b50374331ad2fabe03a7ce28a0012bfaca5fa48ee917339c3eec39a319f",
+    "sourceCodeHash": "0x22cbcab45d46c3746e0ef6a7c39192ce85a21d0cabf055c2cc21796714f5c47f"
   },
   "src/legacy/DeployerWhitelist.sol": {
     "initCodeHash": "0x8de80fb23b26dd9d849f6328e56ea7c173cd9e9ce1f05c9beea559d1720deb3d",

packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol

@@ -85,7 +85,8 @@ contract DelayedWETH is OwnableUpgradeable, WETH98, IDelayedWETH, ISemver {
     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);
+        (bool success,) = payable(msg.sender).call{ value: amount }(hex"");
+        require(success, "DelayedWETH: recover failed");
     }
 
     /// @inheritdoc IDelayedWETH

packages/contracts-bedrock/test/dispute/DelayedWETH.t.sol

@@ -9,6 +9,7 @@ import { DisputeGameFactory, IDisputeGameFactory } from "src/dispute/DisputeGame
 import { IDisputeGame } from "src/dispute/interfaces/IDisputeGame.sol";
 import { DelayedWETH } from "src/dispute/weth/DelayedWETH.sol";
 import { Proxy } from "src/universal/Proxy.sol";
+import { Burn } from "src/libraries/Burn.sol";
 import { CommonTest } from "test/setup/CommonTest.sol";
 
 contract DelayedWETH_Init is CommonTest {
@@ -45,7 +46,7 @@ contract DelayedWETH_Unlock_Test is DelayedWETH_Init {
         assertEq(timestamp, block.timestamp);
     }
 
-    /// @dev TEsts that unlocking twice is successful and timestamp/amount is updated.
+    /// @dev Tests that unlocking twice is successful and timestamp/amount is updated.
     function test_unlock_twice_succeeds() public {
         // Unlock once.
         uint256 ts = block.timestamp;
@@ -170,24 +171,35 @@ contract DelayedWETH_Withdraw_Test is DelayedWETH_Init {
 }
 
 contract DelayedWETH_Recover_Test is DelayedWETH_Init {
-    /// @dev Tests that recovering WETH succeeds.
-    function test_recover_succeeds() public {
+    /// @dev Tests that recovering WETH succeeds. Makes sure that doing so succeeds with any amount
+    ///      of ETH in the contract and any amount of gas used in the fallback function up to a
+    ///      maximum of 20,000,000 gas. Owner contract should never be using that much gas but we
+    ///      might as well set a very large upper bound for ourselves.
+    /// @param _amount Amount of WETH to recover.
+    /// @param _fallbackGasUsage Amount of gas to use in the fallback function.
+    function testFuzz_recover_succeeds(uint256 _amount, uint256 _fallbackGasUsage) public {
+        // Assume
+        _fallbackGasUsage = bound(_fallbackGasUsage, 0, 20000000);
+
+        // Set up the gas burner.
+        FallbackGasUser gasUser = new FallbackGasUser(_fallbackGasUsage);
+
         // Transfer ownership to alice.
-        delayedWeth.transferOwnership(alice);
+        delayedWeth.transferOwnership(address(gasUser));
 
         // Give the contract some WETH to recover.
-        vm.deal(address(delayedWeth), 1 ether);
+        vm.deal(address(delayedWeth), _amount);
 
         // Record the initial balance.
-        uint256 initialBalance = address(alice).balance;
+        uint256 initialBalance = address(gasUser).balance;
 
         // Recover the WETH.
-        vm.prank(alice);
-        delayedWeth.recover(1 ether);
+        vm.prank(address(gasUser));
+        delayedWeth.recover(_amount);
 
         // Verify the WETH was recovered.
         assertEq(address(delayedWeth).balance, 0);
-        assertEq(address(alice).balance, initialBalance + 1 ether);
+        assertEq(address(gasUser).balance, initialBalance + _amount);
     }
 
     /// @dev Tests that recovering WETH by non-owner fails.
@@ -219,6 +231,23 @@ contract DelayedWETH_Recover_Test is DelayedWETH_Init {
         assertEq(address(delayedWeth).balance, 0);
         assertEq(address(alice).balance, initialBalance + 0.5 ether);
     }
+
+    /// @dev Tests that recover reverts when recipient reverts.
+    function test_recover_whenRecipientReverts_fails() public {
+        // Set up the reverter.
+        FallbackReverter reverter = new FallbackReverter();
+
+        // Transfer ownership to the reverter.
+        delayedWeth.transferOwnership(address(reverter));
+
+        // Give the contract some WETH to recover.
+        vm.deal(address(delayedWeth), 1 ether);
+
+        // Recover fails.
+        vm.expectRevert("DelayedWETH: recover failed");
+        vm.prank(address(reverter));
+        delayedWeth.recover(1 ether);
+    }
 }
 
 contract DelayedWETH_Hold_Test is DelayedWETH_Init {
@@ -255,3 +284,39 @@ contract DelayedWETH_Hold_Test is DelayedWETH_Init {
         delayedWeth.hold(bob, 1 ether);
     }
 }
+
+/// @title FallbackGasUser
+/// @notice Contract that burns gas in the fallback function.
+contract FallbackGasUser {
+    /// @notice Amount of gas to use in the fallback function.
+    uint256 public gas;
+
+    /// @param _gas Amount of gas to use in the fallback function.
+    constructor(uint256 _gas) {
+        gas = _gas;
+    }
+
+    /// @notice Burn gas on fallback;
+    fallback() external payable {
+        Burn.gas(gas);
+    }
+
+    /// @notice Burn gas on receive.
+    receive() external payable {
+        Burn.gas(gas);
+    }
+}
+
+/// @title FallbackReverter
+/// @notice Contract that reverts in the fallback function.
+contract FallbackReverter {
+    /// @notice Revert on fallback.
+    fallback() external payable {
+        revert("FallbackReverter: revert");
+    }
+
+    /// @notice Revert on receive.
+    receive() external payable {
+        revert("FallbackReverter: revert");
+    }
+}

packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummary.sol

@@ -15,7 +15,7 @@ contract DeploymentSummary is DeploymentSummaryCode {
     address internal constant addressManagerAddress = 0x50EEf481cae4250d252Ae577A09bF514f224C6C4;
     address internal constant anchorStateRegistryAddress = 0xE2a80256d1dAFe06683F231F8e9561639Aa0e9b9;
     address internal constant anchorStateRegistryProxyAddress = 0xd6EAF4c146261653EE059077B78ED088Add54309;
-    address internal constant delayedWETHAddress = 0x49BBFf1629824A1e7993Ab5c17AFa45D24AB28c9;
+    address internal constant delayedWETHAddress = 0x90b505357aFad15A1fb8F1098B3295b7cfac1c24;
     address internal constant delayedWETHProxyAddress = 0xEF179756ea6525AFade217cA5aB0b1b5CfE0fd92;
     address internal constant disputeGameFactoryAddress = 0x20B168142354Cee65a32f6D8cf3033E592299765;
     address internal constant disputeGameFactoryProxyAddress = 0x5207CfA0166E8de0FCdFd78B4d17b68587bE306d;
@@ -698,7 +698,7 @@ contract DeploymentSummary is DeploymentSummaryCode {
         value = hex"000000000000000000000000000000000000000000000000000000000000000e";
         vm.store(systemOwnerSafeAddress, slot, value);
         slot = hex"360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc";
-        value = hex"00000000000000000000000049bbff1629824a1e7993ab5c17afa45d24ab28c9";
+        value = hex"00000000000000000000000090b505357afad15a1fb8f1098b3295b7cfac1c24";
         vm.store(delayedWETHProxyAddress, slot, value);
         slot = hex"0000000000000000000000000000000000000000000000000000000000000000";
         value = hex"0000000000000000000000000000000000000000000000000000000000000001";

packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummaryFaultProofs.sol

@@ -15,7 +15,7 @@ contract DeploymentSummaryFaultProofs is DeploymentSummaryFaultProofsCode {
     address internal constant addressManagerAddress = 0x50EEf481cae4250d252Ae577A09bF514f224C6C4;
     address internal constant anchorStateRegistryAddress = 0xE2a80256d1dAFe06683F231F8e9561639Aa0e9b9;
     address internal constant anchorStateRegistryProxyAddress = 0xd6EAF4c146261653EE059077B78ED088Add54309;
-    address internal constant delayedWETHAddress = 0x49BBFf1629824A1e7993Ab5c17AFa45D24AB28c9;
+    address internal constant delayedWETHAddress = 0x90b505357aFad15A1fb8F1098B3295b7cfac1c24;
     address internal constant delayedWETHProxyAddress = 0xEF179756ea6525AFade217cA5aB0b1b5CfE0fd92;
     address internal constant disputeGameFactoryAddress = 0x20B168142354Cee65a32f6D8cf3033E592299765;
     address internal constant disputeGameFactoryProxyAddress = 0x5207CfA0166E8de0FCdFd78B4d17b68587bE306d;
@@ -701,7 +701,7 @@ contract DeploymentSummaryFaultProofs is DeploymentSummaryFaultProofsCode {
         value = hex"000000000000000000000000000000000000000000000000000000000000000e";
         vm.store(systemOwnerSafeAddress, slot, value);
         slot = hex"360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc";
-        value = hex"00000000000000000000000049bbff1629824a1e7993ab5c17afa45d24ab28c9";
+        value = hex"00000000000000000000000090b505357afad15a1fb8f1098b3295b7cfac1c24";
         vm.store(delayedWETHProxyAddress, slot, value);
         slot = hex"0000000000000000000000000000000000000000000000000000000000000000";
         value = hex"0000000000000000000000000000000000000000000000000000000000000001";