From 7cfb9ddeb5f4c741987a360477928d1c581f22cd Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Wed, 22 Nov 2023 11:20:17 +0100 Subject: [PATCH] Send encoded Safe operation bytes to `checkSignatures` call (#165) Fixes #160 This PR fixes the `checkSignatures` call to the Safe to include both the Safe operation hash for recovery, but also the encoded operation bytes, which is required when using an ERC-1271 signer. In order to test this, the `SafeMock` now requires that `dataHash == keccak256(data)` and I added a unit test using a nested Safe set (note that this would require a staked paymaster to work with 4337 bundlers, but the test does check that the contract signatures works as expected). --- 4337/contracts/Safe4337Module.sol | 57 +++++++++++-- 4337/contracts/test/SafeMock.sol | 3 +- 4337/src/utils/execution.ts | 8 +- 4337/src/utils/safe.ts | 9 ++- 4337/src/utils/userOp.ts | 4 + 4337/test/e2e/LocalBundler.spec.ts | 6 +- 4337/test/eip4337/ReferenceEntryPoint.spec.ts | 80 ++++++++++++++++++- 7 files changed, 143 insertions(+), 24 deletions(-) diff --git a/4337/contracts/Safe4337Module.sol b/4337/contracts/Safe4337Module.sol index fff17f764..c78bfc0bf 100644 --- a/4337/contracts/Safe4337Module.sol +++ b/4337/contracts/Safe4337Module.sol @@ -128,7 +128,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } /** - * @dev Returns the bytes that are hashed to be signed by owners. + * @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. @@ -138,9 +138,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @param maxFeePerGas Max fee per gas. * @param maxPriorityFeePerGas Max priority fee per gas. * @param entryPoint Address of the entry point. - * @return Operation hash bytes. + * @return Operation bytes. */ - function getOperationHash( + function getOperationData( address safe, bytes memory callData, uint256 nonce, @@ -151,7 +151,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle uint256 maxPriorityFeePerGas, uint96 signatureTimestamps, address entryPoint - ) public view returns (bytes32) { + ) internal view returns (bytes memory) { bytes32 safeOperationHash = keccak256( abi.encode( SAFE_OP_TYPEHASH, @@ -167,7 +167,49 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle entryPoint ) ); - return keccak256(abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOperationHash)); + return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOperationHash); + } + + /** + * @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 callGasLimit Gas available during the execution of the call. + * @param maxFeePerGas Max fee per gas. + * @param maxPriorityFeePerGas Max priority fee per gas. + * @param entryPoint Address of the entry point. + * @return Operation hash. + */ + function getOperationHash( + address safe, + bytes memory callData, + uint256 nonce, + uint256 preVerificationGas, + uint256 verificationGasLimit, + uint256 callGasLimit, + uint256 maxFeePerGas, + uint256 maxPriorityFeePerGas, + uint96 signatureTimestamps, + address entryPoint + ) external view returns (bytes32) { + return + keccak256( + getOperationData( + safe, + callData, + nonce, + preVerificationGas, + verificationGasLimit, + callGasLimit, + maxFeePerGas, + maxPriorityFeePerGas, + signatureTimestamps, + entryPoint + ) + ); } /** @@ -178,7 +220,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle */ function _validateSignatures(address entryPoint, UserOperation calldata userOp) internal view returns (uint256 validationData) { uint96 signatureTimestamps = uint96(bytes12(userOp.signature[:12])); - bytes32 operationHash = getOperationHash( + bytes memory operationData = getOperationData( payable(userOp.sender), userOp.callData, userOp.nonce, @@ -190,11 +232,12 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle signatureTimestamps, entryPoint ); + bytes32 operationHash = keccak256(operationData); // The timestamps are validated by the entry point, therefore we will not check them again uint48 validUntil = uint48(signatureTimestamps >> 48); uint48 validAfter = uint48(signatureTimestamps); - try ISafe(payable(userOp.sender)).checkSignatures(operationHash, "", userOp.signature[12:]) { + try ISafe(payable(userOp.sender)).checkSignatures(operationHash, operationData, userOp.signature[12:]) { validationData = _packValidationData(false, validUntil, validAfter); } catch { validationData = _packValidationData(true, validUntil, validAfter); diff --git a/4337/contracts/test/SafeMock.sol b/4337/contracts/test/SafeMock.sol index 2b63725e0..3f9b3f3cd 100644 --- a/4337/contracts/test/SafeMock.sol +++ b/4337/contracts/test/SafeMock.sol @@ -36,7 +36,8 @@ contract SafeMock { } } - function checkSignatures(bytes32 dataHash, bytes memory, bytes memory signature) public view { + function checkSignatures(bytes32 dataHash, bytes memory data, bytes memory signature) public view { + require(dataHash == keccak256(data), "Invalid data hash"); uint8 v; bytes32 r; bytes32 s; diff --git a/4337/src/utils/execution.ts b/4337/src/utils/execution.ts index 1cb2d91af..f70cd9043 100644 --- a/4337/src/utils/execution.ts +++ b/4337/src/utils/execution.ts @@ -43,12 +43,8 @@ export const signHash = async (signer: Signer, hash: string): Promise { signatures.sort((left, right) => left.signer.toLowerCase().localeCompare(right.signer.toLowerCase())) - let signatureBytes = '0x' - for (const sig of signatures) { - signatureBytes += sig.data.slice(2) - } - - const signatureWithTimestamps = ethers.solidityPacked(['uint96', 'bytes'], [timestamps.toString(), signatureBytes]) + const signatureBytes = ethers.concat(signatures.map((signature) => signature.data)) + const signatureWithTimestamps = ethers.solidityPacked(['uint96', 'bytes'], [timestamps, signatureBytes]) return signatureWithTimestamps } diff --git a/4337/src/utils/safe.ts b/4337/src/utils/safe.ts index 00f9e48c4..3baeab6e4 100644 --- a/4337/src/utils/safe.ts +++ b/4337/src/utils/safe.ts @@ -1,4 +1,4 @@ -import { AddressLike, JsonRpcProvider, Provider, ethers } from 'ethers' +import { AddressLike, JsonRpcProvider, Provider, Signer, ethers } from 'ethers' // Import from Safe contracts repo once fixed import { MetaTransaction, SafeSignature, buildSignatureBytes } from './execution' @@ -271,6 +271,13 @@ export class Safe4337 { return code !== '0x' } + async deploy(deployer: Signer): Promise { + const initCode = this.getInitCode() + const factory = ethers.getAddress(ethers.dataSlice(initCode, 0, 20)) + const initCallData = ethers.dataSlice(initCode, 20) + await deployer.sendTransaction({ to: factory, data: initCallData }).then((tx) => tx.wait()) + } + getInitCode(): string { if (!this.safeConfig) throw Error('Init code not available') const initParams = buildInitParamsForConfig(this.safeConfig, this.globalConfig) diff --git a/4337/src/utils/userOp.ts b/4337/src/utils/userOp.ts index a0e587534..4eb79bf77 100644 --- a/4337/src/utils/userOp.ts +++ b/4337/src/utils/userOp.ts @@ -52,6 +52,10 @@ export const calculateSafeOperationHash = (eip4337ModuleAddress: string, safeOp: return ethers.TypedDataEncoder.hash({ chainId, verifyingContract: eip4337ModuleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp) } +export const calculateSafeOperationData = (eip4337ModuleAddress: string, safeOp: SafeUserOperation, chainId: BigNumberish): string => { + return ethers.TypedDataEncoder.encode({ chainId, verifyingContract: eip4337ModuleAddress }, EIP712_SAFE_OPERATION_TYPE, safeOp) +} + export const signSafeOp = async ( signer: Signer, moduleAddress: string, diff --git a/4337/test/e2e/LocalBundler.spec.ts b/4337/test/e2e/LocalBundler.spec.ts index 0a0158cbb..bbd41bc1a 100644 --- a/4337/test/e2e/LocalBundler.spec.ts +++ b/4337/test/e2e/LocalBundler.spec.ts @@ -124,11 +124,7 @@ describe('E2E - Local Bundler', () => { it('should execute a transaction for an exsiting Safe', async () => { const { user, bundler, safe, validator, entryPoint, token } = await setupTests() - const initCode = safe.getInitCode() - const factory = ethers.getAddress(ethers.dataSlice(initCode, 0, 20)) - const initCallData = ethers.dataSlice(initCode, 20) - await user.sendTransaction({ to: factory, data: initCallData }).then((tx) => tx.wait()) - + await safe.deploy(user) await token.transfer(safe.address, ethers.parseUnits('4.2', 18)).then((tx) => tx.wait()) await user.sendTransaction({ to: safe.address, value: ethers.parseEther('0.5') }).then((tx) => tx.wait()) diff --git a/4337/test/eip4337/ReferenceEntryPoint.spec.ts b/4337/test/eip4337/ReferenceEntryPoint.spec.ts index 52bd792d0..6154b6670 100644 --- a/4337/test/eip4337/ReferenceEntryPoint.spec.ts +++ b/4337/test/eip4337/ReferenceEntryPoint.spec.ts @@ -6,10 +6,11 @@ import EntryPointArtifact from '@account-abstraction/contracts/artifacts/EntryPo import { getFactory, getAddModulesLib } from '../utils/setup' import { buildSignatureBytes, logGas } from '../../src/utils/execution' import { - buildUserOperationFromSafeUserOperation, buildSafeUserOpTransaction, - signSafeOp, + buildUserOperationFromSafeUserOperation, + calculateSafeOperationData, encodeSignatureTimestamp, + signSafeOp, } from '../../src/utils/userOp' import { chainId } from '../utils/encoding' import { Safe4337 } from '../../src/utils/safe' @@ -28,7 +29,7 @@ describe('Safe4337Module - Reference EntryPoint', () => { const singletonFactory = await ethers.getContractFactory('SafeL2', deployer) const singleton = await singletonFactory.deploy() - const safe = await Safe4337.withSigner(user.address, { + const safeGlobalConfig = { safeSingleton: await singleton.getAddress(), entryPoint: await entryPoint.getAddress(), erc4337module: await module.getAddress(), @@ -36,7 +37,8 @@ describe('Safe4337Module - Reference EntryPoint', () => { addModulesLib: await addModulesLib.getAddress(), proxyCreationCode, chainId: Number(await chainId()), - }) + } + const safe = await Safe4337.withSigner(user.address, safeGlobalConfig) return { user, @@ -44,6 +46,7 @@ describe('Safe4337Module - Reference EntryPoint', () => { safe, validator: module, entryPoint, + safeGlobalConfig, } } @@ -152,6 +155,75 @@ describe('Safe4337Module - Reference EntryPoint', () => { expect(await ethers.provider.getBalance(safe.address)).to.be.eq(accountBalance - transfers - deposits) }) + it('should support a Safe signer (NOTE: would require a staked paymaster for ERC-4337)', async () => { + const { user, relayer, safe: parentSafe, validator, entryPoint, safeGlobalConfig } = await setupTests() + + await parentSafe.deploy(user) + const daughterSafe = await Safe4337.withSigner(parentSafe.address, safeGlobalConfig) + + const accountBalance = ethers.parseEther('1.0') + await user.sendTransaction({ to: daughterSafe.address, value: accountBalance }) + 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 opData = calculateSafeOperationData(await validator.getAddress(), safeOp, await chainId()) + const signature = ethers.solidityPacked( + ['bytes', 'bytes'], + [ + buildSignatureBytes([ + { + signer: parentSafe.address, + data: ethers.solidityPacked( + ['bytes32', 'bytes32', 'uint8'], + [ + ethers.toBeHex(parentSafe.address, 32), // `r` holds the contract signer + ethers.toBeHex(65, 32), // `s` holds the offset of the signature bytes + 0, // `v` of 0 indicates a contract signer + ], + ), + }, + ]), + ethers.solidityPacked( + ['uint256', 'bytes'], + [ + 65, // signature length + await user.signTypedData( + { + verifyingContract: parentSafe.address, + chainId: await chainId(), + }, + { + SafeMessage: [{ type: 'bytes', name: 'message' }], + }, + { + message: opData, + }, + ), + ], + ), + ], + ) + const userOp = buildUserOperationFromSafeUserOperation({ + safeAddress: daughterSafe.address, + safeOp, + signature, + initCode: daughterSafe.getInitCode(), + }) + + const transaction = await logGas( + 'Execute UserOps with reference EntryPoint', + entryPoint.handleOps([userOp], await relayer.getAddress()), + ) + const receipt = await transaction.wait() + + const deposits = receipt.logs + .filter(isEventLog) + .filter((log) => log.eventName === 'Deposited') + .reduce((acc, { args: [, deposit] }) => acc + deposit, 0n) + expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance - transfer - deposits) + }) + function isEventLog(log: Log): log is EventLog { return typeof (log as Partial).eventName === 'string' }