Signature length check

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;
 

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,

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 Indicates the number of iterations to perform in the loop.
+     * @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,18 @@ 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);
     }
 
     /**

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

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

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",

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