From 16f86e295fea165393c2615723984d49f0c55f0e Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 10:01:38 +0000 Subject: [PATCH 01/13] Sign Paymaster and Data --- 4337/contracts/Safe4337Module.sol | 125 ++++++++++-------- 4337/contracts/test/SafeMock.sol | 107 +++++++-------- 4337/src/utils/userOp.ts | 66 ++++----- 4337/test/e2e/LocalBundler.spec.ts | 2 - 4337/test/e2e/SingletonSigners.spec.ts | 8 +- .../eip4337/EIP4337ModuleExisting.spec.ts | 14 +- 4337/test/eip4337/EIP4337ModuleNew.spec.ts | 7 - 4337/test/eip4337/ReferenceEntryPoint.spec.ts | 3 - 4337/test/eip4337/Safe4337Mock.spec.ts | 11 +- 4337/test/eip4337/Safe4337Module.spec.ts | 13 +- 10 files changed, 178 insertions(+), 178 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 627d3a71..1796f869 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -25,13 +25,14 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle /** * @notice The keccak256 hash of the EIP-712 SafeOp struct, representing the structure of a User Operation for Safe. * {address} safe - The address of the safe on which the operation is performed. - * {bytes} callData - The bytes representing the data of the function call to be executed. * {uint256} nonce - A unique number associated with the user operation, preventing replay attacks by ensuring each operation is unique. - * {uint256} preVerificationGas - The amount of gas allocated for pre-verification steps before executing the main operation. - * {uint256} verificationGasLimit - The maximum amount of gas allowed for the verification process. + * {bytes} callData - The bytes representing the data of the function call to be executed. * {uint256} callGasLimit - The maximum amount of gas allowed for executing the function call. + * {uint256} verificationGasLimit - The maximum amount of gas allowed for the verification process. + * {uint256} preVerificationGas - The amount of gas allocated for pre-verification steps before executing the main operation. * {uint256} maxFeePerGas - The maximum fee per gas that the user is willing to pay for the transaction. * {uint256} maxPriorityFeePerGas - The maximum priority fee per gas that the user is willing to pay for the transaction. + * {bytes} paymasterAndData - The packed encoding of a paymaster address and its paymaster-specific data for sponsoring the user operation. * {uint48} validAfter - A timestamp representing from when the user operation is valid. * {uint48} validUntil - A timestamp representing until when the user operation is valid, or 0 to indicated "forever". * {address} entryPoint - The address of the entry point that will execute the user operation. @@ -39,7 +40,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle */ bytes32 private constant SAFE_OP_TYPEHASH = keccak256( - "SafeOp(address safe,bytes callData,uint256 nonce,uint256 preVerificationGas,uint256 verificationGasLimit,uint256 callGasLimit,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,uint48 validAfter,uint48 validUntil,address entryPoint)" + "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); address public immutable SUPPORTED_ENTRYPOINT; @@ -126,47 +127,51 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this)); } + // TODO(nlodell): @dev When validating the user operation, the signature timestamps are pre-pended to the signature bytes. + /** * @dev Returns the 32-byte Safe operation hash to be signed by owners. * @param safe Safe address. - * @param callData Call data. * @param nonce Nonce of the operation. - * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). - * @param verificationGasLimit Gas required for verification. + * @param callData Call data. * @param callGasLimit Gas available during the execution of the call. + * @param verificationGasLimit Gas required for verification. + * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). * @param maxFeePerGas Max fee per gas. * @param maxPriorityFeePerGas Max priority fee per gas. + * @param paymasterAndData The paymaster address and data that will sponsor the user operation. * @param validAfter The timestamp the operation is valid from. * @param validUntil The timestamp the operation is valid until. - * @return Operation hash. + * @return operationHash Operation hash. */ function getOperationHash( address safe, - bytes memory callData, uint256 nonce, - uint256 preVerificationGas, - uint256 verificationGasLimit, + bytes memory callData, uint256 callGasLimit, + uint256 verificationGasLimit, + uint256 preVerificationGas, uint256 maxFeePerGas, uint256 maxPriorityFeePerGas, + bytes memory paymasterAndData, uint48 validAfter, uint48 validUntil - ) external view returns (bytes32) { - return - keccak256( - _getOperationData( - safe, - callData, - nonce, - preVerificationGas, - verificationGasLimit, - callGasLimit, - maxFeePerGas, - maxPriorityFeePerGas, - validAfter, - validUntil - ) - ); + ) external view returns (bytes32 operationHash) { + operationHash = keccak256( + _getOperationData( + safe, + nonce, + callData, + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + paymasterAndData, + validAfter, + validUntil + ) + ); } /** @@ -177,14 +182,15 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle function _validateSignatures(UserOperation calldata userOp) internal view returns (uint256 validationData) { (uint48 validAfter, uint48 validUntil, bytes calldata signature) = _splitSignatureData(userOp.signature); bytes memory operationData = _getOperationData( - payable(userOp.sender), - userOp.callData, + userOp.sender, userOp.nonce, - userOp.preVerificationGas, - userOp.verificationGasLimit, + userOp.callData, userOp.callGasLimit, + userOp.verificationGasLimit, + userOp.preVerificationGas, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, + userOp.paymasterAndData, validAfter, validUntil ); @@ -201,46 +207,57 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle /** * @dev Returns the bytes to be hashed and signed by owners. * @param safe Safe address. - * @param callData Call data. * @param nonce Nonce of the operation. - * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). - * @param verificationGasLimit Gas required for verification. + * @param callData Call data. * @param callGasLimit Gas available during the execution of the call. + * @param verificationGasLimit Gas required for verification. + * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). * @param maxFeePerGas Max fee per gas. * @param maxPriorityFeePerGas Max priority fee per gas. + * @param paymasterAndData The paymaster address and data that will sponsor the user operation. * @param validAfter The timestamp the operation is valid from. * @param validUntil The timestamp the operation is valid until. - * @return Operation bytes. + * @return operationData Operation data bytes. */ function _getOperationData( address safe, - bytes memory callData, uint256 nonce, - uint256 preVerificationGas, - uint256 verificationGasLimit, + bytes memory callData, uint256 callGasLimit, + uint256 verificationGasLimit, + uint256 preVerificationGas, uint256 maxFeePerGas, uint256 maxPriorityFeePerGas, + bytes memory paymasterAndData, uint48 validAfter, uint48 validUntil - ) internal view returns (bytes memory) { - bytes32 safeOperationHash = keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - keccak256(callData), - nonce, - preVerificationGas, - verificationGasLimit, - callGasLimit, - maxFeePerGas, - maxPriorityFeePerGas, - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT + ) internal view returns (bytes memory operationData) { + // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent + // user operations from being submitted that do not fully respect the user preferences. The only exception are + // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise + // it can be replaced with a more expensive initialization that would charge the user additional fees. + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + safe, + nonce, + keccak256(callData), + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + keccak256(paymasterAndData), + validAfter, + validUntil, + SUPPORTED_ENTRYPOINT + ) ) ); - return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOperationHash); } /** diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index 6dd62caa..df8da05d 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -102,7 +102,7 @@ contract Safe4337Mock is SafeMock, IAccount { bytes32 private constant SAFE_OP_TYPEHASH = keccak256( - "SafeOp(address safe,bytes callData,uint256 nonce,uint256 preVerificationGas,uint256 verificationGasLimit,uint256 callGasLimit,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,uint48 validAfter,uint48 validUntil,address entryPoint)" + "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); constructor(address entryPoint) SafeMock(entryPoint) {} @@ -171,31 +171,32 @@ contract Safe4337Mock is SafeMock, IAccount { function getOperationHash( address safe, - bytes calldata callData, uint256 nonce, - uint256 preVerificationGas, - uint256 verificationGasLimit, + bytes memory callData, uint256 callGasLimit, + uint256 verificationGasLimit, + uint256 preVerificationGas, uint256 maxFeePerGas, uint256 maxPriorityFeePerGas, + bytes memory paymasterAndData, uint48 validAfter, uint48 validUntil - ) external view returns (bytes32) { - return - keccak256( - _getOperationData( - safe, - callData, - nonce, - preVerificationGas, - verificationGasLimit, - callGasLimit, - maxFeePerGas, - maxPriorityFeePerGas, - validAfter, - validUntil - ) - ); + ) external view returns (bytes32 operationHash) { + operationHash = keccak256( + _getOperationData( + safe, + nonce, + callData, + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + paymasterAndData, + validAfter, + validUntil + ) + ); } function chainId() public view returns (uint256) { @@ -208,14 +209,15 @@ contract Safe4337Mock is SafeMock, IAccount { uint48 validAfter = uint48(bytes6(userOp.signature[:6])); uint48 validUntil = uint48(bytes6(userOp.signature[6:12])); bytes memory operationData = _getOperationData( - payable(userOp.sender), - userOp.callData, + userOp.sender, userOp.nonce, - userOp.preVerificationGas, - userOp.verificationGasLimit, + userOp.callData, userOp.callGasLimit, + userOp.verificationGasLimit, + userOp.preVerificationGas, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, + userOp.paymasterAndData, validAfter, validUntil ); @@ -224,47 +226,40 @@ contract Safe4337Mock is SafeMock, IAccount { checkSignatures(operationHash, operationData, userOp.signature[12:]); } - /// @dev Returns the bytes that are hashed to be signed by owners. - /// @param safe Safe address - /// @param callData Call data - /// @param nonce Nonce of the operation - /// @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification) - /// @param verificationGasLimit Gas required for verification - /// @param callGasLimit Gas available during the execution of the call - /// @param maxFeePerGas Max fee per gas - /// @param maxPriorityFeePerGas Max priority fee per gas - /// @param validAfter The timestamp the operation is valid from. - /// @param validUntil The timestamp the operation is valid until. - /// @return Operation data function _getOperationData( address safe, - bytes calldata callData, uint256 nonce, - uint256 preVerificationGas, - uint256 verificationGasLimit, + bytes memory callData, uint256 callGasLimit, + uint256 verificationGasLimit, + uint256 preVerificationGas, uint256 maxFeePerGas, uint256 maxPriorityFeePerGas, + bytes memory paymasterAndData, uint48 validAfter, uint48 validUntil - ) internal view returns (bytes memory) { - bytes32 safeOperationHash = keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - keccak256(callData), - nonce, - preVerificationGas, - verificationGasLimit, - callGasLimit, - maxFeePerGas, - maxPriorityFeePerGas, - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT + ) internal view returns (bytes memory operationData) { + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + safe, + nonce, + keccak256(callData), + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + keccak256(paymasterAndData), + validAfter, + validUntil, + SUPPORTED_ENTRYPOINT + ) ) ); - - return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOperationHash); } } diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts index e36d525f..e39b966c 100644 --- a/4337/src/utils/userOp.ts +++ b/4337/src/utils/userOp.ts @@ -1,4 +1,4 @@ -import { BigNumberish, Contract, Signer, ethers } from 'ethers' +import { BigNumberish, BytesLike, Contract, Signer, ethers } from 'ethers' import { SafeSignature } from './execution' @@ -21,13 +21,14 @@ export interface UserOperation { export interface SafeUserOperation { safe: string - callData: string - nonce: string - preVerificationGas: string - verificationGasLimit: string - callGasLimit: string - maxFeePerGas: string - maxPriorityFeePerGas: string + nonce: BigNumberish + callData: BytesLike + callGasLimit: BigNumberish + verificationGasLimit: BigNumberish + preVerificationGas: BigNumberish + maxFeePerGas: BigNumberish + maxPriorityFeePerGas: BigNumberish + paymasterAndData: BytesLike validAfter: BigNumberish validUntil: BigNumberish entryPoint: string @@ -36,13 +37,14 @@ export interface SafeUserOperation { export const EIP712_SAFE_OPERATION_TYPE = { SafeOp: [ { type: 'address', name: 'safe' }, - { type: 'bytes', name: 'callData' }, { type: 'uint256', name: 'nonce' }, - { type: 'uint256', name: 'preVerificationGas' }, - { type: 'uint256', name: 'verificationGasLimit' }, + { type: 'bytes', name: 'callData' }, { type: 'uint256', name: 'callGasLimit' }, + { type: 'uint256', name: 'verificationGasLimit' }, + { type: 'uint256', name: 'preVerificationGas' }, { type: 'uint256', name: 'maxFeePerGas' }, { type: 'uint256', name: 'maxPriorityFeePerGas' }, + { type: 'bytes', name: 'paymasterAndData' }, { type: 'uint48', name: 'validAfter' }, { type: 'uint48', name: 'validUntil' }, { type: 'address', name: 'entryPoint' }, @@ -76,15 +78,19 @@ export const buildSafeUserOp = (template: OptionalExceptFor { return { - nonce: ethers.toBeHex(safeOp.nonce), - callData: safeOp.callData || '0x', - verificationGasLimit: ethers.toBeHex(safeOp.verificationGasLimit || '300000'), - preVerificationGas: ethers.toBeHex(safeOp.preVerificationGas || '50000'), - callGasLimit: ethers.toBeHex(safeOp.callGasLimit || '2000000'), - // use same maxFeePerGas and maxPriorityFeePerGas to ease testing prefund validation - // otherwise it's tricky to calculate the prefund because of dynamic parameters like block.basefee - // check UserOperation.sol#gasPrice() - maxFeePerGas: ethers.toBeHex(safeOp.maxFeePerGas || '5000000000'), - maxPriorityFeePerGas: ethers.toBeHex(safeOp.maxPriorityFeePerGas || '1500000000'), - initCode, - paymasterAndData: '0x', sender: safeOp.safe, + initCode, + nonce: ethers.toBeHex(safeOp.nonce), + callData: ethers.hexlify(safeOp.callData), + callGasLimit: ethers.toBeHex(safeOp.callGasLimit), + verificationGasLimit: ethers.toBeHex(safeOp.verificationGasLimit), + preVerificationGas: ethers.toBeHex(safeOp.preVerificationGas), + maxFeePerGas: ethers.toBeHex(safeOp.maxFeePerGas), + maxPriorityFeePerGas: ethers.toBeHex(safeOp.maxPriorityFeePerGas), + paymasterAndData: ethers.hexlify(safeOp.paymasterAndData), signature, } } diff --git a/4337/test/e2e/LocalBundler.spec.ts b/4337/test/e2e/LocalBundler.spec.ts index b1e911b4..6aa86431 100644 --- a/4337/test/e2e/LocalBundler.spec.ts +++ b/4337/test/e2e/LocalBundler.spec.ts @@ -73,7 +73,6 @@ describe('E2E - Local Bundler', () => { validUntil, ) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -107,7 +106,6 @@ describe('E2E - Local Bundler', () => { ) const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: '0x', diff --git a/4337/test/e2e/SingletonSigners.spec.ts b/4337/test/e2e/SingletonSigners.spec.ts index 2102f22f..e336247b 100644 --- a/4337/test/e2e/SingletonSigners.spec.ts +++ b/4337/test/e2e/SingletonSigners.spec.ts @@ -103,13 +103,14 @@ describe('E2E - Singleton Signers', () => { ) const opHash = await validator.getOperationHash( safeOp.safe, - safeOp.callData, safeOp.nonce, - safeOp.preVerificationGas, - safeOp.verificationGasLimit, + safeOp.callData, safeOp.callGasLimit, + safeOp.verificationGasLimit, + safeOp.preVerificationGas, safeOp.maxFeePerGas, safeOp.maxPriorityFeePerGas, + safeOp.paymasterAndData, safeOp.validAfter, safeOp.validUntil, ) @@ -123,7 +124,6 @@ describe('E2E - Singleton Signers', () => { ...customSigners.map(({ key }) => ethers.solidityPacked(['uint256', 'bytes'], [32, ethers.toBeHex(BigInt(opHash) ^ key)])), ]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safeAddress, safeOp, signature, initCode, diff --git a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts b/4337/test/eip4337/EIP4337ModuleExisting.spec.ts index 4fd2b1a3..7692c55d 100644 --- a/4337/test/eip4337/EIP4337ModuleExisting.spec.ts +++ b/4337/test/eip4337/EIP4337ModuleExisting.spec.ts @@ -37,7 +37,7 @@ describe('Safe4337Module - Existing Safe', () => { await entryPoint.getAddress(), ) const signature = buildSignatureBytes([await signHash(user1, ethers.keccak256('0xbaddad42'))]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await expect(entryPoint.executeUserOp(userOp, 0)).to.be.revertedWith('Signature validation failed') expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('1.0')) }) @@ -57,7 +57,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0)) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.5')) }) @@ -77,7 +77,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await entryPoint.executeUserOp(userOp, 0) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.5')) await expect(entryPoint.executeUserOp(userOp, 0)).to.be.revertedWithCustomError(entryPoint, 'InvalidNonce').withArgs(0) @@ -98,7 +98,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await logGas('Execute UserOp with fee payment', entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001'))) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.499999')) }) @@ -118,7 +118,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) const transaction = await entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001')).then((tx) => tx.wait()) const logs = transaction.logs.map((log) => entryPoint.interface.parseLog(log)) @@ -144,7 +144,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await logGas('Execute UserOp without fee payment and bubble up error string', entryPoint.executeUserOp(userOp, 0)) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.5')) }) @@ -168,7 +168,7 @@ describe('Safe4337Module - Existing Safe', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) const transaction = await entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001')).then((tx) => tx.wait()) const logs = transaction.logs.map((log) => entryPoint.interface.parseLog(log)) ?? [] diff --git a/4337/test/eip4337/EIP4337ModuleNew.spec.ts b/4337/test/eip4337/EIP4337ModuleNew.spec.ts index 149e630b..e11a5c62 100644 --- a/4337/test/eip4337/EIP4337ModuleNew.spec.ts +++ b/4337/test/eip4337/EIP4337ModuleNew.spec.ts @@ -53,7 +53,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, user1.address, safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -77,7 +76,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -101,7 +99,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -126,7 +123,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -154,7 +150,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -180,7 +175,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), @@ -212,7 +206,6 @@ describe('Safe4337Module - Newly deployed safe', () => { ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: safe.getInitCode(), diff --git a/4337/test/eip4337/ReferenceEntryPoint.spec.ts b/4337/test/eip4337/ReferenceEntryPoint.spec.ts index a6d1c43c..d6777b72 100644 --- a/4337/test/eip4337/ReferenceEntryPoint.spec.ts +++ b/4337/test/eip4337/ReferenceEntryPoint.spec.ts @@ -79,7 +79,6 @@ describe('Safe4337Module - Reference EntryPoint', () => { ) const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())]) return buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: nonce === 0 ? safe.getInitCode() : '0x', @@ -131,7 +130,6 @@ describe('Safe4337Module - Reference EntryPoint', () => { validUntil, ) return buildUserOperationFromSafeUserOperation({ - safeAddress: safe.address, safeOp, signature, initCode: nonce === 0 ? safe.getInitCode() : '0x', @@ -205,7 +203,6 @@ describe('Safe4337Module - Reference EntryPoint', () => { ], ) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: daughterSafe.address, safeOp, signature, initCode: daughterSafe.getInitCode(), diff --git a/4337/test/eip4337/Safe4337Mock.spec.ts b/4337/test/eip4337/Safe4337Mock.spec.ts index 16094d1b..3af4c4e8 100644 --- a/4337/test/eip4337/Safe4337Mock.spec.ts +++ b/4337/test/eip4337/Safe4337Mock.spec.ts @@ -44,13 +44,14 @@ describe('Safe4337Mock', () => { }) const operationHash = await validator.getOperationHash( safeAddress, - operation.callData, operation.nonce, - operation.preVerificationGas, - operation.verificationGasLimit, + operation.callData, operation.callGasLimit, + operation.verificationGasLimit, + operation.preVerificationGas, operation.maxFeePerGas, operation.maxPriorityFeePerGas, + operation.paymasterAndData, operation.validAfter, operation.validUntil, ) @@ -75,7 +76,7 @@ describe('Safe4337Mock', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0)) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.5')) }) @@ -95,7 +96,7 @@ describe('Safe4337Mock', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user1, safeOpHash)]) - const userOp = buildUserOperationFromSafeUserOperation({ safeAddress: await safe.getAddress(), safeOp, signature }) + const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature }) await logGas('Execute UserOp with fee payment', entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001'))) expect(await ethers.provider.getBalance(await safe.getAddress())).to.be.eq(ethers.parseEther('0.499999')) }) diff --git a/4337/test/eip4337/Safe4337Module.spec.ts b/4337/test/eip4337/Safe4337Module.spec.ts index 54da3e4b..c58e3db4 100644 --- a/4337/test/eip4337/Safe4337Module.spec.ts +++ b/4337/test/eip4337/Safe4337Module.spec.ts @@ -51,14 +51,15 @@ describe('Safe4337Module', () => { validUntil, }) const operationHash = await safeModule.getOperationHash( - safeAddress, - operation.callData, + operation.safe, operation.nonce, - operation.preVerificationGas, - operation.verificationGasLimit, + operation.callData, operation.callGasLimit, + operation.verificationGasLimit, + operation.preVerificationGas, operation.maxFeePerGas, operation.maxPriorityFeePerGas, + operation.paymasterAndData, operation.validAfter, operation.validUntil, ) @@ -82,7 +83,6 @@ describe('Safe4337Module', () => { const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user, safeOpHash)]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: await safeModule.getAddress(), safeOp, signature, }) @@ -109,7 +109,6 @@ describe('Safe4337Module', () => { const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user, safeOpHash)]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: await safeModule.getAddress(), safeOp, signature, }) @@ -127,7 +126,6 @@ describe('Safe4337Module', () => { const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user, safeOpHash)]) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: await safeModule.getAddress(), safeOp, signature, }) @@ -157,7 +155,6 @@ describe('Safe4337Module', () => { const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) const signature = buildSignatureBytes([await signHash(user, safeOpHash)], validAfter, validUntil) const userOp = buildUserOperationFromSafeUserOperation({ - safeAddress: await safeModule.getAddress(), safeOp, signature, }) From dfc8e62927e08ccefbd6537ea0257e1cf1bb76df Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 10:01:52 +0000 Subject: [PATCH 02/13] WIP: SafeOp library --- 4337/contracts/libraries/SafeOp.sol | 134 ++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 4337/contracts/libraries/SafeOp.sol diff --git a/4337/contracts/libraries/SafeOp.sol b/4337/contracts/libraries/SafeOp.sol new file mode 100644 index 00000000..a79150c1 --- /dev/null +++ b/4337/contracts/libraries/SafeOp.sol @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.8.0 <0.9.0; + +library SafeOp { + /** + * @notice The SafeOp struct, representing the an ERC-4337 User Operation for the Safe. + * {address} safe - The address of the safe on which the operation is performed. + * {uint256} nonce - A unique number associated with the user operation, preventing replay attacks by ensuring each operation is unique. + * {bytes} callData - The bytes representing the data of the function call to be executed. + * {uint256} callGasLimit - The maximum amount of gas allowed for executing the function call. + * {uint256} verificationGasLimit - The maximum amount of gas allowed for the verification process. + * {uint256} preVerificationGas - The amount of gas allocated for pre-verification steps before executing the main operation. + * {uint256} maxFeePerGas - The maximum fee per gas that the user is willing to pay for the transaction. + * {uint256} maxPriorityFeePerGas - The maximum priority fee per gas that the user is willing to pay for the transaction. + * {bytes} paymasterAndData - The packed encoding of a paymaster address and its paymaster-specific data for sponsoring the user operation. + * {uint48} validAfter - A timestamp representing from when the user operation is valid. + * {uint48} validUntil - A timestamp representing until when the user operation is valid, or 0 to indicated "forever". + * {address} entryPoint - The address of the entry point that will execute the user operation. + * @dev It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent + * user operations from being submitted that do not fully respect the user preferences. The only exception is the + * `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise it can be + * replaced with a more expensive initialization that would charge the user additional fees. + */ + struct Data { + address safe; + uint256 nonce; + bytes initCode; + bytes callData; + uint256 callGasLimit; + uint256 verificationGasLimit; + uint256 preVerificationGas; + uint256 maxFeePerGas; + uint256 maxPriorityFeePerGas; + bytes paymasterAndData; + uint48 validAfter; + uint48 validUntil; + } + + /** + * @notice The EIP-712 type-hash of the SafeOp struct. + */ + bytes32 internal constant TYPEHASH = + keccak256( + "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" + ); + + /** + * @dev Computes the EIP-712 struct-hash for the Safe operation. + * @param op The Safe operation to encode. + * @return typeHash The Safe operation EIP-712 struct-hash. + */ + function structHash(Data memory op) internal view returns (bytes32 opHash) { + // We use the efficient in-place implementation suggested in the EIP: + // Unfortunately, this means that this assembly is explicitely **not** memory Safe and we lose the posibility + // of using the IR optimizer. + + bytes32 typeHash = TYPEHASH; + bytes32 initCodeHash = keccak256(op.initCode); + bytes32 callDataHash = keccak256(op.callData); + bytes32 paymasterAndDataHash = keccak256(op.paymasterAndData); + + assembly { + // Back up select memory + let temp1 := mload(sub(op, 32)) + let temp2 := mload(add(op, 64)) + let temp2 := mload(add(op, 96)) + + // Write typeHash and sub-hashes + mstore(sub(mail, 32), typeHash) + mstore(add(mail, 64), contentsHash) + + // Compute hash + hash := keccak256(sub(mail, 32), 128) + + // Restore memory + mstore(sub(mail, 32), temp1) + mstore(add(mail, 64), temp2) + } + + opHash = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + safe, + nonce, + keccak256(callData), + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + keccak256(paymasterAndData), + validAfter, + validUntil, + SUPPORTED_ENTRYPOINT + ) + ) + ); + } + + /** + * @dev Returns the bytes to be hashed and signed by Safe owners. + * @param op The Safe operation to encode. + * @param domainSeparator The EIP-712 domain separator. + * @return data Encoded operation data bytes. + */ + function encode(Data memory op, bytes32 domainSeparator) internal view returns (bytes memory data) { + data = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + safe, + nonce, + keccak256(callData), + callGasLimit, + verificationGasLimit, + preVerificationGas, + maxFeePerGas, + maxPriorityFeePerGas, + keccak256(paymasterAndData), + validAfter, + validUntil, + SUPPORTED_ENTRYPOINT + ) + ) + ); + } +} From 3e4034cbf68b3cae248b155d028de3f209109b2d Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 11:31:07 +0000 Subject: [PATCH 03/13] WIP: refactor to sign UserOperation --- 4337/contracts/Safe4337Module.sol | 172 ++++++------------ 4337/contracts/libraries/SafeOp.sol | 134 -------------- 4337/contracts/test/SafeMock.sol | 130 +++++-------- 4337/src/utils/userOp.ts | 15 +- 4337/test/e2e/LocalBundler.spec.ts | 9 +- 4337/test/e2e/SingletonSigners.spec.ts | 19 +- 4337/test/eip4337/EIP4337ModuleNew.spec.ts | 38 +++- 4337/test/eip4337/ReferenceEntryPoint.spec.ts | 29 ++- 4337/test/eip4337/Safe4337Mock.spec.ts | 33 ---- 4337/test/eip4337/Safe4337Module.spec.ts | 30 ++- 10 files changed, 182 insertions(+), 427 deletions(-) delete mode 100644 4337/contracts/libraries/SafeOp.sol diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 1796f869..ac41c390 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -26,6 +26,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @notice The keccak256 hash of the EIP-712 SafeOp struct, representing the structure of a User Operation for Safe. * {address} safe - The address of the safe on which the operation is performed. * {uint256} nonce - A unique number associated with the user operation, preventing replay attacks by ensuring each operation is unique. + * {bytes} initCode - The packed encoding of a factory address and its factory-specific data for creating a new Safe account. * {bytes} callData - The bytes representing the data of the function call to be executed. * {uint256} callGasLimit - The maximum amount of gas allowed for executing the function call. * {uint256} verificationGasLimit - The maximum amount of gas allowed for the verification process. @@ -40,7 +41,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle */ bytes32 private constant SAFE_OP_TYPEHASH = keccak256( - "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" + "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); address public immutable SUPPORTED_ENTRYPOINT; @@ -130,48 +131,13 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // TODO(nlodell): @dev When validating the user operation, the signature timestamps are pre-pended to the signature bytes. /** - * @dev Returns the 32-byte Safe operation hash to be signed by owners. - * @param safe Safe address. - * @param nonce Nonce of the operation. - * @param callData Call data. - * @param callGasLimit Gas available during the execution of the call. - * @param verificationGasLimit Gas required for verification. - * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). - * @param maxFeePerGas Max fee per gas. - * @param maxPriorityFeePerGas Max priority fee per gas. - * @param paymasterAndData The paymaster address and data that will sponsor the user operation. - * @param validAfter The timestamp the operation is valid from. - * @param validUntil The timestamp the operation is valid until. + * @dev Returns the 32-byte Safe operation hash to be signed by owners for the specified ERC-4337 user operation. + * @param userOp The ERC-4337 user operation. * @return operationHash Operation hash. */ - function getOperationHash( - address safe, - uint256 nonce, - bytes memory callData, - uint256 callGasLimit, - uint256 verificationGasLimit, - uint256 preVerificationGas, - uint256 maxFeePerGas, - uint256 maxPriorityFeePerGas, - bytes memory paymasterAndData, - uint48 validAfter, - uint48 validUntil - ) external view returns (bytes32 operationHash) { - operationHash = keccak256( - _getOperationData( - safe, - nonce, - callData, - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - paymasterAndData, - validAfter, - validUntil - ) - ); + function getOperationHash(UserOperation calldata userOp) external view returns (bytes32 operationHash) { + (bytes memory operationData, , , ) = _getSafeOp(userOp); + operationHash = keccak256(operationData); } /** @@ -180,23 +146,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @return validationData An integer indicating the result of the validation. */ function _validateSignatures(UserOperation calldata userOp) internal view returns (uint256 validationData) { - (uint48 validAfter, uint48 validUntil, bytes calldata signature) = _splitSignatureData(userOp.signature); - bytes memory operationData = _getOperationData( - userOp.sender, - userOp.nonce, - userOp.callData, - userOp.callGasLimit, - userOp.verificationGasLimit, - userOp.preVerificationGas, - userOp.maxFeePerGas, - userOp.maxPriorityFeePerGas, - userOp.paymasterAndData, - validAfter, - validUntil - ); + (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); bytes32 operationHash = keccak256(operationData); - try ISafe(payable(userOp.sender)).checkSignatures(operationHash, operationData, signature) { + try ISafe(payable(userOp.sender)).checkSignatures(operationHash, operationData, signatures) { // The timestamps are validated by the entry point, therefore we will not check them again validationData = _packValidationData(false, validUntil, validAfter); } catch { @@ -205,73 +158,56 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } /** - * @dev Returns the bytes to be hashed and signed by owners. - * @param safe Safe address. - * @param nonce Nonce of the operation. - * @param callData Call data. - * @param callGasLimit Gas available during the execution of the call. - * @param verificationGasLimit Gas required for verification. - * @param preVerificationGas Gas required for pre-verification (e.g. for EOA signature verification). - * @param maxFeePerGas Max fee per gas. - * @param maxPriorityFeePerGas Max priority fee per gas. - * @param paymasterAndData The paymaster address and data that will sponsor the user operation. - * @param validAfter The timestamp the operation is valid from. - * @param validUntil The timestamp the operation is valid until. - * @return operationData Operation data bytes. + * @dev Decodes an ERC-4337 user operation and returns ERC-712 Safe operation bytes. + * @param userOp The ERC-4337 user operation. + * @return operationData Encoded operation data bytes. + * @return validAfter The timestamp the user operation is valid from. + * @return validUntil The timestamp the user operation is valid until. + * @return signatures The Safe signatures extracted from the user operation. */ - function _getOperationData( - address safe, - uint256 nonce, - bytes memory callData, - uint256 callGasLimit, - uint256 verificationGasLimit, - uint256 preVerificationGas, - uint256 maxFeePerGas, - uint256 maxPriorityFeePerGas, - bytes memory paymasterAndData, - uint48 validAfter, - uint48 validUntil - ) internal view returns (bytes memory operationData) { + function _getSafeOp( + UserOperation calldata userOp + ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + { + bytes calldata sig = userOp.signature; + validAfter = uint48(bytes6(sig[0:6])); + validUntil = uint48(bytes6(sig[6:12])); + signatures = sig[12:]; + } + // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent // user operations from being submitted that do not fully respect the user preferences. The only exception are // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - nonce, - keccak256(callData), - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - keccak256(paymasterAndData), - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT + { + // Work around "stack too deep" errors. + UserOperation calldata _userOp = userOp; + uint256 _validAfter; + uint256 _validUntil; + + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + _userOp.sender, + _userOp.nonce, + keccak256(_userOp.initCode), + keccak256(_userOp.callData), + _userOp.callGasLimit, + _userOp.verificationGasLimit, + _userOp.preVerificationGas, + _userOp.maxFeePerGas, + _userOp.maxPriorityFeePerGas, + keccak256(_userOp.paymasterAndData), + _validAfter, + _validUntil, + SUPPORTED_ENTRYPOINT + ) ) - ) - ); - } - - /** - * @dev Splits the user operation signature bytes into its parts. - * @param signatureData The user operation signature. - * @return validAfter The timestamp the user operation is valid from. - * @return validUntil The timestamp the user operation is valid until. - * @return signature The actual signature for the Safe user operation. - */ - function _splitSignatureData( - bytes calldata signatureData - ) internal pure returns (uint48 validAfter, uint48 validUntil, bytes calldata signature) { - validAfter = uint48(bytes6(signatureData[0:6])); - validUntil = uint48(bytes6(signatureData[6:12])); - signature = signatureData[12:]; + ); + } } } diff --git a/4337/contracts/libraries/SafeOp.sol b/4337/contracts/libraries/SafeOp.sol deleted file mode 100644 index a79150c1..00000000 --- a/4337/contracts/libraries/SafeOp.sol +++ /dev/null @@ -1,134 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-only -pragma solidity >=0.8.0 <0.9.0; - -library SafeOp { - /** - * @notice The SafeOp struct, representing the an ERC-4337 User Operation for the Safe. - * {address} safe - The address of the safe on which the operation is performed. - * {uint256} nonce - A unique number associated with the user operation, preventing replay attacks by ensuring each operation is unique. - * {bytes} callData - The bytes representing the data of the function call to be executed. - * {uint256} callGasLimit - The maximum amount of gas allowed for executing the function call. - * {uint256} verificationGasLimit - The maximum amount of gas allowed for the verification process. - * {uint256} preVerificationGas - The amount of gas allocated for pre-verification steps before executing the main operation. - * {uint256} maxFeePerGas - The maximum fee per gas that the user is willing to pay for the transaction. - * {uint256} maxPriorityFeePerGas - The maximum priority fee per gas that the user is willing to pay for the transaction. - * {bytes} paymasterAndData - The packed encoding of a paymaster address and its paymaster-specific data for sponsoring the user operation. - * {uint48} validAfter - A timestamp representing from when the user operation is valid. - * {uint48} validUntil - A timestamp representing until when the user operation is valid, or 0 to indicated "forever". - * {address} entryPoint - The address of the entry point that will execute the user operation. - * @dev It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent - * user operations from being submitted that do not fully respect the user preferences. The only exception is the - * `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise it can be - * replaced with a more expensive initialization that would charge the user additional fees. - */ - struct Data { - address safe; - uint256 nonce; - bytes initCode; - bytes callData; - uint256 callGasLimit; - uint256 verificationGasLimit; - uint256 preVerificationGas; - uint256 maxFeePerGas; - uint256 maxPriorityFeePerGas; - bytes paymasterAndData; - uint48 validAfter; - uint48 validUntil; - } - - /** - * @notice The EIP-712 type-hash of the SafeOp struct. - */ - bytes32 internal constant TYPEHASH = - keccak256( - "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" - ); - - /** - * @dev Computes the EIP-712 struct-hash for the Safe operation. - * @param op The Safe operation to encode. - * @return typeHash The Safe operation EIP-712 struct-hash. - */ - function structHash(Data memory op) internal view returns (bytes32 opHash) { - // We use the efficient in-place implementation suggested in the EIP: - // Unfortunately, this means that this assembly is explicitely **not** memory Safe and we lose the posibility - // of using the IR optimizer. - - bytes32 typeHash = TYPEHASH; - bytes32 initCodeHash = keccak256(op.initCode); - bytes32 callDataHash = keccak256(op.callData); - bytes32 paymasterAndDataHash = keccak256(op.paymasterAndData); - - assembly { - // Back up select memory - let temp1 := mload(sub(op, 32)) - let temp2 := mload(add(op, 64)) - let temp2 := mload(add(op, 96)) - - // Write typeHash and sub-hashes - mstore(sub(mail, 32), typeHash) - mstore(add(mail, 64), contentsHash) - - // Compute hash - hash := keccak256(sub(mail, 32), 128) - - // Restore memory - mstore(sub(mail, 32), temp1) - mstore(add(mail, 64), temp2) - } - - opHash = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - nonce, - keccak256(callData), - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - keccak256(paymasterAndData), - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT - ) - ) - ); - } - - /** - * @dev Returns the bytes to be hashed and signed by Safe owners. - * @param op The Safe operation to encode. - * @param domainSeparator The EIP-712 domain separator. - * @return data Encoded operation data bytes. - */ - function encode(Data memory op, bytes32 domainSeparator) internal view returns (bytes memory data) { - data = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - nonce, - keccak256(callData), - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - keccak256(paymasterAndData), - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT - ) - ) - ); - } -} diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index df8da05d..5be0aa92 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -4,6 +4,7 @@ pragma solidity >=0.8.0; import {IAccount} from "@account-abstraction/contracts/interfaces/IAccount.sol"; import {UserOperation, UserOperationLib} from "@account-abstraction/contracts/interfaces/UserOperation.sol"; +import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; contract SafeMock { address public immutable SUPPORTED_ENTRYPOINT; @@ -102,7 +103,7 @@ contract Safe4337Mock is SafeMock, IAccount { bytes32 private constant SAFE_OP_TYPEHASH = keccak256( - "SafeOp(address safe,uint256 nonce,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" + "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); constructor(address entryPoint) SafeMock(entryPoint) {} @@ -121,7 +122,7 @@ contract Safe4337Mock is SafeMock, IAccount { UserOperation calldata userOp, bytes32, uint256 missingAccountFunds - ) external onlySupportedEntryPoint returns (uint256) { + ) external onlySupportedEntryPoint returns (uint256 validationData) { // 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( @@ -129,7 +130,7 @@ contract Safe4337Mock is SafeMock, IAccount { "Unsupported execution function id" ); - _validateSignatures(userOp); + validationData = _validateSignatures(userOp); if (missingAccountFunds != 0) { (bool success, ) = SUPPORTED_ENTRYPOINT.call{value: missingAccountFunds}(""); @@ -169,97 +170,58 @@ contract Safe4337Mock is SafeMock, IAccount { return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this)); } - function getOperationHash( - address safe, - uint256 nonce, - bytes memory callData, - uint256 callGasLimit, - uint256 verificationGasLimit, - uint256 preVerificationGas, - uint256 maxFeePerGas, - uint256 maxPriorityFeePerGas, - bytes memory paymasterAndData, - uint48 validAfter, - uint48 validUntil - ) external view returns (bytes32 operationHash) { - operationHash = keccak256( - _getOperationData( - safe, - nonce, - callData, - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - paymasterAndData, - validAfter, - validUntil - ) - ); - } - function chainId() public view returns (uint256) { return block.chainid; } /// @dev Validates that the user operation is correctly signed. Users methods from Safe contract, reverts if signatures are invalid /// @param userOp User operation struct - function _validateSignatures(UserOperation calldata userOp) internal view { - uint48 validAfter = uint48(bytes6(userOp.signature[:6])); - uint48 validUntil = uint48(bytes6(userOp.signature[6:12])); - bytes memory operationData = _getOperationData( - userOp.sender, - userOp.nonce, - userOp.callData, - userOp.callGasLimit, - userOp.verificationGasLimit, - userOp.preVerificationGas, - userOp.maxFeePerGas, - userOp.maxPriorityFeePerGas, - userOp.paymasterAndData, - validAfter, - validUntil - ); + function _validateSignatures(UserOperation calldata userOp) internal view returns (uint256 validationData) { + (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); bytes32 operationHash = keccak256(operationData); - checkSignatures(operationHash, operationData, userOp.signature[12:]); + checkSignatures(operationHash, operationData, signatures); + validationData = _packValidationData(false, validUntil, validAfter); } - function _getOperationData( - address safe, - uint256 nonce, - bytes memory callData, - uint256 callGasLimit, - uint256 verificationGasLimit, - uint256 preVerificationGas, - uint256 maxFeePerGas, - uint256 maxPriorityFeePerGas, - bytes memory paymasterAndData, - uint48 validAfter, - uint48 validUntil - ) internal view returns (bytes memory operationData) { - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - safe, - nonce, - keccak256(callData), - callGasLimit, - verificationGasLimit, - preVerificationGas, - maxFeePerGas, - maxPriorityFeePerGas, - keccak256(paymasterAndData), - validAfter, - validUntil, - SUPPORTED_ENTRYPOINT + function _getSafeOp( + UserOperation calldata userOp + ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + { + bytes calldata sig = userOp.signature; + validAfter = uint48(bytes6(sig[0:6])); + validUntil = uint48(bytes6(sig[6:12])); + signatures = sig[12:]; + } + + { + UserOperation calldata _userOp = userOp; + uint256 _validAfter; + uint256 _validUntil; + + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + _userOp.sender, + _userOp.nonce, + keccak256(_userOp.initCode), + keccak256(_userOp.callData), + _userOp.callGasLimit, + _userOp.verificationGasLimit, + _userOp.preVerificationGas, + _userOp.maxFeePerGas, + _userOp.maxPriorityFeePerGas, + keccak256(_userOp.paymasterAndData), + _validAfter, + _validUntil, + SUPPORTED_ENTRYPOINT + ) ) - ) - ); + ); + } } } diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts index e39b966c..3e768002 100644 --- a/4337/src/utils/userOp.ts +++ b/4337/src/utils/userOp.ts @@ -22,6 +22,7 @@ export interface UserOperation { export interface SafeUserOperation { safe: string nonce: BigNumberish + initCode: BytesLike callData: BytesLike callGasLimit: BigNumberish verificationGasLimit: BigNumberish @@ -38,6 +39,7 @@ export const EIP712_SAFE_OPERATION_TYPE = { SafeOp: [ { type: 'address', name: 'safe' }, { type: 'uint256', name: 'nonce' }, + { type: 'bytes', name: 'initCode' }, { type: 'bytes', name: 'callData' }, { type: 'uint256', name: 'callGasLimit' }, { type: 'uint256', name: 'verificationGasLimit' }, @@ -78,6 +80,7 @@ export const buildSafeUserOp = (template: OptionalExceptFor, ): SafeUserOperation => { const abi = [ @@ -121,8 +122,6 @@ export const buildSafeUserOpTransaction = ( callData, nonce, entryPoint, - validAfter, - validUntil, }, overrides, ), @@ -139,8 +138,6 @@ export const buildSafeUserOpContractCall = async ( entryPoint: string, delegateCall?: boolean, bubbleUpRevertReason?: boolean, - validAfter?: BigNumberish, - validUntil?: BigNumberish, overrides?: Partial, ): Promise => { const data = contract.interface.encodeFunctionData(method, params) @@ -154,8 +151,6 @@ export const buildSafeUserOpContractCall = async ( entryPoint, delegateCall, bubbleUpRevertReason, - validAfter, - validUntil, overrides, ) } @@ -163,16 +158,14 @@ export const buildSafeUserOpContractCall = async ( export const buildUserOperationFromSafeUserOperation = ({ safeOp, signature, - initCode = '0x', }: { safeOp: SafeUserOperation signature: string - initCode?: string }): UserOperation => { return { sender: safeOp.safe, - initCode, nonce: ethers.toBeHex(safeOp.nonce), + initCode: ethers.hexlify(safeOp.initCode), callData: ethers.hexlify(safeOp.callData), callGasLimit: ethers.toBeHex(safeOp.callGasLimit), verificationGasLimit: ethers.toBeHex(safeOp.verificationGasLimit), diff --git a/4337/test/e2e/LocalBundler.spec.ts b/4337/test/e2e/LocalBundler.spec.ts index 6aa86431..86b89773 100644 --- a/4337/test/e2e/LocalBundler.spec.ts +++ b/4337/test/e2e/LocalBundler.spec.ts @@ -64,8 +64,11 @@ describe('E2E - Local Bundler', () => { await entryPoint.getAddress(), false, false, - validAfter, - validUntil, + { + initCode: safe.getInitCode(), + validAfter, + validUntil, + }, ) const signature = buildSignatureBytes( [await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())], @@ -75,7 +78,6 @@ describe('E2E - Local Bundler', () => { const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await bundler.sendUserOperation(userOp, await entryPoint.getAddress()) @@ -108,7 +110,6 @@ describe('E2E - Local Bundler', () => { const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: '0x', }) await bundler.sendUserOperation(userOp, await entryPoint.getAddress()) diff --git a/4337/test/e2e/SingletonSigners.spec.ts b/4337/test/e2e/SingletonSigners.spec.ts index e336247b..9d95966e 100644 --- a/4337/test/e2e/SingletonSigners.spec.ts +++ b/4337/test/e2e/SingletonSigners.spec.ts @@ -100,19 +100,15 @@ describe('E2E - Singleton Signers', () => { '0x', await entryPoint.getNonce(safeAddress, 0), await entryPoint.getAddress(), + false, + false, + { initCode }, ) const opHash = await validator.getOperationHash( - safeOp.safe, - safeOp.nonce, - safeOp.callData, - safeOp.callGasLimit, - safeOp.verificationGasLimit, - safeOp.preVerificationGas, - safeOp.maxFeePerGas, - safeOp.maxPriorityFeePerGas, - safeOp.paymasterAndData, - safeOp.validAfter, - safeOp.validUntil, + buildUserOperationFromSafeUserOperation({ + safeOp, + signature: buildSignatureBytes([]), + }), ) const signature = ethers.concat([ buildSignatureBytes( @@ -126,7 +122,6 @@ describe('E2E - Singleton Signers', () => { const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode, }) await bundler.sendUserOperation(userOp, await entryPoint.getAddress()) diff --git a/4337/test/eip4337/EIP4337ModuleNew.spec.ts b/4337/test/eip4337/EIP4337ModuleNew.spec.ts index e11a5c62..50b0858b 100644 --- a/4337/test/eip4337/EIP4337ModuleNew.spec.ts +++ b/4337/test/eip4337/EIP4337ModuleNew.spec.ts @@ -50,12 +50,16 @@ describe('Safe4337Module - Newly deployed safe', () => { '0x', '0', await entryPoint.getAddress(), + false, + false, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, user1.address, safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await expect(entryPoint.executeUserOp(userOp, 0)).to.be.revertedWith('Signature validation failed') expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.parseEther('1.0')) @@ -73,12 +77,16 @@ describe('Safe4337Module - Newly deployed safe', () => { '0x', '0', await entryPoint.getAddress(), + false, + false, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0)) expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.parseEther('0.5')) @@ -96,12 +104,16 @@ describe('Safe4337Module - Newly deployed safe', () => { '0x', '0', await entryPoint.getAddress(), + false, + false, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await entryPoint.executeUserOp(userOp, 0) expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.parseEther('0.5')) @@ -120,12 +132,16 @@ describe('Safe4337Module - Newly deployed safe', () => { '0x', '0', await entryPoint.getAddress(), + false, + false, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) const transaction = await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0)) const receipt = await transaction.wait() @@ -147,12 +163,16 @@ describe('Safe4337Module - Newly deployed safe', () => { '0x', '0', await entryPoint.getAddress(), + false, + false, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await logGas('Execute UserOp with fee payment', entryPoint.executeUserOp(userOp, ethers.parseEther('0.000001'))) expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.parseEther('0.499999')) @@ -172,12 +192,14 @@ describe('Safe4337Module - Newly deployed safe', () => { await entryPoint.getAddress(), false, true, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) await logGas( 'Execute UserOp with fee payment and bubble up error string', @@ -203,12 +225,14 @@ describe('Safe4337Module - Newly deployed safe', () => { await entryPoint.getAddress(), false, true, + { + initCode: safe.getInitCode(), + }, ) const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: safe.getInitCode(), }) const transaction = await logGas('Execute UserOp without fee payment', entryPoint.executeUserOp(userOp, 0)) const receipt = await transaction.wait() diff --git a/4337/test/eip4337/ReferenceEntryPoint.spec.ts b/4337/test/eip4337/ReferenceEntryPoint.spec.ts index d6777b72..9241c19d 100644 --- a/4337/test/eip4337/ReferenceEntryPoint.spec.ts +++ b/4337/test/eip4337/ReferenceEntryPoint.spec.ts @@ -76,12 +76,16 @@ describe('Safe4337Module - Reference EntryPoint', () => { '0x', `${nonce}`, await entryPoint.getAddress(), + false, + false, + { + initCode: nonce === 0 ? safe.getInitCode() : '0x', + }, ) const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())]) return buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: nonce === 0 ? safe.getInitCode() : '0x', }) }), ) @@ -121,8 +125,11 @@ describe('Safe4337Module - Reference EntryPoint', () => { await entryPoint.getAddress(), false, false, - validAfter, - validUntil, + { + initCode: nonce === 0 ? safe.getInitCode() : '0x', + validAfter, + validUntil, + }, ) const signature = buildSignatureBytes( [await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())], @@ -132,7 +139,6 @@ describe('Safe4337Module - Reference EntryPoint', () => { return buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: nonce === 0 ? safe.getInitCode() : '0x', }) }), ) @@ -164,7 +170,19 @@ describe('Safe4337Module - Reference EntryPoint', () => { expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance) const transfer = ethers.parseEther('0.1') - const safeOp = buildSafeUserOpTransaction(daughterSafe.address, user.address, transfer, '0x', '0x0', await entryPoint.getAddress()) + const safeOp = buildSafeUserOpTransaction( + daughterSafe.address, + user.address, + transfer, + '0x', + '0x0', + await entryPoint.getAddress(), + false, + false, + { + initCode: daughterSafe.getInitCode(), + }, + ) const opData = calculateSafeOperationData(await validator.getAddress(), safeOp, await chainId()) const signature = ethers.solidityPacked( ['bytes', 'bytes'], @@ -205,7 +223,6 @@ describe('Safe4337Module - Reference EntryPoint', () => { const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, - initCode: daughterSafe.getInitCode(), }) const transaction = await logGas( diff --git a/4337/test/eip4337/Safe4337Mock.spec.ts b/4337/test/eip4337/Safe4337Mock.spec.ts index 3af4c4e8..37f077fe 100644 --- a/4337/test/eip4337/Safe4337Mock.spec.ts +++ b/4337/test/eip4337/Safe4337Mock.spec.ts @@ -27,39 +27,6 @@ describe('Safe4337Mock', () => { } }) - describe('getOperationHash', () => { - it('should correctly calculate EIP-712 hash of the operation', async () => { - const { validator, entryPoint } = await setupTests() - - const safeAddress = ethers.hexlify(ethers.randomBytes(20)) - const validAfter = (await timestamp()) + 10000 - const validUntil = validAfter + 10000000000 - - const operation = buildSafeUserOp({ - safe: safeAddress, - nonce: '0', - entryPoint: await entryPoint.getAddress(), - validAfter, - validUntil, - }) - const operationHash = await validator.getOperationHash( - safeAddress, - operation.nonce, - operation.callData, - operation.callGasLimit, - operation.verificationGasLimit, - operation.preVerificationGas, - operation.maxFeePerGas, - operation.maxPriorityFeePerGas, - operation.paymasterAndData, - operation.validAfter, - operation.validUntil, - ) - - expect(operationHash).to.equal(calculateSafeOperationHash(await validator.getAddress(), operation, await chainId())) - }) - }) - describe('executeUserOp', () => { it('should execute contract calls without fee', async () => { const { user1, safe, validator, entryPoint } = await setupTests() diff --git a/4337/test/eip4337/Safe4337Module.spec.ts b/4337/test/eip4337/Safe4337Module.spec.ts index c58e3db4..69d918bc 100644 --- a/4337/test/eip4337/Safe4337Module.spec.ts +++ b/4337/test/eip4337/Safe4337Module.spec.ts @@ -36,35 +36,27 @@ describe('Safe4337Module', () => { }) describe('getOperationHash', () => { - it('should correctly calculate EIP-712 hash of the operation', async () => { + it.only('should correctly calculate EIP-712 hash of the operation', async () => { const { validator, safeModule, entryPoint } = await setupTests() const safeAddress = ethers.hexlify(ethers.randomBytes(20)) const validAfter = (await timestamp()) + 10000 const validUntil = validAfter + 10000000000 - const operation = buildSafeUserOp({ + const safeOp = buildSafeUserOp({ safe: safeAddress, nonce: '0', entryPoint: await entryPoint.getAddress(), validAfter, validUntil, }) - const operationHash = await safeModule.getOperationHash( - operation.safe, - operation.nonce, - operation.callData, - operation.callGasLimit, - operation.verificationGasLimit, - operation.preVerificationGas, - operation.maxFeePerGas, - operation.maxPriorityFeePerGas, - operation.paymasterAndData, - operation.validAfter, - operation.validUntil, - ) + const userOp = buildUserOperationFromSafeUserOperation({ + safeOp, + signature: buildSignatureBytes([]), + }) + const operationHash = await safeModule.getOperationHash(userOp) - expect(operationHash).to.equal(calculateSafeOperationHash(await validator.getAddress(), operation, await chainId())) + expect(operationHash).to.equal(calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId())) }) }) @@ -148,8 +140,10 @@ describe('Safe4337Module', () => { await entryPoint.getAddress(), false, false, - validAfter, - validUntil, + { + validAfter, + validUntil, + }, ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) From b73103e1e29e812ae0b2c18da38192813c70eba4 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 11:46:08 +0000 Subject: [PATCH 04/13] WIP: test passing, but not coverage --- 4337/contracts/Safe4337Module.sol | 6 +++--- 4337/src/utils/execution.ts | 7 ++----- 4337/src/utils/userOp.ts | 2 +- 4337/test/e2e/LocalBundler.spec.ts | 6 +----- 4337/test/eip4337/ReferenceEntryPoint.spec.ts | 6 +----- 4337/test/eip4337/Safe4337Mock.spec.ts | 9 ++------- 4337/test/eip4337/Safe4337Module.spec.ts | 6 +++--- 7 files changed, 13 insertions(+), 29 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index ac41c390..694adecf 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -180,10 +180,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. { - // Work around "stack too deep" errors. + // User assembly to work around "stack too deep" errors. UserOperation calldata _userOp = userOp; - uint256 _validAfter; - uint256 _validUntil; + uint256 _validAfter = validAfter; + uint256 _validUntil = validUntil; operationData = abi.encodePacked( bytes1(0x19), diff --git a/4337/src/utils/execution.ts b/4337/src/utils/execution.ts index 712c0aa0..4dd101e2 100644 --- a/4337/src/utils/execution.ts +++ b/4337/src/utils/execution.ts @@ -41,12 +41,9 @@ export const signHash = async (signer: Signer, hash: string): Promise { +export const buildSignatureBytes = (signatures: SafeSignature[]): string => { signatures.sort((left, right) => left.signer.toLowerCase().localeCompare(right.signer.toLowerCase())) - const signatureBytes = ethers.concat(signatures.map((signature) => signature.data)) - const signatureWithTimestamps = ethers.solidityPacked(['uint48', 'uint48', 'bytes'], [validAfter ?? 0, validUntil ?? 0, signatureBytes]) - - return signatureWithTimestamps + return ethers.concat(signatures.map((signature) => signature.data)) } export const logGas = async (message: string, tx: Promise, skip?: boolean): Promise => { diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts index 3e768002..e52e7bea 100644 --- a/4337/src/utils/userOp.ts +++ b/4337/src/utils/userOp.ts @@ -173,7 +173,7 @@ export const buildUserOperationFromSafeUserOperation = ({ maxFeePerGas: ethers.toBeHex(safeOp.maxFeePerGas), maxPriorityFeePerGas: ethers.toBeHex(safeOp.maxPriorityFeePerGas), paymasterAndData: ethers.hexlify(safeOp.paymasterAndData), - signature, + signature: ethers.solidityPacked(['uint48', 'uint48', 'bytes'], [safeOp.validAfter, safeOp.validUntil, signature]), } } diff --git a/4337/test/e2e/LocalBundler.spec.ts b/4337/test/e2e/LocalBundler.spec.ts index 86b89773..901e820b 100644 --- a/4337/test/e2e/LocalBundler.spec.ts +++ b/4337/test/e2e/LocalBundler.spec.ts @@ -70,11 +70,7 @@ describe('E2E - Local Bundler', () => { validUntil, }, ) - const signature = buildSignatureBytes( - [await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())], - validAfter, - validUntil, - ) + const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, diff --git a/4337/test/eip4337/ReferenceEntryPoint.spec.ts b/4337/test/eip4337/ReferenceEntryPoint.spec.ts index 9241c19d..93622fef 100644 --- a/4337/test/eip4337/ReferenceEntryPoint.spec.ts +++ b/4337/test/eip4337/ReferenceEntryPoint.spec.ts @@ -131,11 +131,7 @@ describe('Safe4337Module - Reference EntryPoint', () => { validUntil, }, ) - const signature = buildSignatureBytes( - [await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())], - validAfter, - validUntil, - ) + const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())]) return buildUserOperationFromSafeUserOperation({ safeOp, signature, diff --git a/4337/test/eip4337/Safe4337Mock.spec.ts b/4337/test/eip4337/Safe4337Mock.spec.ts index 37f077fe..9f3bcee6 100644 --- a/4337/test/eip4337/Safe4337Mock.spec.ts +++ b/4337/test/eip4337/Safe4337Mock.spec.ts @@ -2,13 +2,8 @@ import { expect } from 'chai' import { deployments, ethers } from 'hardhat' import { getEntryPoint, get4337TestSafe } from '../utils/setup' import { buildSignatureBytes, signHash, logGas } from '../../src/utils/execution' -import { - buildSafeUserOp, - buildSafeUserOpTransaction, - buildUserOperationFromSafeUserOperation, - calculateSafeOperationHash, -} from '../../src/utils/userOp' -import { chainId, timestamp } from '../utils/encoding' +import { buildSafeUserOpTransaction, buildUserOperationFromSafeUserOperation, calculateSafeOperationHash } from '../../src/utils/userOp' +import { chainId } from '../utils/encoding' describe('Safe4337Mock', () => { const setupTests = deployments.createFixture(async ({ deployments }) => { diff --git a/4337/test/eip4337/Safe4337Module.spec.ts b/4337/test/eip4337/Safe4337Module.spec.ts index 69d918bc..df0cf1f3 100644 --- a/4337/test/eip4337/Safe4337Module.spec.ts +++ b/4337/test/eip4337/Safe4337Module.spec.ts @@ -36,7 +36,7 @@ describe('Safe4337Module', () => { }) describe('getOperationHash', () => { - it.only('should correctly calculate EIP-712 hash of the operation', async () => { + it('should correctly calculate EIP-712 hash of the operation', async () => { const { validator, safeModule, entryPoint } = await setupTests() const safeAddress = ethers.hexlify(ethers.randomBytes(20)) @@ -52,7 +52,7 @@ describe('Safe4337Module', () => { }) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, - signature: buildSignatureBytes([]), + signature: '0x', }) const operationHash = await safeModule.getOperationHash(userOp) @@ -147,7 +147,7 @@ describe('Safe4337Module', () => { ) const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId()) - const signature = buildSignatureBytes([await signHash(user, safeOpHash)], validAfter, validUntil) + const signature = buildSignatureBytes([await signHash(user, safeOpHash)]) const userOp = buildUserOperationFromSafeUserOperation({ safeOp, signature, From 6fccb45fc630c616869d6dd928f51bfca29faeff Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 12:24:19 +0000 Subject: [PATCH 05/13] WIP: Assembly to work around stack too deep --- 4337/contracts/Safe4337Module.sol | 46 ++++++++++++------------ 4337/contracts/test/SafeMock.sol | 60 ++++++++++++++++--------------- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 694adecf..401d1232 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -180,33 +180,35 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. { - // User assembly to work around "stack too deep" errors. - UserOperation calldata _userOp = userOp; - uint256 _validAfter = validAfter; - uint256 _validUntil = validUntil; + bytes32 structHash; + bytes32 typeHash = SAFE_OP_TYPEHASH; + bytes32 initCodeHash = keccak256(userOp.initCode); + bytes32 callDataHash = keccak256(userOp.callData); + bytes32 paymasterAndDataHash = keccak256(userOp.paymasterAndData); + address entryPoint = SUPPORTED_ENTRYPOINT; + + // Use assembly to work around Solidity "stack too deep" errors. + // solhint-disable-next-line no-inline-assembly + assembly ("memory-safe") { + let ptr := mload(0x40) + + mstore(ptr, typeHash) + calldatacopy(add(ptr, 32), userOp, 384) + mstore(add(ptr, 96), initCodeHash) + mstore(add(ptr, 128), callDataHash) + mstore(add(ptr, 320), paymasterAndDataHash) + mstore(add(ptr, 352), validAfter) + mstore(add(ptr, 384), validUntil) + mstore(add(ptr, 416), entryPoint) + + structHash:= keccak256(ptr, 448) + } operationData = abi.encodePacked( bytes1(0x19), bytes1(0x01), domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - _userOp.sender, - _userOp.nonce, - keccak256(_userOp.initCode), - keccak256(_userOp.callData), - _userOp.callGasLimit, - _userOp.verificationGasLimit, - _userOp.preVerificationGas, - _userOp.maxFeePerGas, - _userOp.maxPriorityFeePerGas, - keccak256(_userOp.paymasterAndData), - _validAfter, - _validUntil, - SUPPORTED_ENTRYPOINT - ) - ) + structHash ); } } diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index 5be0aa92..04793b78 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -194,34 +194,36 @@ contract Safe4337Mock is SafeMock, IAccount { signatures = sig[12:]; } - { - UserOperation calldata _userOp = userOp; - uint256 _validAfter; - uint256 _validUntil; - - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - _userOp.sender, - _userOp.nonce, - keccak256(_userOp.initCode), - keccak256(_userOp.callData), - _userOp.callGasLimit, - _userOp.verificationGasLimit, - _userOp.preVerificationGas, - _userOp.maxFeePerGas, - _userOp.maxPriorityFeePerGas, - keccak256(_userOp.paymasterAndData), - _validAfter, - _validUntil, - SUPPORTED_ENTRYPOINT - ) - ) - ); - } + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + _getSafeOpStructHash(validAfter, validUntil, userOp) + ); + } + + function _getSafeOpStructHash( + uint48 validAfter, + uint48 validUntil, + UserOperation calldata userOp + ) private view returns (bytes32 structHash) { + structHash = keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + userOp.sender, + userOp.nonce, + keccak256(userOp.initCode), + keccak256(userOp.callData), + userOp.callGasLimit, + userOp.verificationGasLimit, + userOp.preVerificationGas, + userOp.maxFeePerGas, + userOp.maxPriorityFeePerGas, + keccak256(userOp.paymasterAndData), + // validAfter, + // validUntil, + SUPPORTED_ENTRYPOINT + ) + ); } } From b3e1785548477343f6c2a9f048585d6bf20d6100 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 13:18:29 +0000 Subject: [PATCH 06/13] Implement Encoding with Assembly --- 4337/contracts/Safe4337Module.sol | 89 +++++++++++++++++++++++++++++++ 4337/contracts/test/SafeMock.sol | 61 ++++++++++----------- 2 files changed, 120 insertions(+), 30 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 401d1232..537f0af6 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -212,4 +212,93 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle ); } } + + /* + function _getSafeOpLessAssembly( + UserOperation calldata userOp + ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + { + bytes calldata sig = userOp.signature; + validAfter = uint48(bytes6(sig[0:6])); + validUntil = uint48(bytes6(sig[6:12])); + signatures = sig[12:]; + } + + // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent + // user operations from being submitted that do not fully respect the user preferences. The only exception are + // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise + // it can be replaced with a more expensive initialization that would charge the user additional fees. + { + bytes32[14] memory structData; + unchecked { + structData[0] = SAFE_OP_TYPEHASH; + structData[1] = bytes32(uint256(uint160(userOp.sender))); + structData[2] = bytes32(userOp.nonce); + structData[3] = keccak256(userOp.initCode); + structData[4] = keccak256(userOp.callData); + structData[5] = bytes32(userOp.callGasLimit); + structData[6] = bytes32(userOp.verificationGasLimit); + structData[7] = bytes32(userOp.preVerificationGas); + structData[8] = bytes32(userOp.maxFeePerGas); + structData[9] = bytes32(userOp.maxPriorityFeePerGas); + structData[10] = keccak256(userOp.paymasterAndData); + structData[11] = bytes32(uint256(validAfter)); + structData[12] = bytes32(uint256(validUntil)); + structData[13] = bytes32(uint256(uint160(SUPPORTED_ENTRYPOINT))); + } + + bytes32 structHash; + assembly ("memory-safe") { + structHash := keccak256(structData, 448) + } + + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + structHash + ); + } + } + + function _getSafeOpThisShouldWork( + UserOperation calldata userOp + ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + { + bytes calldata sig = userOp.signature; + validAfter = uint48(bytes6(sig[0:6])); + validUntil = uint48(bytes6(sig[6:12])); + signatures = sig[12:]; + } + + { + UserOperation calldata _userOp = userOp; + uint48 _validAfter = validAfter; + uint48 _validUntil = validUntil; + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + keccak256( + abi.encode( + SAFE_OP_TYPEHASH, + _userOp.sender, + _userOp.nonce, + keccak256(_userOp.initCode), + keccak256(_userOp.callData), + _userOp.callGasLimit, + _userOp.verificationGasLimit, + _userOp.preVerificationGas, + _userOp.maxFeePerGas, + _userOp.maxPriorityFeePerGas, + keccak256(_userOp.paymasterAndData), + _validAfter, + _validUntil, + SUPPORTED_ENTRYPOINT + ) + ) + ); + } + } + */ } diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index 04793b78..a805cfe8 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -194,36 +194,37 @@ contract Safe4337Mock is SafeMock, IAccount { signatures = sig[12:]; } - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - _getSafeOpStructHash(validAfter, validUntil, userOp) - ); - } + { + bytes32 structHash; + bytes32 typeHash = SAFE_OP_TYPEHASH; + bytes32 initCodeHash = keccak256(userOp.initCode); + bytes32 callDataHash = keccak256(userOp.callData); + bytes32 paymasterAndDataHash = keccak256(userOp.paymasterAndData); + address entryPoint = SUPPORTED_ENTRYPOINT; + + // Use assembly to work around Solidity "stack too deep" errors. + // solhint-disable-next-line no-inline-assembly + assembly ("memory-safe") { + let ptr := mload(0x40) + + mstore(ptr, typeHash) + calldatacopy(add(ptr, 32), userOp, 384) + mstore(add(ptr, 96), initCodeHash) + mstore(add(ptr, 128), callDataHash) + mstore(add(ptr, 320), paymasterAndDataHash) + mstore(add(ptr, 352), validAfter) + mstore(add(ptr, 384), validUntil) + mstore(add(ptr, 416), entryPoint) + + structHash:= keccak256(ptr, 448) + } - function _getSafeOpStructHash( - uint48 validAfter, - uint48 validUntil, - UserOperation calldata userOp - ) private view returns (bytes32 structHash) { - structHash = keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - userOp.sender, - userOp.nonce, - keccak256(userOp.initCode), - keccak256(userOp.callData), - userOp.callGasLimit, - userOp.verificationGasLimit, - userOp.preVerificationGas, - userOp.maxFeePerGas, - userOp.maxPriorityFeePerGas, - keccak256(userOp.paymasterAndData), - // validAfter, - // validUntil, - SUPPORTED_ENTRYPOINT - ) - ); + operationData = abi.encodePacked( + bytes1(0x19), + bytes1(0x01), + domainSeparator(), + structHash + ); + } } } From 5b258617918b5bbb45e38b166e921aef81a9e141 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 15:49:36 +0000 Subject: [PATCH 07/13] Use memory struct for manual encoding and document it --- 4337/contracts/Safe4337Module.sol | 170 +++++++++--------------------- 4337/contracts/test/SafeMock.sol | 99 ++++++++++------- 4337/src/deploy/mock.ts | 2 +- 3 files changed, 115 insertions(+), 156 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 537f0af6..646578ab 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -20,10 +20,33 @@ import {ISafe} from "./interfaces/Safe.sol"; * - Replay protection is handled by the entry point. */ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandler { + /** + * @dev A structure used internally for computing the EIP-712 signing message for a Safe operation. + */ + struct SafeOpFields { + bytes32 typeHash; + address safe; + uint256 nonce; + bytes32 initCodeHash; + bytes32 callDataHash; + uint256 callGasLimit; + uint256 verificationGasLimit; + uint256 preVerificationGas; + uint256 maxFeePerGas; + uint256 maxPriorityFeePerGas; + bytes32 paymasterAndDataHash; + uint48 validAfter; + uint48 validUntil; + address entryPoint; + } + + /** + * @notice The EIP-712 type-hash for the domain separator used for verifying Safe operation signatures. + */ bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"); /** - * @notice The keccak256 hash of the EIP-712 SafeOp struct, representing the structure of a User Operation for Safe. + * @notice The EIP-712 type-hash for a SafeOp, representing the structure of a User Operation for the Safe. * {address} safe - The address of the safe on which the operation is performed. * {uint256} nonce - A unique number associated with the user operation, preventing replay attacks by ensuring each operation is unique. * {bytes} initCode - The packed encoding of a factory address and its factory-specific data for creating a new Safe account. @@ -44,6 +67,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); + /** + * @notice The EIP-712 type-hash for the domain separator used for verifying Safe operation signatures. + */ address public immutable SUPPORTED_ENTRYPOINT; constructor(address entryPoint) { @@ -122,6 +148,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } /** + * @notice Computes the 32-byte domain separator used in EIP-712 signature verification for Safe operations. * @return The EIP-712 domain separator hash for this contract. */ function domainSeparator() public view returns (bytes32) { @@ -131,7 +158,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // TODO(nlodell): @dev When validating the user operation, the signature timestamps are pre-pended to the signature bytes. /** - * @dev Returns the 32-byte Safe operation hash to be signed by owners for the specified ERC-4337 user operation. + * @notice Returns the 32-byte Safe operation hash to be signed by owners for the specified ERC-4337 user operation. * @param userOp The ERC-4337 user operation. * @return operationHash Operation hash. */ @@ -147,9 +174,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle */ function _validateSignatures(UserOperation calldata userOp) internal view returns (uint256 validationData) { (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); - bytes32 operationHash = keccak256(operationData); - - try ISafe(payable(userOp.sender)).checkSignatures(operationHash, operationData, signatures) { + try ISafe(payable(userOp.sender)).checkSignatures(keccak256(operationData), operationData, signatures) { // The timestamps are validated by the entry point, therefore we will not check them again validationData = _packValidationData(false, validUntil, validAfter); } catch { @@ -180,125 +205,32 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. { - bytes32 structHash; - bytes32 typeHash = SAFE_OP_TYPEHASH; - bytes32 initCodeHash = keccak256(userOp.initCode); - bytes32 callDataHash = keccak256(userOp.callData); - bytes32 paymasterAndDataHash = keccak256(userOp.paymasterAndData); - address entryPoint = SUPPORTED_ENTRYPOINT; - - // Use assembly to work around Solidity "stack too deep" errors. - // solhint-disable-next-line no-inline-assembly - assembly ("memory-safe") { - let ptr := mload(0x40) - - mstore(ptr, typeHash) - calldatacopy(add(ptr, 32), userOp, 384) - mstore(add(ptr, 96), initCodeHash) - mstore(add(ptr, 128), callDataHash) - mstore(add(ptr, 320), paymasterAndDataHash) - mstore(add(ptr, 352), validAfter) - mstore(add(ptr, 384), validUntil) - mstore(add(ptr, 416), entryPoint) - - structHash:= keccak256(ptr, 448) - } - - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - structHash - ); - } - } - - /* - function _getSafeOpLessAssembly( - UserOperation calldata userOp - ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { - { - bytes calldata sig = userOp.signature; - validAfter = uint48(bytes6(sig[0:6])); - validUntil = uint48(bytes6(sig[6:12])); - signatures = sig[12:]; - } - - // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent - // user operations from being submitted that do not fully respect the user preferences. The only exception are - // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise - // it can be replaced with a more expensive initialization that would charge the user additional fees. - { - bytes32[14] memory structData; - unchecked { - structData[0] = SAFE_OP_TYPEHASH; - structData[1] = bytes32(uint256(uint160(userOp.sender))); - structData[2] = bytes32(userOp.nonce); - structData[3] = keccak256(userOp.initCode); - structData[4] = keccak256(userOp.callData); - structData[5] = bytes32(userOp.callGasLimit); - structData[6] = bytes32(userOp.verificationGasLimit); - structData[7] = bytes32(userOp.preVerificationGas); - structData[8] = bytes32(userOp.maxFeePerGas); - structData[9] = bytes32(userOp.maxPriorityFeePerGas); - structData[10] = keccak256(userOp.paymasterAndData); - structData[11] = bytes32(uint256(validAfter)); - structData[12] = bytes32(uint256(validUntil)); - structData[13] = bytes32(uint256(uint160(SUPPORTED_ENTRYPOINT))); - } + // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually + // encode the `SafeOp` fields into a memory `struct`. + SafeOpFields memory fields = SafeOpFields({ + typeHash: SAFE_OP_TYPEHASH, + safe: userOp.sender, + nonce: userOp.nonce, + initCodeHash: keccak256(userOp.initCode), + callDataHash: keccak256(userOp.callData), + callGasLimit: userOp.callGasLimit, + verificationGasLimit: userOp.verificationGasLimit, + preVerificationGas: userOp.preVerificationGas, + maxFeePerGas: userOp.maxFeePerGas, + maxPriorityFeePerGas: userOp.maxPriorityFeePerGas, + paymasterAndDataHash: keccak256(userOp.paymasterAndData), + validAfter: validAfter, + validUntil: validUntil, + entryPoint: SUPPORTED_ENTRYPOINT + }); bytes32 structHash; + // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - structHash := keccak256(structData, 448) + structHash := keccak256(fields, 448) } - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - structHash - ); - } - } - - function _getSafeOpThisShouldWork( - UserOperation calldata userOp - ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { - { - bytes calldata sig = userOp.signature; - validAfter = uint48(bytes6(sig[0:6])); - validUntil = uint48(bytes6(sig[6:12])); - signatures = sig[12:]; - } - - { - UserOperation calldata _userOp = userOp; - uint48 _validAfter = validAfter; - uint48 _validUntil = validUntil; - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - keccak256( - abi.encode( - SAFE_OP_TYPEHASH, - _userOp.sender, - _userOp.nonce, - keccak256(_userOp.initCode), - keccak256(_userOp.callData), - _userOp.callGasLimit, - _userOp.verificationGasLimit, - _userOp.preVerificationGas, - _userOp.maxFeePerGas, - _userOp.maxPriorityFeePerGas, - keccak256(_userOp.paymasterAndData), - _validAfter, - _validUntil, - SUPPORTED_ENTRYPOINT - ) - ) - ); + operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); } } - */ } diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index a805cfe8..fc7a123b 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -7,16 +7,13 @@ import {UserOperation, UserOperationLib} from "@account-abstraction/contracts/in import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; contract SafeMock { - address public immutable SUPPORTED_ENTRYPOINT; - address public singleton; address public owner; address public fallbackHandler; mapping(address => bool) public modules; - constructor(address entryPoint) { + constructor() { owner = msg.sender; - SUPPORTED_ENTRYPOINT = entryPoint; } function setup(address _fallbackHandler, address _module) public virtual { @@ -24,7 +21,6 @@ contract SafeMock { owner = msg.sender; fallbackHandler = _fallbackHandler; modules[_module] = true; - modules[SUPPORTED_ENTRYPOINT] = true; } function _signatureSplit(bytes memory signature) internal pure returns (uint8 v, bytes32 r, bytes32 s) { @@ -99,6 +95,24 @@ contract SafeMock { contract Safe4337Mock is SafeMock, IAccount { using UserOperationLib for UserOperation; + + struct SafeOpFields { + bytes32 typeHash; + address safe; + uint256 nonce; + bytes32 initCodeHash; + bytes32 callDataHash; + uint256 callGasLimit; + uint256 verificationGasLimit; + uint256 preVerificationGas; + uint256 maxFeePerGas; + uint256 maxPriorityFeePerGas; + bytes32 paymasterAndDataHash; + uint48 validAfter; + uint48 validUntil; + address entryPoint; + } + bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"); bytes32 private constant SAFE_OP_TYPEHASH = @@ -106,7 +120,11 @@ contract Safe4337Mock is SafeMock, IAccount { "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); - constructor(address entryPoint) SafeMock(entryPoint) {} + address public immutable SUPPORTED_ENTRYPOINT; + + constructor(address entryPoint) { + SUPPORTED_ENTRYPOINT = entryPoint; + } /** * @notice Validates the call is initiated by the entry point. @@ -174,16 +192,25 @@ contract Safe4337Mock is SafeMock, IAccount { return block.chainid; } - /// @dev Validates that the user operation is correctly signed. Users methods from Safe contract, reverts if signatures are invalid - /// @param userOp User operation struct + /** + * @dev Validates that the user operation is correctly signed. Reverts if signatures are invalid. + * @param userOp User operation struct. + * @return validationData An integer indicating the result of the validation. + */ function _validateSignatures(UserOperation calldata userOp) internal view returns (uint256 validationData) { (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); - bytes32 operationHash = keccak256(operationData); - - checkSignatures(operationHash, operationData, signatures); + checkSignatures(keccak256(operationData), operationData, signatures); validationData = _packValidationData(false, validUntil, validAfter); } + /** + * @dev Decodes an ERC-4337 user operation and returns ERC-712 Safe operation bytes. + * @param userOp The ERC-4337 user operation. + * @return operationData Encoded operation data bytes. + * @return validAfter The timestamp the user operation is valid from. + * @return validUntil The timestamp the user operation is valid until. + * @return signatures The Safe signatures extracted from the user operation. + */ function _getSafeOp( UserOperation calldata userOp ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { @@ -194,37 +221,37 @@ contract Safe4337Mock is SafeMock, IAccount { signatures = sig[12:]; } + // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent + // user operations from being submitted that do not fully respect the user preferences. The only exception are + // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise + // it can be replaced with a more expensive initialization that would charge the user additional fees. { - bytes32 structHash; - bytes32 typeHash = SAFE_OP_TYPEHASH; - bytes32 initCodeHash = keccak256(userOp.initCode); - bytes32 callDataHash = keccak256(userOp.callData); - bytes32 paymasterAndDataHash = keccak256(userOp.paymasterAndData); - address entryPoint = SUPPORTED_ENTRYPOINT; + // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually + // encode the `SafeOp` fields into a memory `struct`. + SafeOpFields memory fields = SafeOpFields({ + typeHash: SAFE_OP_TYPEHASH, + safe: userOp.sender, + nonce: userOp.nonce, + initCodeHash: keccak256(userOp.initCode), + callDataHash: keccak256(userOp.callData), + callGasLimit: userOp.callGasLimit, + verificationGasLimit: userOp.verificationGasLimit, + preVerificationGas: userOp.preVerificationGas, + maxFeePerGas: userOp.maxFeePerGas, + maxPriorityFeePerGas: userOp.maxPriorityFeePerGas, + paymasterAndDataHash: keccak256(userOp.paymasterAndData), + validAfter: validAfter, + validUntil: validUntil, + entryPoint: SUPPORTED_ENTRYPOINT + }); - // Use assembly to work around Solidity "stack too deep" errors. + bytes32 structHash; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - let ptr := mload(0x40) - - mstore(ptr, typeHash) - calldatacopy(add(ptr, 32), userOp, 384) - mstore(add(ptr, 96), initCodeHash) - mstore(add(ptr, 128), callDataHash) - mstore(add(ptr, 320), paymasterAndDataHash) - mstore(add(ptr, 352), validAfter) - mstore(add(ptr, 384), validUntil) - mstore(add(ptr, 416), entryPoint) - - structHash:= keccak256(ptr, 448) + structHash := keccak256(fields, 448) } - operationData = abi.encodePacked( - bytes1(0x19), - bytes1(0x01), - domainSeparator(), - structHash - ); + operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); } } } diff --git a/4337/src/deploy/mock.ts b/4337/src/deploy/mock.ts index a69daa64..41e71206 100644 --- a/4337/src/deploy/mock.ts +++ b/4337/src/deploy/mock.ts @@ -14,7 +14,7 @@ const deploy: DeployFunction = async ({ deployments, getNamedAccounts, network } await deploy('SafeMock', { from: deployer, - args: [entryPoint], + args: [], log: true, deterministicDeployment: true, }) From 8a53ac6944469b54156ee2932318d5b9d38c180e Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 16:10:10 +0000 Subject: [PATCH 08/13] Comment fixes --- 4337/contracts/Safe4337Module.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 646578ab..536ddfa0 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -155,10 +155,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this)); } - // TODO(nlodell): @dev When validating the user operation, the signature timestamps are pre-pended to the signature bytes. - /** * @notice Returns the 32-byte Safe operation hash to be signed by owners for the specified ERC-4337 user operation. + * @dev The Safe operation timestamps are pre-pended to the signature bytes as `abi.encodePacked(validAfter, validUntil, signatures)`. * @param userOp The ERC-4337 user operation. * @return operationHash Operation hash. */ @@ -193,6 +192,8 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle function _getSafeOp( UserOperation calldata userOp ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + // Extract additional Safe operation fields from the user operation signature which is encoded as: + // `abi.encodePacked(validAfter, validUntil, signatures)` { bytes calldata sig = userOp.signature; validAfter = uint48(bytes6(sig[0:6])); From 0402aba14665d7a140a509662bdf4d0f09e71d21 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 16:13:34 +0000 Subject: [PATCH 09/13] Small review improvements --- 4337/contracts/Safe4337Module.sol | 15 +++++++-------- 4337/contracts/test/SafeMock.sol | 17 +++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 536ddfa0..6901eb7f 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -182,12 +182,12 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } /** - * @dev Decodes an ERC-4337 user operation and returns ERC-712 Safe operation bytes. + * @dev Decodes an ERC-4337 user operation into a Safe operation. * @param userOp The ERC-4337 user operation. - * @return operationData Encoded operation data bytes. + * @return operationData Encoded EIP-712 Safe operation data bytes used for signature verification. * @return validAfter The timestamp the user operation is valid from. * @return validUntil The timestamp the user operation is valid until. - * @return signatures The Safe signatures extracted from the user operation. + * @return signatures The Safe owner signatures extracted from the user operation. */ function _getSafeOp( UserOperation calldata userOp @@ -202,13 +202,13 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent - // user operations from being submitted that do not fully respect the user preferences. The only exception are + // user operations from being submitted that do not fully respect the user preferences. The only exception is // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. { // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually - // encode the `SafeOp` fields into a memory `struct`. - SafeOpFields memory fields = SafeOpFields({ + // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. + SafeOpFields memory structFields = SafeOpFields({ typeHash: SAFE_OP_TYPEHASH, safe: userOp.sender, nonce: userOp.nonce, @@ -224,11 +224,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle validUntil: validUntil, entryPoint: SUPPORTED_ENTRYPOINT }); - bytes32 structHash; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - structHash := keccak256(fields, 448) + structHash := keccak256(structFields, 448) } operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index fc7a123b..67e4fa20 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -204,16 +204,18 @@ contract Safe4337Mock is SafeMock, IAccount { } /** - * @dev Decodes an ERC-4337 user operation and returns ERC-712 Safe operation bytes. + * @dev Decodes an ERC-4337 user operation into a Safe operation. * @param userOp The ERC-4337 user operation. - * @return operationData Encoded operation data bytes. + * @return operationData Encoded EIP-712 Safe operation data bytes used for signature verification. * @return validAfter The timestamp the user operation is valid from. * @return validUntil The timestamp the user operation is valid until. - * @return signatures The Safe signatures extracted from the user operation. + * @return signatures The Safe owner signatures extracted from the user operation. */ function _getSafeOp( UserOperation calldata userOp ) internal view returns (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) { + // Extract additional Safe operation fields from the user operation signature which is encoded as: + // `abi.encodePacked(validAfter, validUntil, signatures)` { bytes calldata sig = userOp.signature; validAfter = uint48(bytes6(sig[0:6])); @@ -222,13 +224,13 @@ contract Safe4337Mock is SafeMock, IAccount { } // It is important that **all** user operation fields are represented in the `SafeOp` data somehow, to prevent - // user operations from being submitted that do not fully respect the user preferences. The only exception are + // user operations from being submitted that do not fully respect the user preferences. The only exception is // the `signature` bytes. Note that even `initCode` needs to be represented in the operation data, otherwise // it can be replaced with a more expensive initialization that would charge the user additional fees. { // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually - // encode the `SafeOp` fields into a memory `struct`. - SafeOpFields memory fields = SafeOpFields({ + // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. + SafeOpFields memory structFields = SafeOpFields({ typeHash: SAFE_OP_TYPEHASH, safe: userOp.sender, nonce: userOp.nonce, @@ -244,11 +246,10 @@ contract Safe4337Mock is SafeMock, IAccount { validUntil: validUntil, entryPoint: SUPPORTED_ENTRYPOINT }); - bytes32 structHash; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - structHash := keccak256(fields, 448) + structHash := keccak256(structFields, 448) } operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); From aa8e9055c5048b824ccad1b427347ee76a5c2685 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 16:51:07 +0000 Subject: [PATCH 10/13] Fix runOp script for new signing scheme --- 4337/scripts/runOp.ts | 2 +- 4337/src/tasks/local_verify.ts | 5 ++++- 4337/src/utils/safe.ts | 17 ++++++++++++++--- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/4337/scripts/runOp.ts b/4337/scripts/runOp.ts index 1afaf6c2..ebda03cd 100644 --- a/4337/scripts/runOp.ts +++ b/4337/scripts/runOp.ts @@ -78,7 +78,7 @@ const runOp = async () => { let toAddress = '0x02270bd144e70cE6963bA02F575776A16184E1E6' let callData = '0x' - let value: BigNumberish = ethers.parseEther('0.0001') + let value = ethers.parseEther('0.0001') if (ERC20_TOKEN_ADDRESS) { toAddress = ERC20_TOKEN_ADDRESS callData = buildData('transfer(address,uint256)', [user1.address, ethers.parseEther('1')]) diff --git a/4337/src/tasks/local_verify.ts b/4337/src/tasks/local_verify.ts index 1b1ebce1..adc9f565 100644 --- a/4337/src/tasks/local_verify.ts +++ b/4337/src/tasks/local_verify.ts @@ -6,7 +6,10 @@ task('local-verify', 'Verifies that the local deployment files correspond to the const deployedContracts = await hre.deployments.all() for (const contract of Object.keys(deployedContracts)) { const deployment = await hre.deployments.get(contract) - if (!deployment.metadata) throw new Error(`No metadata for ${contract}`) + if (!deployment.metadata) { + console.log(`Verification status for ${contract}: SKIPPED`) + continue + } const meta = JSON.parse(deployment.metadata) const solcjs = await loadSolc(meta.compiler.version) delete meta.compiler diff --git a/4337/src/utils/safe.ts b/4337/src/utils/safe.ts index 7fcb8410..beec7ce9 100644 --- a/4337/src/utils/safe.ts +++ b/4337/src/utils/safe.ts @@ -24,11 +24,15 @@ const INTERFACES = new ethers.Interface([ export interface OperationParams { nonce: bigint + initCode: string preVerificationGas: bigint verificationGasLimit: bigint callGasLimit: bigint maxFeePerGas: bigint maxPriorityFeePerGas: bigint + paymasterAndData: string + validAfter: bigint + validUntil: bigint } export interface GlobalConfig { @@ -153,7 +157,6 @@ export class Safe4337Operation { } async userOperation(paymasterAndData = '0x'): Promise { - const initCode = (await this.safe.isDeployed()) ? '0x' : this.safe.getInitCode() return { nonce: ethers.toBeHex(this.params.nonce), callData: actionCalldata(this.action), @@ -162,10 +165,13 @@ export class Safe4337Operation { callGasLimit: ethers.toBeHex(this.params.callGasLimit), maxFeePerGas: ethers.toBeHex(this.params.maxFeePerGas), maxPriorityFeePerGas: ethers.toBeHex(this.params.maxPriorityFeePerGas), - initCode, + initCode: this.params.initCode, paymasterAndData, sender: this.safe.address, - signature: await this.encodedSignatures(), + signature: ethers.solidityPacked( + ['uint48', 'uint48', 'bytes'], + [this.params.validAfter, this.params.validUntil, await this.encodedSignatures()], + ), } } @@ -174,6 +180,7 @@ export class Safe4337Operation { const signerAddress = await signer.getAddress() if (validSigners.indexOf(signerAddress) < 0) throw Error('Invalid Signer') if (this.signatures.findIndex((signature) => signature.signer === signerAddress) >= 0) throw Error('Already signed') + const initCode = (await this.safe.isDeployed()) ? '0x' : this.safe.getInitCode() this.signatures.push({ signer: signerAddress, data: await signer.signTypedData( @@ -227,6 +234,7 @@ export class Safe4337Operation { const params: OperationParams = { nonce, + initCode, maxFeePerGas: feeData.maxFeePerGas, maxPriorityFeePerGas: feeData.maxPriorityFeePerGas, // Add a small margin as some dataoverhead calculation is not always accurate @@ -234,6 +242,9 @@ export class Safe4337Operation { // Add 20% to the gas limits to account for inaccurate estimations verificationGasLimit: (BigInt(estimates.verificationGasLimit) * 12n) / 10n, callGasLimit: (BigInt(estimates.callGasLimit) * 12n) / 10n, + validAfter: 0n, + validUntil: 0n, + paymasterAndData: '0x', } return new Safe4337Operation(safe, action, params, globalConfig) } From 9de5b74a75c9874fb8c49094f6e733ec5c63a8c2 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 29 Nov 2023 16:54:22 +0000 Subject: [PATCH 11/13] Fix minor lint error --- 4337/src/utils/safe.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/4337/src/utils/safe.ts b/4337/src/utils/safe.ts index beec7ce9..c0b423e8 100644 --- a/4337/src/utils/safe.ts +++ b/4337/src/utils/safe.ts @@ -180,7 +180,6 @@ export class Safe4337Operation { const signerAddress = await signer.getAddress() if (validSigners.indexOf(signerAddress) < 0) throw Error('Invalid Signer') if (this.signatures.findIndex((signature) => signature.signer === signerAddress) >= 0) throw Error('Already signed') - const initCode = (await this.safe.isDeployed()) ? '0x' : this.safe.getInitCode() this.signatures.push({ signer: signerAddress, data: await signer.signTypedData( From ca530e28a16a649a35906a8bd7ed20889e351065 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Thu, 30 Nov 2023 09:02:59 +0000 Subject: [PATCH 12/13] Nits --- 4337/contracts/Safe4337Module.sol | 56 +++++++++++++++++-------------- 4337/contracts/test/SafeMock.sol | 32 +++++++++++------- 2 files changed, 50 insertions(+), 38 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index 6901eb7f..72814591 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -20,26 +20,6 @@ import {ISafe} from "./interfaces/Safe.sol"; * - Replay protection is handled by the entry point. */ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandler { - /** - * @dev A structure used internally for computing the EIP-712 signing message for a Safe operation. - */ - struct SafeOpFields { - bytes32 typeHash; - address safe; - uint256 nonce; - bytes32 initCodeHash; - bytes32 callDataHash; - uint256 callGasLimit; - uint256 verificationGasLimit; - uint256 preVerificationGas; - uint256 maxFeePerGas; - uint256 maxPriorityFeePerGas; - bytes32 paymasterAndDataHash; - uint48 validAfter; - uint48 validUntil; - address entryPoint; - } - /** * @notice The EIP-712 type-hash for the domain separator used for verifying Safe operation signatures. */ @@ -67,6 +47,26 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" ); + /** + * @dev A structure used internally for manually encoding a Safe operation for when computing the EIP-712 struct hash. + */ + struct EncodedSafeOpStruct { + bytes32 typeHash; + address safe; + uint256 nonce; + bytes32 initCodeHash; + bytes32 callDataHash; + uint256 callGasLimit; + uint256 verificationGasLimit; + uint256 preVerificationGas; + uint256 maxFeePerGas; + uint256 maxPriorityFeePerGas; + bytes32 paymasterAndDataHash; + uint48 validAfter; + uint48 validUntil; + address entryPoint; + } + /** * @notice The EIP-712 type-hash for the domain separator used for verifying Safe operation signatures. */ @@ -207,8 +207,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle // it can be replaced with a more expensive initialization that would charge the user additional fees. { // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually - // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. - SafeOpFields memory structFields = SafeOpFields({ + // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. This works + // because the `EncodedSafeOpStruct` struct has no "dynamic" fields so its memory layout is identical to the + // result of `abi.encode`-ing the individual fields. + EncodedSafeOpStruct memory encodedSafeOp = EncodedSafeOpStruct({ typeHash: SAFE_OP_TYPEHASH, safe: userOp.sender, nonce: userOp.nonce, @@ -224,13 +226,17 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle validUntil: validUntil, entryPoint: SUPPORTED_ENTRYPOINT }); - bytes32 structHash; + + bytes32 safeOpStructHash; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - structHash := keccak256(structFields, 448) + // Since the `encodedSafeOp` value's memory layout is identical to the result of `abi.encode`-ing the + // individual `SafeOp` fields, we can pass it directly to `keccak256`. Additionally, there are 14 + // 32-byte fields to hash, for a length of `14 * 32 = 448` bytes. + safeOpStructHash := keccak256(encodedSafeOp, 448) } - operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); + operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOpStructHash); } } } diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index 67e4fa20..2f60581e 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -96,7 +96,14 @@ contract SafeMock { contract Safe4337Mock is SafeMock, IAccount { using UserOperationLib for UserOperation; - struct SafeOpFields { + bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"); + + bytes32 private constant SAFE_OP_TYPEHASH = + keccak256( + "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" + ); + + struct EncodedSafeOpStruct { bytes32 typeHash; address safe; uint256 nonce; @@ -113,13 +120,6 @@ contract Safe4337Mock is SafeMock, IAccount { address entryPoint; } - bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"); - - bytes32 private constant SAFE_OP_TYPEHASH = - keccak256( - "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)" - ); - address public immutable SUPPORTED_ENTRYPOINT; constructor(address entryPoint) { @@ -229,8 +229,10 @@ contract Safe4337Mock is SafeMock, IAccount { // it can be replaced with a more expensive initialization that would charge the user additional fees. { // In order to work around Solidity "stack too deep" errors related to too many stack variables, manually - // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. - SafeOpFields memory structFields = SafeOpFields({ + // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712 struct-hash. This works + // because the `EncodedSafeOpStruct` struct has no "dynamic" fields so its memory layout is identical to the + // result of `abi.encode`-ing the individual fields. + EncodedSafeOpStruct memory encodedSafeOp = EncodedSafeOpStruct({ typeHash: SAFE_OP_TYPEHASH, safe: userOp.sender, nonce: userOp.nonce, @@ -246,13 +248,17 @@ contract Safe4337Mock is SafeMock, IAccount { validUntil: validUntil, entryPoint: SUPPORTED_ENTRYPOINT }); - bytes32 structHash; + + bytes32 safeOpStructHash; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - structHash := keccak256(structFields, 448) + // Since the `encodedSafeOp` value's memory layout is identical to the result of `abi.encode`-ing the + // individual `SafeOp` fields, we can pass it directly to `keccak256`. Additionally, there are 14 + // 32-byte fields to hash, for a length of `14 * 32 = 448` bytes. + safeOpStructHash := keccak256(encodedSafeOp, 448) } - operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), structHash); + operationData = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOpStructHash); } } } From 467df98a52be80fe10434b92aee69f8bfc381ed5 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Thu, 30 Nov 2023 09:27:16 +0000 Subject: [PATCH 13/13] Add test to verify operation hash changes if any UserOperation fields change --- 4337/test/eip4337/Safe4337Module.spec.ts | 41 ++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/4337/test/eip4337/Safe4337Module.spec.ts b/4337/test/eip4337/Safe4337Module.spec.ts index df0cf1f3..89500f8e 100644 --- a/4337/test/eip4337/Safe4337Module.spec.ts +++ b/4337/test/eip4337/Safe4337Module.spec.ts @@ -58,6 +58,47 @@ describe('Safe4337Module', () => { expect(operationHash).to.equal(calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId())) }) + + it('should change if any UserOperation fields change', async () => { + const { validator } = await setupTests() + + const signature = ({ validAfter, validUntil }: { validAfter: number; validUntil: number }) => + ethers.solidityPacked(['uint48', 'uint48'], [validAfter, validUntil]) + const userOp = { + sender: '0x0000000000000000000000000000000000000011', + nonce: 0x12, + initCode: '0x13', + callData: '0x14', + callGasLimit: 0x15, + verificationGasLimit: 0x16, + preVerificationGas: 0x17, + maxFeePerGas: 0x18, + maxPriorityFeePerGas: 0x19, + paymasterAndData: '0x1a', + signature: signature({ + validAfter: 0x1b, + validUntil: 0x1c, + }), + } + const operationHash = await validator.getOperationHash(userOp) + + for (const [name, value] of [ + ['sender', '0x0000000000000000000000000000000000000021'], + ['nonce', 0x22], + ['initCode', '0x23'], + ['callData', '0x24'], + ['callGasLimit', 0x25], + ['verificationGasLimit', 0x26], + ['preVerificationGas', 0x27], + ['maxFeePerGas', 0x28], + ['maxPriorityFeePerGas', 0x29], + ['paymasterAndData', '0x2a'], + ['signature', signature({ validAfter: 0x2b, validUntil: 0x1c })], + ['signature', signature({ validAfter: 0x1b, validUntil: 0x2c })], + ]) { + expect(await validator.getOperationHash({ ...userOp, [name]: value })).to.not.equal(operationHash) + } + }) }) describe('constructor', () => {