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 533402ff7..c9513eef3 100644 --- a/4337/contracts/EIP4337Module.sol +++ b/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)"))) - {} -} 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 1b23e92a8..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("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)); } diff --git a/4337/contracts/test/TestEntryPoint.sol b/4337/contracts/test/TestEntryPoint.sol index b400f8034..495d0415a 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(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) { 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/safe.ts b/4337/src/utils/safe.ts index f05bedbd2..3b250caa9 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(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] ) } diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts index a099365f6..61798a884 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 => { - return { - signer: await signer.getAddress(), - data: await signer._signTypedData({ chainId, verifyingContract: moduleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp), - } +): Promise => { + return { + signer: await signer.getAddress(), + data: await signer._signTypedData({ chainId, verifyingContract: moduleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp), + } } export const buildSafeUserOp = (template: OptionalExceptFor): SafeUserOperation => { @@ -87,12 +87,15 @@ export const buildSafeUserOpTransaction = ( nonce: string, entryPoint: string, delegateCall?: boolean, + bubbleUpRevertReason?: boolean, overrides?: Partial, ): 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 => { 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 => { 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..20fc86572 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,42 @@ 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 + }) + + 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") + + }) }) }) 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')) }) }) })