Signature length check (#453)

Related to safe-global/safe-smart-account#754 ## Changes in PR - Add a test case that verifies that UserOp is reverted if signature contains additional bytes data - Add `_checkSignatureLength` function in `Safe4437Module` contract . This function retrieves threshold from Safe and iterates over signature data. - Add a test contract which is used in testing the internal function: - Add tests - Summarise `_checkSignatureLength` in FV specs. The prover was reporting error without giving call traces. - Unrelated to issue: [Fix script](https://github.com/safe-global/safe-modules/pull/453/files#diff-d6fef5446f8c9e62fd4180360854e7e830def1aa48a27dda677a99a74fcd6a44R24) in `package.json` ## Open for discussion Should `_checkSignatureLength` also do more stricter checks that are already there in `checkNSignature` function of `Safe`? If `Safe4337Module` does not perform below mentioned checks then it will be implicitly assumed to be done in `Safe` contract and this creates a requirement that all future `Safe` versions to have these checks if used with this `Safe4337Module`. 1. `if (signatures.length < offset)` this check is currently done twice. - In Safe4337Module: https://github.com/safe-global/safe-modules/pull/453/files#diff-8ec9433e2f98b74922f038206ac421a769cf3996997ad55ed210299abddb7908R223 - In Safe: https://github.com/safe-global/safe-smart-account/blob/8f80a8372d193be121dcdb52e869a258824e5c0f/contracts/Safe.sol#L279 2. Other potential check that can be repeated: https://github.com/safe-global/safe-smart-account/blob/8f80a8372d193be121dcdb52e869a258824e5c0f/contracts/Safe.sol#L233 ## Code size change ### This PR ``` Safe4337Module 8910 bytes (limit is 24576) ``` ### Main branch ``` Safe4337Module 8373 bytes (limit is 24576) ``` ## Impact on gas usage ### This PR ``` Gas Metering Safe Deployment + Enabling 4337 Module Used 420045 gas (Account or Paymaster) for >Safe with 4337 Module Deployment< Used 416973 gas (Transaction) for >Safe with 4337 Module Deployment< ✔ Safe with 4337 Module Deployment Safe Deployment + Enabling 4337 Module + Native Transfers Used 451408 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer< Used 449392 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer< ✔ Safe with 4337 Module Deployment + Native Transfer Used 193399 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer< Used 183844 gas (Transaction) for >Safe with 4337 Module Native Transfer< ✔ Safe with 4337 Module Native Transfer Safe Deployment + Enabling 4337 Module + Token Operations Used 436781 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer< Used 427911 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer< ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer Used 471043 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer< Used 469710 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer< ✔ Safe with 4337 Module Deployment + ERC721 Token Minting Token Operations Only Used 177996 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer< Used 162338 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer< ✔ Safe with 4337 Module ERC20 Token Transfer Used 213064 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Token Minting< Used 204125 gas (Transaction) for >Safe with 4337 Module ERC721 Token Minting< ✔ Safe with 4337 Module ERC721 Token Minting ``` ### Main branch ``` Gas Metering Safe Deployment + Enabling 4337 Module Used 419096 gas (Account or Paymaster) for >Safe with 4337 Module Deployment< Used 414864 gas (Transaction) for >Safe with 4337 Module Deployment< ✔ Safe with 4337 Module Deployment Safe Deployment + Enabling 4337 Module + Native Transfers Used 450537 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer< Used 447308 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer< ✔ Safe with 4337 Module Deployment + Native Transfer Used 192424 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer< Used 182182 gas (Transaction) for >Safe with 4337 Module Native Transfer< ✔ Safe with 4337 Module Native Transfer Safe Deployment + Enabling 4337 Module + Token Operations Used 435685 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer< Used 425687 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer< ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer Used 470086 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer< Used 467491 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer< ✔ Safe with 4337 Module Deployment + ERC721 Token Minting Token Operations Only Used 176846 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer< Used 160584 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer< ✔ Safe with 4337 Module ERC20 Token Transfer Used 212013 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Token Minting< Used 202376 gas (Transaction) for >Safe with 4337 Module ERC721 Token Minting< ✔ Safe with 4337 Module ERC721 Token Minting ``` --------- Co-authored-by: Nicholas Rodrigues Lordello <nick@safe.global> Co-authored-by: Shebin John <admin@remedcu.com>
safe-global · Jul 12, 2024 · c3a4d06 · c3a4d06
1 parent c3f6d36
commit c3a4d06
Show file tree

Hide file tree

Showing 9 changed files with 550 additions and 10 deletions.
diff --git a/modules/4337/certora/specs/Safe4337Module.spec b/modules/4337/certora/specs/Safe4337Module.spec
@@ -25,6 +25,7 @@ methods {
     function getOperationHash(
         Safe4337Module.PackedUserOperation userOp
     ) external returns(bytes32) envfree => PER_CALLEE_CONSTANT;
+    function _checkSignaturesLength(bytes calldata, uint256) internal returns(bool) => ALWAYS(true);
 }
 persistent ghost ERC2771MessageSender() returns address;
 

diff --git a/modules/4337/certora/specs/ValidationDataLastBitOne.spec b/modules/4337/certora/specs/ValidationDataLastBitOne.spec
@@ -11,6 +11,7 @@ methods {
     function getOperationHash(
         Safe4337Module.PackedUserOperation userOp
     ) external returns(bytes32) envfree => PER_CALLEE_CONSTANT;
+    function _checkSignaturesLength(bytes calldata, uint256) internal returns(bool) => ALWAYS(true);
 }
 
 rule validationDataLastBitOneIfCheckSignaturesFails(address sender,

diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol
@@ -211,6 +211,53 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
         operationHash = keccak256(operationData);
     }
 
+    /**
+     * @dev Checks if the signatures length is correct and does not contain additional bytes. The function does not
+     * check the integrity of the signature encoding, as this is expected to be checked by the {Safe} implementation
+     * of {checkSignatures}.
+     * @param signatures Signatures data.
+     * @param threshold Signer threshold for the Safe account.
+     * @return isValid True if length check passes, false otherwise.
+     */
+    function _checkSignaturesLength(bytes calldata signatures, uint256 threshold) internal pure returns (bool isValid) {
+        uint256 maxLength = threshold * 0x41;
+
+        // Make sure that `signatures` bytes are at least as long as the static part of the signatures for the specified
+        // threshold (i.e. we have at least 65 bytes per signer). This avoids out-of-bound access reverts when decoding
+        // the signature in order to adhere to the ERC-4337 specification.
+        if (signatures.length < maxLength) {
+            return false;
+        }
+
+        for (uint256 i = 0; i < threshold; i++) {
+            // Each signature is 0x41 (65) bytes long, where fixed part of a Safe contract signature is encoded as:
+            //      {32-bytes signature verifier}{32-bytes dynamic data position}{1-byte signature type}
+            // and the dynamic part is encoded as:
+            //      {32-bytes signature length}{bytes signature data}
+            //
+            // For each signature we check whether or not the signature is a contract signature (signature type of 0).
+            // If it is, we need to read the length of the contract signature bytes from the signature data, and add it
+            // to the maximum signatures length.
+            //
+            // In order to keep the implementation simpler, and unlike in the length check above, we intentionally
+            // revert here on out-of-bound bytes array access as well as arithmetic overflow, as you would have to
+            // **intentionally** build invalid `signatures` data to trigger these conditions. Furthermore, there are no
+            // security issues associated with reverting in these cases, just not optimally following the ERC-4337
+            // standard (specifically: "SHOULD return `SIG_VALIDATION_FAILED` (and not revert) on signature mismatch").
+
+            uint256 signaturePos = i * 0x41;
+            uint8 signatureType = uint8(signatures[signaturePos + 0x40]);
+
+            if (signatureType == 0) {
+                uint256 signatureOffset = uint256(bytes32(signatures[signaturePos + 0x20:]));
+                uint256 signatureLength = uint256(bytes32(signatures[signatureOffset:]));
+                maxLength += 0x20 + signatureLength;
+            }
+        }
+
+        isValid = signatures.length <= maxLength;
+    }
+
     /**
      * @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data
      * of `validAfter || validUntil || authorizer`:
@@ -222,12 +269,19 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
      */
     function _validateSignatures(PackedUserOperation calldata userOp) internal view returns (uint256 validationData) {
         (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp);
-        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 {
-            validationData = _packValidationData(true, validUntil, validAfter);
+
+        // The `checkSignatures` function in the Safe contract does not force a fixed size on signature length.
+        // A malicious bundler can pad the Safe operation `signatures` with additional bytes, causing the account to pay
+        // more gas than needed for user operation validation (capped by `verificationGasLimit`).
+        // `_checkSignaturesLength` ensures that there are no additional bytes in the `signature` than are required.
+        bool validSignature = _checkSignaturesLength(signatures, ISafe(payable(userOp.sender)).getThreshold());
+
+        try ISafe(payable(userOp.sender)).checkSignatures(keccak256(operationData), operationData, signatures) {} catch {
+            validSignature = false;
         }
+
+        // The timestamps are validated by the entry point, therefore we will not check them again.
+        validationData = _packValidationData(!validSignature, validUntil, validAfter);
     }
 
     /**

diff --git a/modules/4337/contracts/interfaces/Safe.sol b/modules/4337/contracts/interfaces/Safe.sol
@@ -56,4 +56,10 @@ interface ISafe {
      * @param module Module to be enabled.
      */
     function enableModule(address module) external;
+
+    /**
+     * @notice Returns the number of required confirmations for a Safe transaction aka the threshold.
+     * @return Threshold number.
+     */
+    function getThreshold() external view returns (uint256);
 }
diff --git a/modules/4337/contracts/test/SafeMock.sol b/modules/4337/contracts/test/SafeMock.sol
@@ -72,6 +72,10 @@ contract SafeMock {
         else (success, returnData) = to.call{value: value}(data);
     }
 
+    function getThreshold() external pure returns (uint256) {
+        return 1;
+    }
+
     // solhint-disable-next-line payable-fallback,no-complex-fallback
     fallback() external payable {
         // solhint-disable-next-line no-inline-assembly

diff --git a/modules/4337/package.json b/modules/4337/package.json
@@ -21,7 +21,7 @@
     "test:all": "pnpm run test && npm run test:4337",
     "coverage": "hardhat coverage",
     "codesize": "hardhat codesize",
-    "benchmark": "pnpm run test benchmark/*.ts",
+    "benchmark": "pnpm run test test/gas/*.ts",
     "deploy-all": "hardhat deploy-contracts --network",
     "deploy": "hardhat deploy --network",
     "lint": "pnpm run lint:sol && npm run lint:ts",

diff --git a/modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts b/modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts
@@ -12,6 +12,7 @@ import { buildSignatureBytes, logUserOperationGas } from '../../src/utils/execut
 import {
   buildPackedUserOperationFromSafeUserOperation,
   buildSafeUserOpTransaction,
+  calculateSafeOperationData,
   getRequiredPrefund,
   signSafeOp,
 } from '../../src/utils/userOp'
@@ -31,15 +32,17 @@ describe('Safe4337Module - Newly deployed safe', () => {
     const proxyCreationCode = await proxyFactory.proxyCreationCode()
     const safeModuleSetup = await getSafeModuleSetup()
     const singleton = await getSafeL2Singleton()
-    const safe = await Safe4337.withSigner(user1.address, {
+    const safeGlobalConfig = {
       safeSingleton: await singleton.getAddress(),
       entryPoint: await entryPoint.getAddress(),
       erc4337module: await module.getAddress(),
       proxyFactory: await proxyFactory.getAddress(),
       safeModuleSetup: await safeModuleSetup.getAddress(),
       proxyCreationCode,
       chainId: Number(await chainId()),
-    })
+    }
+
+    const safe = Safe4337.withSigner(user1.address, safeGlobalConfig)
 
     return {
       user1,
@@ -50,6 +53,7 @@ describe('Safe4337Module - Newly deployed safe', () => {
       entryPoint,
       entryPointSimulations,
       relayer,
+      safeGlobalConfig,
     }
   })
 
@@ -119,6 +123,149 @@ describe('Safe4337Module - Newly deployed safe', () => {
       expect(await ethers.provider.getBalance(safe.address)).to.be.eq(ethers.parseEther('0'))
     })
 
+    it('should revert when signature length contains additional bytes - EOA signature', async () => {
+      const { user1, safe, validator, entryPoint } = await setupTests()
+
+      await entryPoint.depositTo(safe.address, { value: ethers.parseEther('1.0') })
+
+      await user1.sendTransaction({ to: safe.address, value: ethers.parseEther('0.5') })
+      const safeOp = buildSafeUserOpTransaction(
+        safe.address,
+        user1.address,
+        ethers.parseEther('0.5'),
+        '0x',
+        '0',
+        await entryPoint.getAddress(),
+        false,
+        false,
+        {
+          initCode: safe.getInitCode(),
+        },
+      )
+
+      // Add additional byte to the signature to make signature length invalid
+      const signature = buildSignatureBytes([await signSafeOp(user1, await validator.getAddress(), safeOp, await chainId())]).concat('00')
+      const userOp = buildPackedUserOperationFromSafeUserOperation({
+        safeOp,
+        signature,
+      })
+      await expect(entryPoint.handleOps([userOp], user1.address))
+        .to.be.revertedWithCustomError(entryPoint, 'FailedOp')
+        .withArgs(0, 'AA24 signature error')
+    })
+
+    it('should revert when signature length contains additional bytes - Smart contract signature', async () => {
+      const { user1, relayer, safe: parentSafe, validator, entryPoint, safeGlobalConfig } = await setupTests()
+
+      await parentSafe.deploy(user1)
+
+      const daughterSafe = Safe4337.withSigner(parentSafe.address, safeGlobalConfig)
+
+      const accountBalance = ethers.parseEther('1.0')
+      await user1.sendTransaction({ to: daughterSafe.address, value: accountBalance })
+      expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance)
+
+      const safeOp = buildSafeUserOpTransaction(
+        daughterSafe.address,
+        user1.address,
+        ethers.parseEther('0.1'),
+        '0x',
+        '0x0',
+        await entryPoint.getAddress(),
+        false,
+        false,
+        {
+          initCode: daughterSafe.getInitCode(),
+        },
+      )
+
+      const opData = calculateSafeOperationData(await validator.getAddress(), safeOp, await chainId())
+      const signature = buildSignatureBytes([
+        {
+          signer: parentSafe.address,
+          data: await user1.signTypedData(
+            {
+              verifyingContract: parentSafe.address,
+              chainId: await chainId(),
+            },
+            {
+              SafeMessage: [{ type: 'bytes', name: 'message' }],
+            },
+            {
+              message: opData,
+            },
+          ),
+          dynamic: true,
+        },
+      ])
+      const userOp = buildPackedUserOperationFromSafeUserOperation({
+        safeOp,
+        signature: signature.concat('00'), // adding '00' invalidates signature length
+      })
+
+      await expect(entryPoint.handleOps([userOp], await relayer.getAddress()))
+        .to.be.revertedWithCustomError(entryPoint, 'FailedOp')
+        .withArgs(0, 'AA24 signature error')
+    })
+
+    it('should revert when signature offset points to invalid part of signature data - Smart contract signature', async () => {
+      const { user1, relayer, safe: parentSafe, validator, entryPoint, safeGlobalConfig } = await setupTests()
+
+      await parentSafe.deploy(user1)
+
+      const daughterSafe = Safe4337.withSigner(parentSafe.address, safeGlobalConfig)
+
+      const accountBalance = ethers.parseEther('1.0')
+      await user1.sendTransaction({ to: daughterSafe.address, value: accountBalance })
+      expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance)
+
+      const safeOp = buildSafeUserOpTransaction(
+        daughterSafe.address,
+        user1.address,
+        ethers.parseEther('0.1'),
+        '0x',
+        '0x0',
+        await entryPoint.getAddress(),
+        false,
+        false,
+        {
+          initCode: daughterSafe.getInitCode(),
+        },
+      )
+
+      const opData = calculateSafeOperationData(await validator.getAddress(), safeOp, await chainId())
+      const parentSafeSignature = await user1.signTypedData(
+        {
+          verifyingContract: parentSafe.address,
+          chainId: await chainId(),
+        },
+        {
+          SafeMessage: [{ type: 'bytes', name: 'message' }],
+        },
+        {
+          message: opData,
+        },
+      )
+
+      // The 2nd word of static part of signature containing invalid value pointing to dynamic part
+      const signature = ethers.concat([
+        ethers.zeroPadValue(parentSafe.address, 32), // address of the signer
+        ethers.toBeHex(0, 32), // offset of the start of the signature
+        '0x00', // contract signature type
+        ethers.toBeHex(ethers.dataLength(parentSafeSignature), 32), // length of the dynamic signature
+        parentSafeSignature,
+      ])
+
+      const userOp = buildPackedUserOperationFromSafeUserOperation({
+        safeOp,
+        signature,
+      })
+
+      await expect(entryPoint.handleOps([userOp], await relayer.getAddress()))
+        .to.be.revertedWithCustomError(entryPoint, 'FailedOp')
+        .withArgs(0, 'AA24 signature error')
+    })
+
     it('should not be able to execute contract calls twice', async () => {
       const { user1, safe, validator, entryPoint } = await setupTests()