Merge pull request #95 from safe-global/feature/require-success-for-o…

…peration-execution

ERC4337 module feature: Extract execution to a separate function and require success in the internal call

4337/.env.sample

@@ -7,11 +7,18 @@ ETHERSCAN_API_KEY=""
 
 # Mnemonic to use in the test script
 SCRIPT_MNEMONIC=""
-# Safe to use in the user op test script
-SCRIPT_SAFE_ADDRESS=
 # Enable debug in the test script
 SCRIPT_DEBUG=true
 # Bundler to use in the user op test script
 SCRIPT_BUNDLER_URL=""
 # Module to use in the user op test script
-SCRIPT_MODULE_ADDRESS=
+SCRIPT_MODULE_ADDRESS=
+# Library to add modules in the user op test script
+SCRIPT_ADD_MODULES_LIB_ADDRESS=""
+# Proxy factory to use in the user op test script
+SCRIPT_PROXY_FACTORY_ADDRESS=
+# Singleton address to use in the user op test script
+SCRIPT_SAFE_SINGLETON_ADDRESS=
+
+# Entrypoint address to use for the module deployment
+DEPLOY_ENTRY_POINT=""

4337/contracts/EIP4337Module.sol

@@ -8,7 +8,7 @@ import {INonceManager} from "./interfaces/ERC4337.sol";
 import {ISafe} from "./interfaces/Safe.sol";
 
 /// @title EIP4337Module
-abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler {
+contract Simple4337Module is HandlerContext, CompatibilityFallbackHandler {
     using UserOperationLib for UserOperation;
 
     // value in case of signature failure, with no time-range.
@@ -23,11 +23,9 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         );
 
     address public immutable supportedEntryPoint;
-    bytes4 public immutable expectedExecutionFunctionId;
 
-    constructor(address entryPoint, bytes4 executionFunctionId) {
+    constructor(address entryPoint) {
         supportedEntryPoint = entryPoint;
-        expectedExecutionFunctionId = executionFunctionId;
     }
 
     /// @dev Validates user operation provided by the entry point
@@ -44,11 +42,16 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         // the sender is the Safe specified in the userOperation
         require(safeAddress == msg.sender, "Invalid Caller");
 
-        // We verify that the userOp calls the expected execution function
-        require(expectedExecutionFunctionId == bytes4(userOp.callData), "Unsupported execution function id");
+        // We check the execution function signature to make sure the entryPoint can't call any other function
+        // and make sure the execution of the user operation is handled by the module
+        require(
+            this.executeUserOp.selector == bytes4(userOp.callData) || this.executeUserOpWithErrorString.selector == bytes4(userOp.callData),
+            "Unsupported execution function id"
+        );
 
         address entryPoint = _msgSender();
         require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
         // The userOp nonce is validated in the Entrypoint (for 0.6.0+), therefore we will not check it again
         validationResult = validateSignatures(entryPoint, userOp);
 
@@ -59,6 +62,47 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         }
     }
 
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful
+    /// @param to Destination address of the user operation.
+    /// @param value Ether value of the user operation.
+    /// @param data Data payload of the user operation.
+    /// @param operation Operation type of the user operation.
+    function executeUserOp(
+        address to,
+        uint256 value,
+        bytes memory data,
+        uint8 operation
+    ) external {
+        address entryPoint = _msgSender();
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        require(ISafe(msg.sender).execTransactionFromModule(to, value, data, operation), "Execution failed");
+    }
+
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful and bubbles up the error message
+    /// @param to Destination address of the user operation.
+    /// @param value Ether value of the user operation.
+    /// @param data Data payload of the user operation.
+    /// @param operation Operation type of the user operation.
+    function executeUserOpWithErrorString(
+        address to,
+        uint256 value,
+        bytes memory data,
+        uint8 operation
+    ) external {
+        address entryPoint = _msgSender();
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        (bool success, bytes memory returnData) = ISafe(msg.sender).execTransactionFromModuleReturnData(to, value, data, operation);
+        if (!success) {
+            assembly {
+                revert(add(returnData, 0x20), returnData)
+            }
+        }
+    }
+
     function domainSeparator() public view returns (bytes32) {
         return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this));
     }
@@ -125,9 +169,3 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         }
     }
 }
-
-contract Simple4337Module is EIP4337Module {
-    constructor(address entryPoint)
-        EIP4337Module(entryPoint, bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)")))
-    {}
-}

4337/contracts/interfaces/Safe.sol

@@ -24,6 +24,22 @@ interface ISafe {
         uint8 operation
     ) external returns (bool success);
 
+    /**
+     * @notice Execute `operation` (0: Call, 1: DelegateCall) to `to` with `value` (Native Token) and return data
+     * @param to Destination address of module transaction.
+     * @param value Ether value of module transaction.
+     * @param data Data payload of module transaction.
+     * @param operation Operation type of module transaction.
+     * @return success Boolean flag indicating if the call succeeded.
+     * @return returnData Data returned by the call.
+     */
+    function execTransactionFromModuleReturnData(
+        address to,
+        uint256 value,
+        bytes memory data,
+        uint8 operation
+    ) external returns (bool success, bytes memory returnData);
+
     /**
      * @dev Checks whether the signature provided is valid for the provided data, hash. Will revert otherwise.
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)

4337/contracts/test/SafeMock.sol

@@ -73,6 +73,18 @@ contract SafeMock {
         else (success, ) = to.call{value: value}(data);
     }
 
+    function execTransactionFromModuleReturnData(
+        address payable to,
+        uint256 value,
+        bytes calldata data,
+        uint8 operation
+    ) external returns (bool success, bytes memory returnData) {
+        require(modules[msg.sender], "not executing that");
+
+        if (operation == 1) (success, returnData) = to.delegatecall(data);
+        else (success, returnData) = to.call{value: value}(data);
+    }
+
     /**
      * @dev Reads `length` bytes of storage in the currents contract
      * @param offset - the offset in the current contract's storage in words to start reading from
@@ -125,11 +137,7 @@ contract Safe4337Mock is SafeMock {
             "SafeOp(address safe,bytes callData,uint256 nonce,uint256 preVerificationGas,uint256 verificationGasLimit,uint256 callGasLimit,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,address entryPoint)"
         );
 
-    bytes4 public immutable expectedExecutionFunctionId;
-
-    constructor(address entryPoint) SafeMock(entryPoint) {
-        expectedExecutionFunctionId = bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)"));
-    }
+    constructor(address entryPoint) SafeMock(entryPoint) {}
 
     /// @dev Validates user operation provided by the entry point
     /// @param userOp User operation struct
@@ -144,7 +152,12 @@ contract Safe4337Mock is SafeMock {
 
         validateReplayProtection(userOp);
 
-        require(expectedExecutionFunctionId == bytes4(userOp.callData), "Unsupported execution function id");
+        // We check the execution function signature to make sure the entryPoint can't call any other function
+        // and make sure the execution of the user operation is handled by the module
+        require(
+            this.executeUserOp.selector == bytes4(userOp.callData) || this.executeUserOpWithErrorString.selector == bytes4(userOp.callData),
+            "Unsupported execution function id"
+        );
 
         // We need to make sure that the entryPoint's requested prefund is in bounds
         require(requiredPrefund <= userOp.requiredPreFund(), "Prefund too high");
@@ -156,6 +169,49 @@ contract Safe4337Mock is SafeMock {
         return 0;
     }
 
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful
+    /// @param to Destination address of the user operation.
+    /// @param value Ether value of the user operation.
+    /// @param data Data payload of the user operation.
+    /// @param operation Operation type of the user operation.
+    function executeUserOp(
+        address to,
+        uint256 value,
+        bytes memory data,
+        uint8 operation
+    ) external {
+        address entryPoint = msg.sender;
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        bool success;
+        if (operation == 1) (success, ) = to.delegatecall(data);
+        else (success, ) = to.call{value: value}(data);
+        require(success, "Execution failed");
+    }
+
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful and bubbles up the error message
+    /// @param to Destination address of the user operation.
+    /// @param value Ether value of the user operation.
+    /// @param data Data payload of the user operation.
+    /// @param operation Operation type of the user operation.
+    function executeUserOpWithErrorString(
+        address to,
+        uint256 value,
+        bytes memory data,
+        uint8 operation
+    ) external {
+        address entryPoint = msg.sender;
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        bool success;
+        bytes memory returnData;
+        if (operation == 1) (success, returnData) = to.delegatecall(data);
+        else (success, returnData) = to.call{value: value}(data);
+        require(success, string(returnData));
+    }
+
     function domainSeparator() public view returns (bytes32) {
         return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this));
     }

4337/contracts/test/TestEntryPoint.sol

@@ -39,6 +39,7 @@ contract SenderCreator {
 contract TestEntryPoint is INonceManager {
     error NotEnoughFunds(uint256 expected, uint256 available);
     error InvalidNonce(uint256 userNonce);
+    event UserOpReverted(bytes reason);
     SenderCreator public immutable senderCreator;
     mapping(address => uint256) balances;
     mapping(address => mapping(uint192 => uint256)) public nonceSequenceNumber;
@@ -76,7 +77,10 @@ contract TestEntryPoint is INonceManager {
         }
 
         require(gasleft() > userOp.callGasLimit, "Not enough gas for execution");
-        userOp.sender.call{gas: userOp.callGasLimit}(userOp.callData);
+        (bool success, bytes memory returnData) = userOp.sender.call{gas: userOp.callGasLimit}(userOp.callData);
+        if (!success) {
+            emit UserOpReverted(returnData);
+        }
     }
 
     function getNonce(address sender, uint192 key) external view override returns (uint256 nonce) {

4337/contracts/test/TestReverter.sol

@@ -0,0 +1,7 @@
+pragma solidity >=0.8.0;
+
+contract TestReverter {
+    function alwaysReverting() external pure {
+        revert("You called a function that always reverts");
+    }
+}

4337/src/utils/safe.ts

@@ -14,6 +14,7 @@ const INTERFACES = new ethers.utils.Interface([
     'function proxyCreationCode() returns (bytes)',
     'function enableModules(address[])',
     'function execTransactionFromModule(address to, uint256 value, bytes calldata data, uint8 operation) external payable returns (bool success)',
+    'function executeUserOp(address to, uint256 value, bytes calldata data, uint8 operation)',
     'function getNonce(address,uint192) returns (uint256 nonce)',
     'function supportedEntryPoint() returns (address)',
     'function getOwners() returns (address[])',
@@ -70,7 +71,7 @@ const calculateProxyAddress = (globalConfig: GlobalConfig, inititalizer: string,
   };
 
 const buildInitParamsForConfig = (safeConfig: SafeConfig, globalConfig: GlobalConfig): { safeAddress: string, initCode: string } => {
-    const initData = INTERFACES.encodeFunctionData("enableModules", [[globalConfig.erc4337module, globalConfig.entryPoint]])
+    const initData = INTERFACES.encodeFunctionData("enableModules", [[globalConfig.erc4337module]])
     const setupData = INTERFACES.encodeFunctionData("setup", [
         safeConfig.signers, safeConfig.threshold, globalConfig.addModulesLib, initData, globalConfig.erc4337module, constants.AddressZero, 0, constants.AddressZero
     ])
@@ -96,7 +97,7 @@ const callInterface = async(provider: providers.Provider, contract: string, meth
 
 const actionCalldata = (action: MetaTransaction): string => {
     return INTERFACES.encodeFunctionData(
-        'execTransactionFromModule',
+        'executeUserOp',
         [action.to, action.value, action.data, action.operation]
     )
 }

4337/src/utils/userOp.ts

@@ -50,16 +50,16 @@ export const calculateSafeOperationHash = (eip4337ModuleAddress: string, safeOp:
   return ethersUtils._TypedDataEncoder.hash({ chainId, verifyingContract: eip4337ModuleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp)
 }
 
-export const signSafeOp = async(
+export const signSafeOp = async (
   signer: Signer & TypedDataSigner,
   moduleAddress: string,
   safeOp: SafeUserOperation,
   chainId: BigNumberish
-  ): Promise<SafeSignature> => {
-    return {
-      signer: await signer.getAddress(),
-      data: await signer._signTypedData({ chainId, verifyingContract: moduleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp),
-    }
+): Promise<SafeSignature> => {
+  return {
+    signer: await signer.getAddress(),
+    data: await signer._signTypedData({ chainId, verifyingContract: moduleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp),
+  }
 }
 
 export const buildSafeUserOp = (template: OptionalExceptFor<SafeUserOperation, 'safe' | 'nonce' | 'entryPoint'>): SafeUserOperation => {
@@ -87,12 +87,15 @@ export const buildSafeUserOpTransaction = (
   nonce: string,
   entryPoint: string,
   delegateCall?: boolean,
+  bubbleUpRevertReason?: boolean,
   overrides?: Partial<SafeUserOperation>,
 ): SafeUserOperation => {
   const abi = [
-    'function execTransactionFromModule(address to, uint256 value, bytes calldata data, uint8 operation) external payable returns (bool success)',
+    'function executeUserOp(address to, uint256 value, bytes calldata data, uint8 operation) external',
+    'function executeUserOpWithErrorString(address to, uint256 value, bytes calldata data, uint8 operation) external',
   ]
-  const callData = new ethersUtils.Interface(abi).encodeFunctionData('execTransactionFromModule', [to, value, data, delegateCall ? 1 : 0])
+  const method = bubbleUpRevertReason ? 'executeUserOpWithErrorString' : 'executeUserOp'
+  const callData = new ethersUtils.Interface(abi).encodeFunctionData(method, [to, value, data, delegateCall ? 1 : 0])
 
   return buildSafeUserOp(
     Object.assign(
@@ -116,11 +119,12 @@ export const buildSafeUserOpContractCall = (
   operationValue: string,
   entryPoint: string,
   delegateCall?: boolean,
+  bubbleUpRevertReason?: boolean,
   overrides?: Partial<SafeUserOperation>,
 ): SafeUserOperation => {
   const data = contract.interface.encodeFunctionData(method, params)
 
-  return buildSafeUserOpTransaction(safeAddress, contract.address, operationValue, data, nonce, entryPoint, delegateCall, overrides)
+  return buildSafeUserOpTransaction(safeAddress, contract.address, operationValue, data, nonce, entryPoint, delegateCall, bubbleUpRevertReason, overrides)
 }
 
 export const buildUserOperationFromSafeUserOperation = ({
@@ -178,6 +182,6 @@ export const calculateIntermediateTxHash = (callData: string, nonce: BigNumberis
 
 export const getSupportedEntryPoints = async (provider: ethers.providers.JsonRpcProvider): Promise<string[]> => {
   const supportedEntryPoints = await provider.send('eth_supportedEntryPoints', [])
-  console.log({supportedEntryPoints})
+  console.log({ supportedEntryPoints })
   return supportedEntryPoints.map(ethers.utils.getAddress)
 }