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

@@ -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);

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;

4337/src/utils/execution.ts

@@ -43,12 +43,8 @@ export const signHash = async (signer: Signer, hash: string): Promise<SafeSignat
 
 export const buildSignatureBytes = (signatures: SafeSignature[], timestamps: BigNumberish = 0): string => {
   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
 }

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<void> {
+    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)

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,

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())
 

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,22 +29,24 @@ 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(),
       proxyFactory: await proxyFactory.getAddress(),
       addModulesLib: await addModulesLib.getAddress(),
       proxyCreationCode,
       chainId: Number(await chainId()),
-    })
+    }
+    const safe = await Safe4337.withSigner(user.address, safeGlobalConfig)
 
     return {
       user,
       relayer,
       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<EventLog>).eventName === 'string'
   }