Sign All Gas Limit and Price Parameters (#261)

Fixes #250

This PR changes the `SafeOperation` type to include ALL gas parameters
for signing, instead of packing them. This ensures the maximum amount of
data is available to the user when signing user operations, instead of
trusting gas parameters from a Dapp.

Note a subtle change in the order of the "fee per gas" fields, this was
done to match the order in the `PackedUserOperation` struct, as well as
the order in EIP-1559 transactions. I figured, since we are changing the
signing format anyway (notice the different integer types), this
wouldn't be a big deal.

Additionally, we have a new test that verifies that `safeOp` hashes are
computed off-chain in the same way they are computed on-chain. This is
to catch issues with field orders being slightly different in the
`EncodedSafeOp` struct that we would otherwise miss. Notably, the "fee
per gas" parameters are always set to the same value, and our existing
test suite was not able to catch swapped order in `EncodedSafeOp`
fields.

modules/4337/contracts/Safe4337Module.sol

@@ -6,6 +6,7 @@ import {CompatibilityFallbackHandler} from "@safe-global/safe-contracts/contract
 import {IAccount} from "@account-abstraction/contracts/contracts/interfaces/IAccount.sol";
 import {PackedUserOperation} from "@account-abstraction/contracts/contracts/interfaces/PackedUserOperation.sol";
 import {_packValidationData} from "@account-abstraction/contracts/contracts/core/Helpers.sol";
+import {UserOperationLib} from "@account-abstraction/contracts/contracts/core/UserOperationLib.sol";
 import {ISafe} from "./interfaces/Safe.sol";
 
 /**
@@ -21,6 +22,8 @@ import {ISafe} from "./interfaces/Safe.sol";
  * @custom:security-contact bounty@safe.global
  */
 contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandler {
+    using UserOperationLib for PackedUserOperation;
+
     /**
      * @notice The EIP-712 type-hash for the domain separator used for verifying Safe operation signatures.
      * @dev keccak256("EIP712Domain(uint256 chainId,address verifyingContract)") = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218
@@ -33,20 +36,21 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
      *  {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.
-     *  {bytes32} accountGasLimits - Packed encoding of the gas limits for the account: {uint128 validationGasLimit, uint128 callGasLimit}.
+     *  {uint128} verificationGasLimit - The maximum amount of gas allowed for the verification process.
+     *  {uint128} callGasLimit - The maximum amount of gas allowed for executing the function call.
      *  {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.
+     *  {uint128} maxPriorityFeePerGas - The maximum priority fee per gas that the user is willing to pay for the transaction.
+     *  {uint128} maxFeePerGas - The maximum 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 When validating the user operation, the signature timestamps are pre-pended to the signature bytes.
+     * @dev When validating the user operation, the signature timestamps are pre-pended to the signature bytes. Equal to:
      * keccak256(
-            "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,bytes32 accountGasLimits,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)"
-        ) = 0x9efbfd16c059a992a21cba49ddec650b37de25cf6baa04788c16c00b47bb62de
+     *     "SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,uint128 verificationGasLimit,uint128 callGasLimit,uint256 preVerificationGas,uint128 maxPriorityFeePerGas,uint128 maxFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)"
+     * ) = 0xc03dfc11d8b10bf9cf703d558958c8c42777f785d998c62060d85a4f0ef6ea7f
      */
-    bytes32 private constant SAFE_OP_TYPEHASH = 0x9efbfd16c059a992a21cba49ddec650b37de25cf6baa04788c16c00b47bb62de;
+    bytes32 private constant SAFE_OP_TYPEHASH = 0xc03dfc11d8b10bf9cf703d558958c8c42777f785d998c62060d85a4f0ef6ea7f;
 
     /**
      * @dev A structure used internally for manually encoding a Safe operation for when computing the EIP-712 struct hash.
@@ -57,10 +61,11 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
         uint256 nonce;
         bytes32 initCodeHash;
         bytes32 callDataHash;
-        bytes32 accountGasLimits;
+        uint128 verificationGasLimit;
+        uint128 callGasLimit;
         uint256 preVerificationGas;
-        uint256 maxFeePerGas;
-        uint256 maxPriorityFeePerGas;
+        uint128 maxPriorityFeePerGas;
+        uint128 maxFeePerGas;
         bytes32 paymasterAndDataHash;
         uint48 validAfter;
         uint48 validUntil;
@@ -260,10 +265,11 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
                 nonce: userOp.nonce,
                 initCodeHash: keccak256(userOp.initCode),
                 callDataHash: keccak256(userOp.callData),
-                accountGasLimits: userOp.accountGasLimits,
+                verificationGasLimit: uint128(userOp.unpackVerificationGasLimit()),
+                callGasLimit: uint128(userOp.unpackCallGasLimit()),
                 preVerificationGas: userOp.preVerificationGas,
-                maxFeePerGas: userOp.maxFeePerGas,
-                maxPriorityFeePerGas: userOp.maxPriorityFeePerGas,
+                maxPriorityFeePerGas: uint128(userOp.unpackMaxPriorityFeePerGas()),
+                maxFeePerGas: uint128(userOp.unpackMaxFeePerGas()),
                 paymasterAndDataHash: keccak256(userOp.paymasterAndData),
                 validAfter: validAfter,
                 validUntil: validUntil,
@@ -275,8 +281,8 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
             assembly ("memory-safe") {
                 // 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 13
-                // 32-byte fields to hash, for a length of `13 * 32 = 448` bytes.
-                safeOpStructHash := keccak256(encodedSafeOp, 416)
+                // 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(), safeOpStructHash);

modules/4337/contracts/test/SafeMock.sol

@@ -102,13 +102,9 @@ contract SafeMock {
 contract Safe4337Mock is SafeMock, IAccount {
     using UserOperationLib for PackedUserOperation;
 
-    /// keccak256("EIP712Domain(uint256 chainId,address verifyingContract)") = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218
     bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
 
-    // keccak256(
-    //     "SafeOp("SafeOp(address safe,uint256 nonce,bytes initCode,bytes callData,bytes32 accountGasLimits,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,uint48 validAfter,uint48 validUntil,address entryPoint)"
-    // ) = 0x9efbfd16c059a992a21cba49ddec650b37de25cf6baa04788c16c00b47bb62de
-    bytes32 private constant SAFE_OP_TYPEHASH = 0x9efbfd16c059a992a21cba49ddec650b37de25cf6baa04788c16c00b47bb62de;
+    bytes32 private constant SAFE_OP_TYPEHASH = 0xc03dfc11d8b10bf9cf703d558958c8c42777f785d998c62060d85a4f0ef6ea7f;
 
     /**
      * @dev A structure used internally for manually encoding a Safe operation for when computing the EIP-712 struct hash.
@@ -119,10 +115,11 @@ contract Safe4337Mock is SafeMock, IAccount {
         uint256 nonce;
         bytes32 initCodeHash;
         bytes32 callDataHash;
-        bytes32 accountGasLimits;
+        uint128 verificationGasLimit;
+        uint128 callGasLimit;
         uint256 preVerificationGas;
-        uint256 maxFeePerGas;
-        uint256 maxPriorityFeePerGas;
+        uint128 maxPriorityFeePerGas;
+        uint128 maxFeePerGas;
         bytes32 paymasterAndDataHash;
         uint48 validAfter;
         uint48 validUntil;
@@ -258,10 +255,11 @@ contract Safe4337Mock is SafeMock, IAccount {
                 nonce: userOp.nonce,
                 initCodeHash: keccak256(userOp.initCode),
                 callDataHash: keccak256(userOp.callData),
-                accountGasLimits: userOp.accountGasLimits,
+                verificationGasLimit: uint128(userOp.unpackVerificationGasLimit()),
+                callGasLimit: uint128(userOp.unpackCallGasLimit()),
                 preVerificationGas: userOp.preVerificationGas,
-                maxFeePerGas: userOp.maxFeePerGas,
-                maxPriorityFeePerGas: userOp.maxPriorityFeePerGas,
+                maxPriorityFeePerGas: uint128(userOp.unpackMaxPriorityFeePerGas()),
+                maxFeePerGas: uint128(userOp.unpackMaxFeePerGas()),
                 paymasterAndDataHash: keccak256(userOp.paymasterAndData),
                 validAfter: validAfter,
                 validUntil: validUntil,
@@ -273,8 +271,8 @@ contract Safe4337Mock is SafeMock, IAccount {
             assembly ("memory-safe") {
                 // 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 13
-                // 32-byte fields to hash, for a length of `13 * 32 = 448` bytes.
-                safeOpStructHash := keccak256(encodedSafeOp, 416)
+                // 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(), safeOpStructHash);

modules/4337/package.json

@@ -48,7 +48,7 @@
     "url": "https://github.com/safe-global/safe-modules/issues"
   },
   "devDependencies": {
-    "@account-abstraction/contracts": "github:5afe/account-abstraction#a476d9da6342cf827d0a458c2fb6356e0f015342",
+    "@account-abstraction/contracts": "github:5afe/account-abstraction#prerelease/2024-02-12",
     "@noble/curves": "^1.3.0",
     "@nomicfoundation/hardhat-toolbox": "^4.0.0",
     "@openzeppelin/contracts": "^5.0.2",

modules/4337/scripts/runOp.ts

@@ -1,7 +1,7 @@
-import { BigNumberish, Result } from 'ethers'
+import { Result } from 'ethers'
 import { ethers } from 'hardhat'
 
-import { getRequiredPrefund, getSupportedEntryPoints } from '../src/utils/userOp'
+import { UserOperation, getRequiredPrefund, getSupportedEntryPoints } from '../src/utils/userOp'
 import { chainId } from '../test/utils/encoding'
 import { getSafe4337Module } from '../test/utils/setup'
 import { GlobalConfig, MultiProvider4337, Safe4337 } from '../src/utils/safe'
@@ -114,16 +114,15 @@ const runOp = async () => {
       {
         from: entryPoint,
         to: safe.address,
-        data: buildData('validateUserOp((address,uint256,bytes,bytes,bytes32,uint256,uint256,uint256,bytes,bytes),bytes32,uint256)', [
+        data: buildData('validateUserOp((address,uint256,bytes,bytes,bytes32,uint256,bytes32,bytes,bytes),bytes32,uint256)', [
           [
             userOp.sender,
             userOp.nonce,
             userOp.initCode,
             userOp.callData,
             userOp.accountGasLimits,
             userOp.preVerificationGas,
-            userOp.maxFeePerGas,
-            userOp.maxPriorityFeePerGas,
+            userOp.gasFees,
             userOp.paymasterAndData,
             userOp.signature,
           ],

modules/4337/src/utils/safe.ts

@@ -2,7 +2,7 @@ import { AddressLike, JsonRpcProvider, Provider, Signer, ethers } from 'ethers'
 
 // Import from Safe contracts repo once it is upgraded to ethers v6 and can be installed via npm
 import { MetaTransaction, SafeSignature, SignedSafeTransaction, buildSignatureBytes } from './execution'
-import { UserOperation, EIP712_SAFE_OPERATION_TYPE, packAccountGasLimits } from './userOp'
+import { UserOperation, EIP712_SAFE_OPERATION_TYPE, packGasParameters } from './userOp'
 
 const AddressOne = '0x0000000000000000000000000000000000000001'
 
@@ -26,11 +26,11 @@ const INTERFACES = new ethers.Interface([
 export interface OperationParams {
   nonce: bigint
   initCode: string
-  preVerificationGas: bigint
   verificationGasLimit: bigint
   callGasLimit: bigint
-  maxFeePerGas: bigint
+  preVerificationGas: bigint
   maxPriorityFeePerGas: bigint
+  maxFeePerGas: bigint
   paymasterAndData: string
   validAfter: bigint
   validUntil: bigint
@@ -158,13 +158,13 @@ export class Safe4337Operation {
   }
 
   async userOperation(paymasterAndData = '0x'): Promise<UserOperation> {
+    const { accountGasLimits, gasFees } = packGasParameters(this.params)
     return {
       nonce: ethers.toBeHex(this.params.nonce),
       callData: actionCalldata(this.action),
-      accountGasLimits: packAccountGasLimits(this.params.verificationGasLimit, this.params.callGasLimit),
+      accountGasLimits: ethers.hexlify(accountGasLimits),
       preVerificationGas: ethers.toBeHex(this.params.preVerificationGas),
-      maxFeePerGas: ethers.toBeHex(this.params.maxFeePerGas),
-      maxPriorityFeePerGas: ethers.toBeHex(this.params.maxPriorityFeePerGas),
+      gasFees: ethers.hexlify(gasFees),
       initCode: this.params.initCode,
       paymasterAndData,
       sender: this.safe.address,

modules/4337/src/utils/userOp.ts

@@ -1,26 +1,31 @@
 import { BigNumberish, BytesLike, Contract, Signer, ethers } from 'ethers'
 import { PackedUserOperationStruct as UserOperation } from '../../typechain-types/contracts/Safe4337Module'
 import { SafeSignature } from './execution'
+
 export { UserOperation }
 
 type OptionalExceptFor<T, TRequired extends keyof T = keyof T> = Partial<Pick<T, Exclude<keyof T, TRequired>>> &
   Required<Pick<T, TRequired>>
 
-type SafeUserOperation = { safe: string; entryPoint: string; validAfter: BigNumberish; validUntil: BigNumberish } & Omit<
-  UserOperation,
-  'sender' | 'signature'
->
+type SafeUserOperation = {
+  safe: string
+  entryPoint: string
+  validAfter: BigNumberish
+  validUntil: BigNumberish
+} & GasParameters &
+  Omit<UserOperation, 'sender' | 'signature' | keyof PackedGasParameters>
 
 export const EIP712_SAFE_OPERATION_TYPE = {
   SafeOp: [
     { type: 'address', name: 'safe' },
     { type: 'uint256', name: 'nonce' },
     { type: 'bytes', name: 'initCode' },
     { type: 'bytes', name: 'callData' },
-    { type: 'bytes32', name: 'accountGasLimits' },
+    { type: 'uint128', name: 'verificationGasLimit' },
+    { type: 'uint128', name: 'callGasLimit' },
     { type: 'uint256', name: 'preVerificationGas' },
-    { type: 'uint256', name: 'maxFeePerGas' },
-    { type: 'uint256', name: 'maxPriorityFeePerGas' },
+    { type: 'uint128', name: 'maxPriorityFeePerGas' },
+    { type: 'uint128', name: 'maxFeePerGas' },
     { type: 'bytes', name: 'paymasterAndData' },
     { type: 'uint48', name: 'validAfter' },
     { type: 'uint48', name: 'validUntil' },
@@ -57,7 +62,8 @@ export const buildSafeUserOp = (template: OptionalExceptFor<SafeUserOperation, '
     nonce: template.nonce,
     initCode: template.initCode ?? '0x',
     callData: template.callData ?? '0x',
-    accountGasLimits: template.accountGasLimits ?? packAccountGasLimits(500000, 2000000),
+    verificationGasLimit: template.verificationGasLimit ?? 500000,
+    callGasLimit: template.callGasLimit ?? 2000000,
     preVerificationGas: template.preVerificationGas ?? 60000,
     // 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
@@ -141,10 +147,8 @@ export const buildUserOperationFromSafeUserOperation = ({
     nonce: ethers.toBeHex(safeOp.nonce),
     initCode: ethers.hexlify(safeOp.initCode),
     callData: ethers.hexlify(safeOp.callData),
-    accountGasLimits: safeOp.accountGasLimits,
     preVerificationGas: ethers.toBeHex(safeOp.preVerificationGas),
-    maxFeePerGas: ethers.toBeHex(safeOp.maxFeePerGas),
-    maxPriorityFeePerGas: ethers.toBeHex(safeOp.maxPriorityFeePerGas),
+    ...packGasParameters(safeOp),
     paymasterAndData: ethers.hexlify(safeOp.paymasterAndData),
     signature: ethers.solidityPacked(['uint48', 'uint48', 'bytes'], [safeOp.validAfter, safeOp.validUntil, signature]),
   }
@@ -155,14 +159,15 @@ export const getRequiredGas = (userOp: UserOperation): string => {
   if (userOp.paymasterAndData === '0x') {
     multiplier = 1n
   }
-  const { validationGasLimit, callGasLimit } = unpackAccountGasLimits(userOp.accountGasLimits)
+  const { verificationGasLimit, callGasLimit } = unpackGasParameters(userOp)
 
-  return (BigInt(callGasLimit) + BigInt(validationGasLimit) * multiplier + BigInt(userOp.preVerificationGas)).toString()
+  return (BigInt(callGasLimit) + BigInt(verificationGasLimit) * multiplier + BigInt(userOp.preVerificationGas)).toString()
 }
 
 export const getRequiredPrefund = (userOp: UserOperation): string => {
   const requiredGas = getRequiredGas(userOp)
-  const requiredPrefund = (BigInt(requiredGas) * BigInt(userOp.maxFeePerGas)).toString()
+  const { maxFeePerGas } = unpackGasParameters(userOp)
+  const requiredPrefund = (BigInt(requiredGas) * BigInt(maxFeePerGas)).toString()
   console.log({ requiredGas, requiredPrefund })
 
   return requiredPrefund
@@ -192,15 +197,31 @@ export const packValidationData = (authorizer: BigNumberish, validUntil: BigNumb
   return result
 }
 
+export interface GasParameters {
+  verificationGasLimit: BigNumberish
+  callGasLimit: BigNumberish
+  maxPriorityFeePerGas: BigNumberish
+  maxFeePerGas: BigNumberish
+}
+
+export interface PackedGasParameters {
+  accountGasLimits: BytesLike
+  gasFees: BytesLike
+}
+
 /**
  * Packs two 128uint gas limit values (validationGasLimit and callGasLimit) into a hex-encoded bytes32 string.
  *
  * @param validationGasLimit - The validation gas limit.
  * @param callGasLimit - The call gas limit.
  * @returns The packed gas limits as a string.
  */
-export const packAccountGasLimits = (validationGasLimit: BigNumberish, callGasLimit: BigNumberish): string => {
-  return ethers.solidityPacked(['uint128', 'uint128'], [validationGasLimit, callGasLimit])
+export const packGasParameters = (unpacked: GasParameters): PackedGasParameters => {
+  const pack = (hi: BigNumberish, lo: BigNumberish) => ethers.solidityPacked(['uint128', 'uint128'], [hi, lo])
+  return {
+    accountGasLimits: pack(unpacked.verificationGasLimit, unpacked.callGasLimit),
+    gasFees: pack(unpacked.maxPriorityFeePerGas, unpacked.maxFeePerGas),
+  }
 }
 
 /**
@@ -209,19 +230,15 @@ export const packAccountGasLimits = (validationGasLimit: BigNumberish, callGasLi
  * @param accountGasLimits - The account gas limits as a bytes32 hex-encoded string.
  * @returns An object containing the validation gas limit and the call gas limit.
  */
-export const unpackAccountGasLimits = (accountGasLimits: BytesLike): { validationGasLimit: bigint; callGasLimit: bigint } => {
-  // Ensure the bytes have the expected length.
-  if (ethers.dataLength(accountGasLimits) !== 32) {
-    throw new Error('Invalid input: account gas limits must be 32-bytes long')
+export const unpackGasParameters = (packed: PackedGasParameters): GasParameters => {
+  const unpack = (word: BytesLike) => {
+    if (ethers.dataLength(word) !== 32) {
+      throw new Error('Invalid input: packed gas parameter value must be 32-bytes')
+    }
+    return [BigInt(ethers.dataSlice(word, 0, 16)), ethers.dataSlice(word, 16, 32)] as const
   }
+  const [verificationGasLimit, callGasLimit] = unpack(packed.accountGasLimits)
+  const [maxPriorityFeePerGas, maxFeePerGas] = unpack(packed.gasFees)
 
-  // Split the hex string into two parts (32 characters each)
-  const validationGasHex = ethers.dataSlice(accountGasLimits, 0, 16)
-  const callGasHex = ethers.dataSlice(accountGasLimits, 16, 32)
-
-  // Convert hex values to BigInts
-  const validationGasLimit = BigInt(validationGasHex)
-  const callGasLimit = BigInt(callGasHex)
-
-  return { validationGasLimit, callGasLimit }
+  return { verificationGasLimit, callGasLimit, maxPriorityFeePerGas, maxFeePerGas }
 }