From cf50a08a2a122869df7b2875b295a4aa9dd76c19 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Sat, 8 Jun 2024 16:24:50 +0900 Subject: [PATCH 1/2] switch to OZ function calls/delegatecalls to error on empty contract; add some more testing coverage for revert cases --- src/executors/BridgeExecutorBase.sol | 41 +++++---------------- src/interfaces/IExecutorBase.sol | 3 +- test/AuthBridgeExecutor.t.sol | 54 ++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/src/executors/BridgeExecutorBase.sol b/src/executors/BridgeExecutorBase.sol index f2d8f3a..0086c53 100644 --- a/src/executors/BridgeExecutorBase.sol +++ b/src/executors/BridgeExecutorBase.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: AGPL-3.0 pragma solidity ^0.8.10; +import { Address } from "lib/openzeppelin-contracts/contracts/utils/Address.sol"; + import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol'; /** @@ -11,6 +13,9 @@ import { IExecutorBase } from 'src/interfaces/IExecutorBase.sol'; * contract with proper access control */ abstract contract BridgeExecutorBase is IExecutorBase { + + using Address for address; + // Minimum allowed grace period, which reduces the risk of having an actions set expire due to network congestion uint256 constant MINIMUM_GRACE_PERIOD = 10 minutes; @@ -168,13 +173,9 @@ abstract contract BridgeExecutorBase is IExecutorBase { payable override onlyThis - returns (bool, bytes memory) + returns (bytes memory) { - bool success; - bytes memory resultData; - // solium-disable-next-line security/no-call-value - (success, resultData) = target.delegatecall(data); - return (success, resultData); + return target.functionDelegateCall(data); } /// @inheritdoc IExecutorBase @@ -355,15 +356,11 @@ abstract contract BridgeExecutorBase is IExecutorBase { callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data); } - bool success; - bytes memory resultData; if (withDelegatecall) { - (success, resultData) = this.executeDelegateCall{value: value}(target, callData); + return this.executeDelegateCall{value: value}(target, callData); } else { - // solium-disable-next-line security/no-call-value - (success, resultData) = target.call{value: value}(callData); + return target.functionCallWithValue(callData, value); } - return _verifyCallResult(success, resultData); } function _cancelTransaction( @@ -385,24 +382,4 @@ abstract contract BridgeExecutorBase is IExecutorBase { if (delay > _maximumDelay) revert DelayLongerThanMax(); } - function _verifyCallResult(bool success, bytes memory returnData) - private pure returns (bytes memory) - { - if (success) { - return returnData; - } else { - // Look for revert reason and bubble it up if present - if (returnData.length > 0) { - // The easiest way to bubble the revert reason is using memory via assembly - - // solhint-disable-next-line no-inline-assembly - assembly { - let returndata_size := mload(returnData) - revert(add(32, returnData), returndata_size) - } - } else { - revert FailedActionExecution(); - } - } - } } diff --git a/src/interfaces/IExecutorBase.sol b/src/interfaces/IExecutorBase.sol index 20fc7dc..e736221 100644 --- a/src/interfaces/IExecutorBase.sol +++ b/src/interfaces/IExecutorBase.sol @@ -176,13 +176,12 @@ interface IExecutorBase { * @notice Allows to delegatecall a given target with an specific amount of value * @dev This function is external so it allows to specify a defined msg.value for the delegate call, reducing * the risk that a delegatecall gets executed with more value than intended - * @return True if the delegate call was successful, false otherwise * @return The bytes returned by the delegate call **/ function executeDelegateCall(address target, bytes calldata data) external payable - returns (bool, bytes memory); + returns (bytes memory); /** * @notice Allows to receive funds into the executor diff --git a/test/AuthBridgeExecutor.t.sol b/test/AuthBridgeExecutor.t.sol index 5f6b752..3073823 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/AuthBridgeExecutor.t.sol @@ -372,7 +372,7 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { executor.execute(0); } - function test_execute_evm_error() public { + function test_execute_delegateCallEvmError() public { // Trigger some evm error like trying to call a non-payable function Action memory action = _getDefaultAction(); action.values[0] = 1 ether; @@ -380,11 +380,11 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { skip(DELAY); vm.deal(address(executor), 1 ether); - vm.expectRevert(abi.encodeWithSignature("FailedActionExecution()")); + vm.expectRevert(abi.encodeWithSignature("FailedInnerCall()")); executor.execute(0); } - function test_execute_revert_error() public { + function test_execute_delegateCallRevertError() public { Action memory action = _getDefaultAction(); action.targets[0] = address(new RevertingPayload()); _queueAction(action); @@ -395,6 +395,54 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { executor.execute(0); } + function test_execute_delegateCallEmptyContract() public { + // Trigger some evm error like trying to call a non-payable function + Action memory action = _getDefaultAction(); + action.targets[0] = makeAddr("emptyContract"); + _queueAction(action); + skip(DELAY); + + vm.expectRevert(abi.encodeWithSignature("AddressEmptyCode(address)", action.targets[0])); + executor.execute(0); + } + + function test_execute_callEvmError() public { + // Trigger some evm error like trying to call a non-payable function + Action memory action = _getDefaultAction(); + action.values[0] = 1 ether; + action.withDelegatecalls[0] = false; + _queueAction(action); + skip(DELAY); + vm.deal(address(executor), 1 ether); + + vm.expectRevert(abi.encodeWithSignature("FailedInnerCall()")); + executor.execute(0); + } + + function test_execute_callRevertError() public { + Action memory action = _getDefaultAction(); + action.targets[0] = address(new RevertingPayload()); + action.withDelegatecalls[0] = false; + _queueAction(action); + skip(DELAY); + + // Should return the underlying error message + vm.expectRevert("An error occurred"); + executor.execute(0); + } + + function test_execute_callEmptyContract() public { + // Trigger some evm error like trying to call a non-payable function + Action memory action = _getDefaultAction(); + action.targets[0] = makeAddr("emptyContract"); + action.withDelegatecalls[0] = false; + _queueAction(action); + skip(DELAY); + + vm.expectRevert(abi.encodeWithSignature("AddressEmptyCode(address)", action.targets[0])); + executor.execute(0); + } + function test_execute_delegateCall() public { Action memory action = _getDefaultAction(); bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); From d3eff318875ba2c91fb4c8b3916dd0294afc9a94 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Tue, 11 Jun 2024 16:28:47 +0900 Subject: [PATCH 2/2] remove incorrect comments --- test/AuthBridgeExecutor.t.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/AuthBridgeExecutor.t.sol b/test/AuthBridgeExecutor.t.sol index 3073823..715abf0 100644 --- a/test/AuthBridgeExecutor.t.sol +++ b/test/AuthBridgeExecutor.t.sol @@ -396,7 +396,6 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { } function test_execute_delegateCallEmptyContract() public { - // Trigger some evm error like trying to call a non-payable function Action memory action = _getDefaultAction(); action.targets[0] = makeAddr("emptyContract"); _queueAction(action); @@ -432,7 +431,6 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase { } function test_execute_callEmptyContract() public { - // Trigger some evm error like trying to call a non-payable function Action memory action = _getDefaultAction(); action.targets[0] = makeAddr("emptyContract"); action.withDelegatecalls[0] = false;