[SC-430] Revert on non-existent contract instead of silent success (#17)

* switch to OZ function calls/delegatecalls to error on empty contract; add some more testing coverage for revert cases

* remove incorrect comments

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;
 
@@ -140,13 +145,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
@@ -307,15 +308,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(
@@ -332,24 +329,4 @@ abstract contract BridgeExecutorBase is IExecutorBase {
         _queuedActions[actionHash] = false;
     }
 
-    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();
-            }
-        }
-    }
 }

src/interfaces/IExecutorBase.sol

@@ -150,13 +150,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

test/AuthBridgeExecutor.t.sol

@@ -362,19 +362,19 @@ 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;
         _queueAction(action);
         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);
@@ -385,6 +385,52 @@ contract AuthBridgeExecutorExecuteTests is AuthBridgeExecutorTestBase {
         executor.execute(0);
     }
 
+    function test_execute_delegateCallEmptyContract() public {
+        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 {
+        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);