chore(protocol): avoid copying any data when send Ether (#16361)

taikoxyz · Mar 10, 2024 · 627bf01 · 627bf01 · engr0001 · Mar 11, 2024
1 parent 0b1385e
commit 627bf01
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 105 deletions.
diff --git a/packages/protocol/contracts/L1/hooks/AssignmentHook.sol b/packages/protocol/contracts/L1/hooks/AssignmentHook.sol
@@ -31,6 +31,8 @@ contract AssignmentHook is EssentialContract, IHook {
         uint256 tip; // A tip to L1 block builder
     }
 
+    event EtherPaymentFailed(address to, uint256 maxGas);
+
     /// @notice Max gas paying the prover.
     /// @dev This should be large enough to prevent the worst cases for the prover.
     /// To assure a trustless relationship between the proposer and the prover it's
@@ -112,7 +114,9 @@ contract AssignmentHook is EssentialContract, IHook {
         // Ether or ERC20 tokens.
         if (assignment.feeToken == address(0)) {
             // Paying Ether
-            _blk.assignedProver.sendEther(proverFee, MAX_GAS_PAYING_PROVER);
+            // Note that this payment may fail if it cost more gas
+            bool success = _blk.assignedProver.sendEther(proverFee, MAX_GAS_PAYING_PROVER, "");
+            if (!success) emit EtherPaymentFailed(_blk.assignedProver, MAX_GAS_PAYING_PROVER);
         } else {
             // Paying ERC20 tokens
             IERC20(assignment.feeToken).safeTransferFrom(
@@ -122,12 +126,12 @@ contract AssignmentHook is EssentialContract, IHook {
 
         // block.coinbase can be address(0) in tests
         if (input.tip != 0 && block.coinbase != address(0)) {
-            address(block.coinbase).sendEther(input.tip);
+            address(block.coinbase).sendEtherAndVerify(input.tip);
         }
 
         // Send all remaining Ether back to TaikoL1 contract
         if (address(this).balance > 0) {
-            taikoL1Address.sendEther(address(this).balance);
+            taikoL1Address.sendEtherAndVerify(address(this).balance);
         }
 
         emit BlockAssigned(_blk.assignedProver, _meta, assignment);

diff --git a/packages/protocol/contracts/L1/libs/LibDepositing.sol b/packages/protocol/contracts/L1/libs/LibDepositing.sol
@@ -38,7 +38,7 @@ library LibDepositing {
             revert L1_INVALID_ETH_DEPOSIT();
         }
 
-        _resolver.resolve("bridge", false).sendEther(msg.value);
+        _resolver.resolve("bridge", false).sendEtherAndVerify(msg.value);
 
         // Append the deposit to the queue.
         address recipient_ = _recipient == address(0) ? msg.sender : _recipient;

diff --git a/packages/protocol/contracts/L1/libs/LibProposing.sol b/packages/protocol/contracts/L1/libs/LibProposing.sol
@@ -259,7 +259,7 @@ library LibProposing {
             }
             // Refund Ether
             if (address(this).balance != 0) {
-                msg.sender.sendEther(address(this).balance);
+                msg.sender.sendEtherAndVerify(address(this).balance);
             }
 
             // Check that after hooks, the Taiko Token balance of this contract

diff --git a/packages/protocol/contracts/L2/TaikoL2.sol b/packages/protocol/contracts/L2/TaikoL2.sol
@@ -171,7 +171,7 @@ contract TaikoL2 is CrossChainOwned {
     {
         if (_to == address(0)) revert L2_INVALID_PARAM();
         if (_token == address(0)) {
-            _to.sendEther(address(this).balance);
+            _to.sendEtherAndVerify(address(this).balance);
         } else {
             IERC20(_token).safeTransfer(_to, IERC20(_token).balanceOf(address(this)));
         }

diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol
@@ -5,7 +5,6 @@ import "@openzeppelin/contracts/utils/Address.sol";
 import "../common/EssentialContract.sol";
 import "../libs/LibAddress.sol";
 import "../signal/ISignalService.sol";
-import "../thirdparty/nomad-xyz/ExcessivelySafeCall.sol";
 import "./IBridge.sol";
 
 /// @title Bridge
@@ -203,7 +202,7 @@ contract Bridge is EssentialContract, IBridge {
                 // Must reset the context after the message call
                 _resetContext();
             } else {
-                _message.srcOwner.sendEther(_message.value);
+                _message.srcOwner.sendEtherAndVerify(_message.value);
             }
             emit MessageRecalled(msgHash);
         } else if (!isMessageProven) {
@@ -292,11 +291,11 @@ contract Bridge is EssentialContract, IBridge {
 
             // Refund the processing fee
             if (msg.sender == refundTo) {
-                refundTo.sendEther(_message.fee + refundAmount);
+                refundTo.sendEtherAndVerify(_message.fee + refundAmount);
             } else {
                 // If sender is another address, reward it and refund the rest
-                msg.sender.sendEther(_message.fee);
-                refundTo.sendEther(refundAmount);
+                msg.sender.sendEtherAndVerify(_message.fee);
+                refundTo.sendEtherAndVerify(refundAmount);
             }
             emit MessageExecuted(msgHash);
         } else if (!isMessageProven) {
@@ -494,13 +493,7 @@ contract Bridge is EssentialContract, IBridge {
         ) {
             success_ = false;
         } else {
-            (success_,) = ExcessivelySafeCall.excessivelySafeCall(
-                _message.to,
-                _gasLimit,
-                _message.value,
-                64, // return max 64 bytes
-                _message.data
-            );
+            success_ = _message.to.sendEther(_message.value, _gasLimit, _message.data);
         }
 
         // Must reset the context after the message call

diff --git a/packages/protocol/contracts/libs/LibAddress.sol b/packages/protocol/contracts/libs/LibAddress.sol
@@ -5,7 +5,6 @@ import "@openzeppelin/contracts/utils/Address.sol";
 import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
 import "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import "@openzeppelin/contracts/interfaces/IERC1271.sol";
-import "../thirdparty/nomad-xyz/ExcessivelySafeCall.sol";
 
 /// @title LibAddress
 /// @dev Provides utilities for address-related operations.
@@ -15,32 +14,58 @@ library LibAddress {
 
     error ETH_TRANSFER_FAILED();
 
-    /// @dev Sends Ether to the specified address.
+    /// @dev Sends Ether to the specified address. This method will not revert even if sending ether
+    /// fails.
+    /// This function is inspired by
+    /// https://github.com/nomad-xyz/ExcessivelySafeCall/blob/main/src/ExcessivelySafeCall.sol
     /// @param _to The recipient address.
     /// @param _amount The amount of Ether to send in wei.
     /// @param _gasLimit The max amount gas to pay for this transaction.
-    function sendEther(address _to, uint256 _amount, uint256 _gasLimit) internal {
+    /// @return success_ true if the call is successful, false otherwise.
+    function sendEther(
+        address _to,
+        uint256 _amount,
+        uint256 _gasLimit,
+        bytes memory _calldata
+    )
+        internal
+        returns (bool success_)
+    {
         // Check for zero-address transactions
         if (_to == address(0)) revert ETH_TRANSFER_FAILED();
+        // dispatch message to recipient
+        // by assembly calling "handle" function
+        // we call via assembly to avoid memcopying a very large returndata
+        // returned by a malicious contract
+        assembly {
+            success_ :=
+                call(
+                    _gasLimit, // gas
+                    _to, // recipient
+                    _amount, // ether value
+                    add(_calldata, 0x20), // inloc
+                    mload(_calldata), // inlen
+                    0, // outloc
+                    0 // outlen
+                )
+        }
+    }
 
-        // Attempt to send Ether to the recipient address
-        (bool success,) = ExcessivelySafeCall.excessivelySafeCall(
-            _to,
-            _gasLimit,
-            _amount,
-            64, // return max 64 bytes
-            ""
-        );
-
-        // Ensure the transfer was successful
-        if (!success) revert ETH_TRANSFER_FAILED();
+    /// @dev Sends Ether to the specified address. This method will revert if sending ether fails.
+    /// @param _to The recipient address.
+    /// @param _amount The amount of Ether to send in wei.
+    /// @param _gasLimit The max amount gas to pay for this transaction.
+    function sendEtherAndVerify(address _to, uint256 _amount, uint256 _gasLimit) internal {
+        if (!sendEther(_to, _amount, _gasLimit, "")) {
+            revert ETH_TRANSFER_FAILED();
+        }
     }
 
-    /// @dev Sends Ether to the specified address.
+    /// @dev Sends Ether to the specified address. This method will revert if sending ether fails.
     /// @param _to The recipient address.
     /// @param _amount The amount of Ether to send in wei.
-    function sendEther(address _to, uint256 _amount) internal {
-        sendEther(_to, _amount, gasleft());
+    function sendEtherAndVerify(address _to, uint256 _amount) internal {
+        sendEtherAndVerify(_to, _amount, gasleft());
     }
 
     function supportsInterface(

diff --git a/packages/protocol/contracts/thirdparty/nomad-xyz/ExcessivelySafeCall.sol b/packages/protocol/contracts/thirdparty/nomad-xyz/ExcessivelySafeCall.sol
diff --git a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol b/packages/protocol/contracts/tokenvault/ERC1155Vault.sol
@@ -109,7 +109,7 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
 
         // Transfer the ETH and the tokens to the `to` address
         address token = _transferTokens(ctoken, to, tokenIds, amounts);
-        to.sendEther(msg.value);
+        to.sendEtherAndVerify(msg.value);
 
         emit TokenReceived({
             msgHash: ctx.msgHash,
@@ -143,7 +143,7 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, message.srcOwner, tokenIds, amounts);
-        message.srcOwner.sendEther(message.value);
+        message.srcOwner.sendEtherAndVerify(message.value);
 
         // Emit TokenReleased event
         emit TokenReleased({

diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol
@@ -268,7 +268,7 @@ contract ERC20Vault is BaseVault {
 
         // Transfer the ETH and the tokens to the `to` address
         address token = _transferTokens(ctoken, to, amount);
-        to.sendEther(msg.value);
+        to.sendEtherAndVerify(msg.value);
 
         emit TokenReceived({
             msgHash: ctx.msgHash,
@@ -301,7 +301,7 @@ contract ERC20Vault is BaseVault {
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, _message.srcOwner, amount);
-        _message.srcOwner.sendEther(_message.value);
+        _message.srcOwner.sendEtherAndVerify(_message.value);
 
         emit TokenReleased({
             msgHash: _msgHash,

diff --git a/packages/protocol/contracts/tokenvault/ERC721Vault.sol b/packages/protocol/contracts/tokenvault/ERC721Vault.sol
@@ -92,7 +92,7 @@ contract ERC721Vault is BaseNFTVault, IERC721Receiver {
 
         // Transfer the ETH and the tokens to the `to` address
         address token = _transferTokens(ctoken, to, tokenIds);
-        to.sendEther(msg.value);
+        to.sendEtherAndVerify(msg.value);
 
         emit TokenReceived({
             msgHash: ctx.msgHash,
@@ -126,7 +126,7 @@ contract ERC721Vault is BaseNFTVault, IERC721Receiver {
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, _message.srcOwner, tokenIds);
-        _message.srcOwner.sendEther(_message.value);
+        _message.srcOwner.sendEtherAndVerify(_message.value);
 
         emit TokenReleased({
             msgHash: _msgHash,