Extract execution to a separate function

4337/contracts/EIP4337Module.sol

@@ -44,11 +44,13 @@ 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
+        // 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(expectedExecutionFunctionId == 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 +61,21 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         }
     }
 
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful
+    /// @param executionData Execution data. Expects it to be a call to the `execTransactionFromModule` function of the Safe.
+    ///                      Validated in the `validateUserOp` function.
+    function executeUserOp(bytes calldata executionData) external {
+        address entryPoint = _msgSender();
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        (bool outerCallSuccess, bytes memory returnData) = msg.sender.call(executionData);
+        bool internalCallSuccess = returnData[31] == 0x01;
+        if (!outerCallSuccess || !internalCallSuccess) {
+            revert("Execution failed");
+        }
+    }
+
     function domainSeparator() public view returns (bytes32) {
         return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this));
     }
@@ -127,7 +144,5 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
 }
 
 contract Simple4337Module is EIP4337Module {
-    constructor(address entryPoint)
-        EIP4337Module(entryPoint, bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)")))
-    {}
+    constructor(address entryPoint) EIP4337Module(entryPoint, bytes4(keccak256("executeUserOp(bytes)"))) {}
 }

4337/contracts/test/SafeMock.sol

@@ -128,7 +128,7 @@ contract Safe4337Mock is SafeMock {
     bytes4 public immutable expectedExecutionFunctionId;
 
     constructor(address entryPoint) SafeMock(entryPoint) {
-        expectedExecutionFunctionId = bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)"));
+        expectedExecutionFunctionId = bytes4(keccak256("executeUserOp(bytes)"));
     }
 
     /// @dev Validates user operation provided by the entry point
@@ -156,6 +156,24 @@ contract Safe4337Mock is SafeMock {
         return 0;
     }
 
+    /// @notice Executes user operation provided by the entry point
+    /// @dev Reverts if unsuccessful
+    /// @param executionData Execution data. Expects it to be a call to the `execTransactionFromModule` function of the Safe.
+    ///                      Validated in the `validateUserOp` function.
+    function executeUserOp(bytes calldata executionData) external {
+        address entryPoint = msg.sender;
+        require(entryPoint == supportedEntryPoint, "Unsupported entry point");
+
+        // [4:] slice is needed to remove the function selector, abi.decode will not work with it because
+        // it is not 32 bytes
+        (address to, uint256 value, bytes memory data, uint8 operation) = abi.decode(executionData[4:], (address, uint256, bytes, uint8));
+
+        bool success;
+        if (operation == 1) (success, ) = to.delegatecall(data);
+        else (success, ) = to.call{value: value}(data);
+        require(success, "Execution failed");
+    }
+
     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();
     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, ) = userOp.sender.call{gas: userOp.callGasLimit}(userOp.callData);
+        if (!success) {
+            emit UserOpReverted();
+        }
     }
 
     function getNonce(address sender, uint192 key) external view override returns (uint256 nonce) {

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(bytes executionData)',
     '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
     ])
@@ -95,10 +96,15 @@ const callInterface = async(provider: providers.Provider, contract: string, meth
 }
 
 const actionCalldata = (action: MetaTransaction): string => {
-    return INTERFACES.encodeFunctionData(
+    const execTransactionFromModuleCallData = INTERFACES.encodeFunctionData(
         'execTransactionFromModule',
         [action.to, action.value, action.data, action.operation]
     )
+
+    return INTERFACES.encodeFunctionData(
+        'executeUserOp',
+        [execTransactionFromModuleCallData]
+    )
 }
 
 export class MultiProvider4337 extends providers.JsonRpcProvider {

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 => {
@@ -91,8 +91,10 @@ export const buildSafeUserOpTransaction = (
 ): SafeUserOperation => {
   const abi = [
     'function execTransactionFromModule(address to, uint256 value, bytes calldata data, uint8 operation) external payable returns (bool success)',
+    'function executeUserOp(bytes calldata executionData) external',
   ]
-  const callData = new ethersUtils.Interface(abi).encodeFunctionData('execTransactionFromModule', [to, value, data, delegateCall ? 1 : 0])
+  const execTransactionFromModuleCallData = new ethersUtils.Interface(abi).encodeFunctionData('execTransactionFromModule', [to, value, data, delegateCall ? 1 : 0])
+  const callData = new ethersUtils.Interface(abi).encodeFunctionData('executeUserOp', [execTransactionFromModuleCallData])
 
   return buildSafeUserOp(
     Object.assign(
@@ -178,6 +180,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)
 }

4337/test/eip4337/EIP4337ModuleExisting.spec.ts

@@ -11,7 +11,7 @@ import {
 } from '../../src/utils/userOp'
 import { chainId } from '../utils/encoding'
 
-describe('EIP4337Module', async () => {
+describe('EIP4337Module - Existing Safe', async () => {
   const [user1] = waffle.provider.getWallets()
 
   const setupTests = deployments.createFixture(async ({ deployments }) => {
@@ -111,5 +111,22 @@ describe('EIP4337Module', async () => {
       )
       expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("0.499999"))
     })
+
+    it('reverts on failure', async () => {
+      const { safe, validator, entryPoint } = await setupTests()
+
+      await user1.sendTransaction({to: safe.address, value: ethers.utils.parseEther("0.000001")})
+      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("0.000001"))
+      const safeOp = buildSafeUserOpTransaction(safe.address, user1.address, ethers.utils.parseEther("0.5"), "0x", '0', entryPoint.address)
+      const safeOpHash = calculateSafeOperationHash(validator.address, safeOp, await chainId())
+      const signature = buildSignatureBytes([await signHash(user1, safeOpHash)])
+      const userOp = buildUserOperationFromSafeUserOperation({safeAddress: safe.address, safeOp, signature})
+
+      const transaction = await entryPoint.executeUserOp(userOp, ethers.utils.parseEther("0.000001")).then((tx: any) => tx.wait())
+      const logs = transaction.logs.map((log: any) => entryPoint.interface.parseLog(log))
+      const emittedRevert = logs.some((l: any) => l.name === "UserOpReverted")
+
+      expect(emittedRevert).to.be.true
+    })
   })
 })

4337/test/eip4337/EIP4337ModuleNew.spec.ts

@@ -11,7 +11,7 @@ import {
 import { chainId } from '../utils/encoding'
 import { Safe4337 } from '../../src/utils/safe'
 
-describe('EIP4337Module', async () => {
+describe('EIP4337Module - Newly deployed safe', async () => {
   const [user1] = waffle.provider.getWallets()
 
   const setupTests = deployments.createFixture(async ({ deployments }) => {

4337/test/eip4337/EIP4337Safe.spec.ts

@@ -16,7 +16,7 @@ describe('EIP4337Safe', async () => {
 
   const setupTests = deployments.createFixture(async ({ deployments }) => {
     await deployments.fixture()
-    
+
     const entryPoint = await getEntryPoint()
     const module = await getSimple4337Module()
     const safe = await get4337TestSafe(user1, ethers.constants.AddressZero, ethers.constants.AddressZero)
@@ -25,7 +25,7 @@ describe('EIP4337Safe', async () => {
     return {
       safe,
       validator: safe4337,
-      entryPoint
+      entryPoint,
     }
   })
 
@@ -54,33 +54,27 @@ describe('EIP4337Safe', async () => {
     it('should execute contract calls without fee', async () => {
       const { safe, validator, entryPoint } = await setupTests()
 
-      await user1.sendTransaction({to: safe.address, value: ethers.utils.parseEther("1.0")})
-      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("1.0"))
-      const safeOp = buildSafeUserOpTransaction(safe.address, user1.address, ethers.utils.parseEther("0.5"), "0x", '0', entryPoint.address)
+      await user1.sendTransaction({ to: safe.address, value: ethers.utils.parseEther('1.0') })
+      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther('1.0'))
+      const safeOp = buildSafeUserOpTransaction(safe.address, user1.address, ethers.utils.parseEther('0.5'), '0x', '0', entryPoint.address)
       const safeOpHash = calculateSafeOperationHash(validator.address, safeOp, await chainId())
       const signature = buildSignatureBytes([await signHash(user1, safeOpHash)])
-      const userOp = buildUserOperationFromSafeUserOperation({safeAddress: safe.address, safeOp, signature})
-      await logGas(
-        "Execute UserOp without fee payment",
-        entryPoint.executeUserOp(userOp, 0)
-      )
-      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("0.5"))
+      const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: safe.address, safeOp, signature })
+      await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0))
+      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther('0.5'))
     })
 
     it('should execute contract calls with fee', async () => {
       const { safe, validator, entryPoint } = await setupTests()
 
-      await user1.sendTransaction({to: safe.address, value: ethers.utils.parseEther("1.0")})
-      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("1.0"))
-      const safeOp = buildSafeUserOpTransaction(safe.address, user1.address, ethers.utils.parseEther("0.5"), "0x", '0', entryPoint.address)
+      await user1.sendTransaction({ to: safe.address, value: ethers.utils.parseEther('1.0') })
+      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther('1.0'))
+      const safeOp = buildSafeUserOpTransaction(safe.address, user1.address, ethers.utils.parseEther('0.5'), '0x', '0', entryPoint.address)
       const safeOpHash = calculateSafeOperationHash(validator.address, safeOp, await chainId())
       const signature = buildSignatureBytes([await signHash(user1, safeOpHash)])
-      const userOp = buildUserOperationFromSafeUserOperation({safeAddress: safe.address, safeOp, signature})
-      await logGas(
-        "Execute UserOp with fee payment",
-        entryPoint.executeUserOp(userOp, ethers.utils.parseEther("0.000001"))
-      )
-      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther("0.499999"))
+      const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: safe.address, safeOp, signature })
+      await logGas('Execute UserOp with fee payment', entryPoint.executeUserOp(userOp, ethers.utils.parseEther('0.000001')))
+      expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.utils.parseEther('0.499999'))
     })
   })
 })