From e3815278237d9bb6b593f1c38227a6720c185024 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Mon, 16 Oct 2023 09:42:30 +0200
Subject: [PATCH 1/4] Extract execution to a separate function

---
 4337/contracts/EIP4337Module.sol              | 48 ++++++++++++++++---
 4337/contracts/test/SafeMock.sol              | 20 +++++++-
 4337/contracts/test/TestEntryPoint.sol        |  6 ++-
 4337/src/utils/safe.ts                        | 10 +++-
 4337/src/utils/userOp.ts                      | 18 +++----
 .../eip4337/EIP4337ModuleExisting.spec.ts     | 19 +++++++-
 4337/test/eip4337/EIP4337ModuleNew.spec.ts    |  2 +-
 4337/test/eip4337/EIP4337Safe.spec.ts         | 34 ++++++-------
 8 files changed, 116 insertions(+), 41 deletions(-)

diff --git a/4337/contracts/EIP4337Module.sol b/4337/contracts/EIP4337Module.sol
index 533402ff7..326318a82 100644
--- a/4337/contracts/EIP4337Module.sol
+++ b/4337/contracts/EIP4337Module.sol
@@ -6,6 +6,7 @@ import {CompatibilityFallbackHandler} from "@safe-global/safe-contracts/contract
 import {UserOperation, UserOperationLib} from "./UserOperation.sol";
 import {INonceManager} from "./interfaces/ERC4337.sol";
 import {ISafe} from "./interfaces/Safe.sol";
+import "hardhat/console.sol";
 
 /// @title EIP4337Module
 abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler {
@@ -23,11 +24,17 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         );
 
     address public immutable supportedEntryPoint;
-    bytes4 public immutable expectedExecutionFunctionId;
-
-    constructor(address entryPoint, bytes4 executionFunctionId) {
+    bytes4 public immutable expectedOuterExecutionFunctionId;
+    bytes4 public immutable expectedInnerExecutionFunctionId;
+
+    constructor(
+        address entryPoint,
+        bytes4 outerExecutionFunctionId,
+        bytes4 innerExecutionFunctionId
+    ) {
         supportedEntryPoint = entryPoint;
-        expectedExecutionFunctionId = executionFunctionId;
+        expectedOuterExecutionFunctionId = outerExecutionFunctionId;
+        expectedInnerExecutionFunctionId = innerExecutionFunctionId;
     }
 
     /// @dev Validates user operation provided by the entry point
@@ -44,11 +51,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
-        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(expectedOuterExecutionFunctionId == 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 +68,27 @@ 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");
+
+        // 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(expectedInnerExecutionFunctionId == bytes4(executionData), "Unsupported execution function id");
+
+        (bool outerCallSuccess, bytes memory returnData) = msg.sender.call(executionData);
+        require(returnData.length == 32, "Invalid return data length");
+
+        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));
     }
@@ -128,6 +158,10 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
 
 contract Simple4337Module is EIP4337Module {
     constructor(address entryPoint)
-        EIP4337Module(entryPoint, bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)")))
+        EIP4337Module(
+            entryPoint,
+            bytes4(keccak256("executeUserOp(bytes)")),
+            bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)"))
+        )
     {}
 }
diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol
index 1b23e92a8..db8452e54 100644
--- a/4337/contracts/test/SafeMock.sol
+++ b/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));
     }
diff --git a/4337/contracts/test/TestEntryPoint.sol b/4337/contracts/test/TestEntryPoint.sol
index b400f8034..bdb230196 100644
--- a/4337/contracts/test/TestEntryPoint.sol
+++ b/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) {
diff --git a/4337/src/utils/safe.ts b/4337/src/utils/safe.ts
index f05bedbd2..236e329c2 100644
--- a/4337/src/utils/safe.ts
+++ b/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 {
diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts
index a099365f6..ff95d7dd2 100644
--- a/4337/src/utils/userOp.ts
+++ b/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)
 }
diff --git a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts b/4337/test/eip4337/EIP4337ModuleExisting.spec.ts
index 2ba993f3f..b744766af 100644
--- a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts
+++ b/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
+    })
   })
 })
diff --git a/4337/test/eip4337/EIP4337ModuleNew.spec.ts b/4337/test/eip4337/EIP4337ModuleNew.spec.ts
index 1cccf6280..b2cab3a90 100644
--- a/4337/test/eip4337/EIP4337ModuleNew.spec.ts
+++ b/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 }) => {
diff --git a/4337/test/eip4337/EIP4337Safe.spec.ts b/4337/test/eip4337/EIP4337Safe.spec.ts
index 27a6240dd..a532716ee 100644
--- a/4337/test/eip4337/EIP4337Safe.spec.ts
+++ b/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'))
     })
   })
 })

From 50e4f94002d820d6a9fdff3a5b2c3995c78f73eb Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Tue, 17 Oct 2023 11:09:33 +0200
Subject: [PATCH 2/4] Use safe's execTransactionFromModule instead of
 forwarding bytes calldata

---
 4337/contracts/EIP4337Module.sol | 51 +++++++++++---------------------
 4337/contracts/test/SafeMock.sol | 19 +++++++-----
 4337/src/utils/safe.ts           |  9 ++----
 4337/src/utils/userOp.ts         |  6 ++--
 4 files changed, 33 insertions(+), 52 deletions(-)

diff --git a/4337/contracts/EIP4337Module.sol b/4337/contracts/EIP4337Module.sol
index 326318a82..6d938122b 100644
--- a/4337/contracts/EIP4337Module.sol
+++ b/4337/contracts/EIP4337Module.sol
@@ -6,7 +6,7 @@ import {CompatibilityFallbackHandler} from "@safe-global/safe-contracts/contract
 import {UserOperation, UserOperationLib} from "./UserOperation.sol";
 import {INonceManager} from "./interfaces/ERC4337.sol";
 import {ISafe} from "./interfaces/Safe.sol";
-import "hardhat/console.sol";
+
 
 /// @title EIP4337Module
 abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler {
@@ -24,17 +24,11 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         );
 
     address public immutable supportedEntryPoint;
-    bytes4 public immutable expectedOuterExecutionFunctionId;
-    bytes4 public immutable expectedInnerExecutionFunctionId;
-
-    constructor(
-        address entryPoint,
-        bytes4 outerExecutionFunctionId,
-        bytes4 innerExecutionFunctionId
-    ) {
+    bytes4 public immutable expectedExecutionFunctionId;
+
+    constructor(address entryPoint, bytes4 executionFunctionId) {
         supportedEntryPoint = entryPoint;
-        expectedOuterExecutionFunctionId = outerExecutionFunctionId;
-        expectedInnerExecutionFunctionId = innerExecutionFunctionId;
+        expectedExecutionFunctionId = executionFunctionId;
     }
 
     /// @dev Validates user operation provided by the entry point
@@ -53,7 +47,7 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
 
         // 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(expectedOuterExecutionFunctionId == bytes4(userOp.callData), "Unsupported execution function id");
+        require(expectedExecutionFunctionId == bytes4(userOp.callData), "Unsupported execution function id");
 
         address entryPoint = _msgSender();
         require(entryPoint == supportedEntryPoint, "Unsupported entry point");
@@ -70,23 +64,20 @@ 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 {
+    /// @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");
 
-        // 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(expectedInnerExecutionFunctionId == bytes4(executionData), "Unsupported execution function id");
-
-        (bool outerCallSuccess, bytes memory returnData) = msg.sender.call(executionData);
-        require(returnData.length == 32, "Invalid return data length");
-
-        bool internalCallSuccess = returnData[31] == 0x01;
-        if (!outerCallSuccess || !internalCallSuccess) {
-            revert("Execution failed");
-        }
+        require(ISafe(msg.sender).execTransactionFromModule(to, value, data, operation), "Execution failed");
     }
 
     function domainSeparator() public view returns (bytes32) {
@@ -157,11 +148,5 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
 }
 
 contract Simple4337Module is EIP4337Module {
-    constructor(address entryPoint)
-        EIP4337Module(
-            entryPoint,
-            bytes4(keccak256("executeUserOp(bytes)")),
-            bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)"))
-        )
-    {}
+    constructor(address entryPoint) EIP4337Module(entryPoint, bytes4(keccak256("executeUserOp(address,uint256,bytes,uint8)"))) {}
 }
diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol
index db8452e54..ae3441303 100644
--- a/4337/contracts/test/SafeMock.sol
+++ b/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("executeUserOp(bytes)"));
+        expectedExecutionFunctionId = bytes4(keccak256("executeUserOp(address,uint256,bytes,uint8)"));
     }
 
     /// @dev Validates user operation provided by the entry point
@@ -158,16 +158,19 @@ contract Safe4337Mock is SafeMock {
 
     /// @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 {
+    /// @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");
 
-        // [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);
diff --git a/4337/src/utils/safe.ts b/4337/src/utils/safe.ts
index 236e329c2..3b250caa9 100644
--- a/4337/src/utils/safe.ts
+++ b/4337/src/utils/safe.ts
@@ -14,7 +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 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[])',
@@ -96,14 +96,9 @@ const callInterface = async(provider: providers.Provider, contract: string, meth
 }
 
 const actionCalldata = (action: MetaTransaction): string => {
-    const execTransactionFromModuleCallData = INTERFACES.encodeFunctionData(
-        'execTransactionFromModule',
-        [action.to, action.value, action.data, action.operation]
-    )
-
     return INTERFACES.encodeFunctionData(
         'executeUserOp',
-        [execTransactionFromModuleCallData]
+        [action.to, action.value, action.data, action.operation]
     )
 }
 
diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts
index ff95d7dd2..ecdfd4da2 100644
--- a/4337/src/utils/userOp.ts
+++ b/4337/src/utils/userOp.ts
@@ -90,11 +90,9 @@ export const buildSafeUserOpTransaction = (
   overrides?: Partial<SafeUserOperation>,
 ): 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',
+    'function executeUserOp(address to, uint256 value, bytes calldata data, uint8 operation) external',
   ]
-  const execTransactionFromModuleCallData = new ethersUtils.Interface(abi).encodeFunctionData('execTransactionFromModule', [to, value, data, delegateCall ? 1 : 0])
-  const callData = new ethersUtils.Interface(abi).encodeFunctionData('executeUserOp', [execTransactionFromModuleCallData])
+  const callData = new ethersUtils.Interface(abi).encodeFunctionData('executeUserOp', [to, value, data, delegateCall ? 1 : 0])
 
   return buildSafeUserOp(
     Object.assign(

From 907e84927607d048e395a6d161ca07a2afec97f5 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Tue, 17 Oct 2023 13:01:53 +0200
Subject: [PATCH 3/4] Add a separate execution method that bubbles up the
 revert reason string

---
 4337/contracts/EIP4337Module.sol              | 38 +++++++++++----
 4337/contracts/interfaces/Safe.sol            | 16 +++++++
 4337/contracts/test/SafeMock.sol              | 47 ++++++++++++++++---
 4337/contracts/test/TestEntryPoint.sol        | 22 +++++++--
 4337/contracts/test/TestReverter.sol          |  7 +++
 4337/src/utils/userOp.ts                      |  8 +++-
 .../eip4337/EIP4337ModuleExisting.spec.ts     | 20 ++++++++
 7 files changed, 139 insertions(+), 19 deletions(-)
 create mode 100644 4337/contracts/test/TestReverter.sol

diff --git a/4337/contracts/EIP4337Module.sol b/4337/contracts/EIP4337Module.sol
index 6d938122b..3605170e4 100644
--- a/4337/contracts/EIP4337Module.sol
+++ b/4337/contracts/EIP4337Module.sol
@@ -9,7 +9,7 @@ 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.
@@ -24,11 +24,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
@@ -47,7 +45,10 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
 
         // 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");
+        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");
@@ -80,6 +81,29 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
         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));
     }
@@ -147,6 +171,4 @@ abstract contract EIP4337Module is HandlerContext, CompatibilityFallbackHandler
     }
 }
 
-contract Simple4337Module is EIP4337Module {
-    constructor(address entryPoint) EIP4337Module(entryPoint, bytes4(keccak256("executeUserOp(address,uint256,bytes,uint8)"))) {}
-}
+
diff --git a/4337/contracts/interfaces/Safe.sol b/4337/contracts/interfaces/Safe.sol
index 8508a78c9..f1db26a8f 100644
--- a/4337/contracts/interfaces/Safe.sol
+++ b/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)
diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol
index ae3441303..992944e32 100644
--- a/4337/contracts/test/SafeMock.sol
+++ b/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("executeUserOp(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");
@@ -177,6 +190,28 @@ contract Safe4337Mock is SafeMock {
         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));
     }
diff --git a/4337/contracts/test/TestEntryPoint.sol b/4337/contracts/test/TestEntryPoint.sol
index bdb230196..d66a83f27 100644
--- a/4337/contracts/test/TestEntryPoint.sol
+++ b/4337/contracts/test/TestEntryPoint.sol
@@ -39,11 +39,13 @@ contract SenderCreator {
 contract TestEntryPoint is INonceManager {
     error NotEnoughFunds(uint256 expected, uint256 available);
     error InvalidNonce(uint256 userNonce);
-    event UserOpReverted();
+    event UserOpReverted(bytes reason);
     SenderCreator public immutable senderCreator;
     mapping(address => uint256) balances;
     mapping(address => mapping(uint192 => uint256)) public nonceSequenceNumber;
 
+    uint256 private constant REVERT_REASON_MAX_LEN = 2048;
+
     constructor() {
         senderCreator = new SenderCreator();
     }
@@ -77,9 +79,23 @@ contract TestEntryPoint is INonceManager {
         }
 
         require(gasleft() > userOp.callGasLimit, "Not enough gas for execution");
-        (bool success, ) = 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();
+            emit UserOpReverted(returnData);
+        }
+    }
+
+    function getRevertReason(uint256 maxLen) internal pure returns (string memory errorString) {
+        assembly {
+            let len := returndatasize()
+            if gt(len, maxLen) {
+                len := maxLen
+            }
+            let ptr := mload(0x40)
+            mstore(0x40, add(ptr, len))
+            mstore(ptr, len)
+            returndatacopy(ptr, 0, len)
+            errorString := ptr
         }
     }
 
diff --git a/4337/contracts/test/TestReverter.sol b/4337/contracts/test/TestReverter.sol
new file mode 100644
index 000000000..db74a187b
--- /dev/null
+++ b/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");
+    }
+}
diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts
index ecdfd4da2..61798a884 100644
--- a/4337/src/utils/userOp.ts
+++ b/4337/src/utils/userOp.ts
@@ -87,12 +87,15 @@ export const buildSafeUserOpTransaction = (
   nonce: string,
   entryPoint: string,
   delegateCall?: boolean,
+  bubbleUpRevertReason?: boolean,
   overrides?: Partial<SafeUserOperation>,
 ): SafeUserOperation => {
   const abi = [
     '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('executeUserOp', [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 = ({
diff --git a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts b/4337/test/eip4337/EIP4337ModuleExisting.spec.ts
index b744766af..20fc86572 100644
--- a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts
+++ b/4337/test/eip4337/EIP4337ModuleExisting.spec.ts
@@ -128,5 +128,25 @@ describe('EIP4337Module - Existing Safe', async () => {
 
       expect(emittedRevert).to.be.true
     })
+
+    it('executeUserOpWithErrorString reverts on failure and bubbles up the revert reason', async () => {
+      const { safe, validator, entryPoint } = await setupTests()
+      const reverterContract = await ethers.getContractFactory("TestReverter").then(factory => factory.deploy())
+      const callData = reverterContract.interface.encodeFunctionData("alwaysReverting", [])
+
+      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, reverterContract.address, 0, callData, '0', entryPoint.address, false, true)
+      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.find((l: any) => l.name === "UserOpReverted")
+      const decodedError = ethers.utils.defaultAbiCoder.decode(["string"], `0x${emittedRevert.args.reason.slice(10)}`)
+      expect(decodedError[0]).to.equal("You called a function that always reverts")
+
+    })
   })
 })

From ca698b062648569329da4384d51a8f368d1d2dcd Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Wed, 18 Oct 2023 11:00:47 +0200
Subject: [PATCH 4/4] Fix sample environment variables

---
 4337/.env.sample                       | 13 ++++++++++---
 4337/contracts/EIP4337Module.sol       |  3 ---
 4337/contracts/test/TestEntryPoint.sol | 16 ----------------
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/4337/.env.sample b/4337/.env.sample
index 4b1a6655c..c8f664524 100644
--- a/4337/.env.sample
+++ b/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=
\ No newline at end of file
+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=""
\ No newline at end of file
diff --git a/4337/contracts/EIP4337Module.sol b/4337/contracts/EIP4337Module.sol
index 3605170e4..c9513eef3 100644
--- a/4337/contracts/EIP4337Module.sol
+++ b/4337/contracts/EIP4337Module.sol
@@ -7,7 +7,6 @@ import {UserOperation, UserOperationLib} from "./UserOperation.sol";
 import {INonceManager} from "./interfaces/ERC4337.sol";
 import {ISafe} from "./interfaces/Safe.sol";
 
-
 /// @title EIP4337Module
 contract Simple4337Module is HandlerContext, CompatibilityFallbackHandler {
     using UserOperationLib for UserOperation;
@@ -170,5 +169,3 @@ contract Simple4337Module is HandlerContext, CompatibilityFallbackHandler {
         }
     }
 }
-
-
diff --git a/4337/contracts/test/TestEntryPoint.sol b/4337/contracts/test/TestEntryPoint.sol
index d66a83f27..495d0415a 100644
--- a/4337/contracts/test/TestEntryPoint.sol
+++ b/4337/contracts/test/TestEntryPoint.sol
@@ -44,8 +44,6 @@ contract TestEntryPoint is INonceManager {
     mapping(address => uint256) balances;
     mapping(address => mapping(uint192 => uint256)) public nonceSequenceNumber;
 
-    uint256 private constant REVERT_REASON_MAX_LEN = 2048;
-
     constructor() {
         senderCreator = new SenderCreator();
     }
@@ -85,20 +83,6 @@ contract TestEntryPoint is INonceManager {
         }
     }
 
-    function getRevertReason(uint256 maxLen) internal pure returns (string memory errorString) {
-        assembly {
-            let len := returndatasize()
-            if gt(len, maxLen) {
-                len := maxLen
-            }
-            let ptr := mload(0x40)
-            mstore(0x40, add(ptr, len))
-            mstore(ptr, len)
-            returndatacopy(ptr, 0, len)
-            errorString := ptr
-        }
-    }
-
     function getNonce(address sender, uint192 key) external view override returns (uint256 nonce) {
         return nonceSequenceNumber[sender][key] | (uint256(key) << 64);
     }