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 _checkSignatureLength(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 _checkSignatureLength(bytes calldata, uint256) internal returns(bool) => ALWAYS(true);
 }
 
 rule validationDataLastBitOneIfCheckSignaturesFails(address sender,

modules/4337/contracts/Safe4337Module.sol

@@ -211,6 +211,43 @@ 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.
+     * @param signatures signatures data
+     * @param threshold Indicates the number of iterations to perform in the loop.
+     * @return result True if length check passes, false otherwise.
+     */
+    function _checkSignatureLength(bytes calldata signatures, uint256 threshold) internal pure returns (bool) {
+        uint256 offset = threshold * 0x41;
+
+        if (signatures.length < offset) return false;
+        bool pointsAtEnd = true;
+        for (uint256 i = 0; i < threshold; i++) {
+            /* solhint-disable no-inline-assembly */
+            /// @solidity memory-safe-assembly
+            assembly {
+                let signaturePos := mul(0x41, i)
+                // read the Safe signature type (uint8) from calldata.
+                let signatureType := byte(0, calldataload(add(signatures.offset, add(signaturePos, 0x40))))
+
+                // signatureType = 0 indicates that signature is a smart contract signature in Safe Signature Encoding
+                if iszero(signatureType) {
+                    // For Safe smart contract signature the s value points to the start of the signature data in the signatures
+                    // s value is relative to the signature data.
+                    let signatureStartPointer := calldataload(add(signatures.offset, add(signaturePos, 0x20)))
+                    pointsAtEnd := eq(signatureStartPointer, offset)
+                    let contractSignatureLen := calldataload(add(signatures.offset, signatureStartPointer))
+                    // Update the required position of the next offset.
+                    offset := add(add(signatureStartPointer, 0x20), contractSignatureLen)
+                }
+            }
+            /* solhint-enable no-inline-assembly */
+            // If the signature data pointer is not pointing to the expected location, return false.
+            if (!pointsAtEnd) return false;
+        }
+        return signatures.length <= offset;
+    }
+
     /**
      * @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data
      * of `validAfter || validUntil || authorizer`:
@@ -222,10 +259,20 @@ 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) {
+
+        // checkSignatures function in Safe contract does not force a fixed size on signature length.
+        // A malicious actor can append additional bytes in the signature than needed and force gas usage to reach verificationGasLimit.
+        // _checkSignatureLength ensures that there are no additional bytes in the signatures than required.
+        bool validSignature = _checkSignatureLength(signatures, ISafe(payable(userOp.sender)).getThreshold());
+
+        try ISafe(payable(userOp.sender)).checkSignatures(keccak256(operationData), operationData, signatures) {} catch {
+            validSignature = false;
+        }
+
+        if (validSignature) {
             // The timestamps are validated by the entry point, therefore we will not check them again
             validationData = _packValidationData(false, validUntil, validAfter);
-        } catch {
+        } else {
             validationData = _packValidationData(true, 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,91 @@ 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 not be able to execute contract calls twice', async () => {
       const { user1, safe, validator, entryPoint } = await setupTests()
 

modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts

@@ -36,7 +36,7 @@ describe('Safe4337Module - Reference EntryPoint', () => {
       proxyCreationCode,
       chainId: Number(await chainId()),
     }
-    const safe = await Safe4337.withSigner(user.address, safeGlobalConfig)
+    const safe = Safe4337.withSigner(user.address, safeGlobalConfig)
 
     return {
       user,
@@ -156,7 +156,7 @@ describe('Safe4337Module - Reference EntryPoint', () => {
     const { user, relayer, safe: parentSafe, validator, entryPoint, safeGlobalConfig } = await setupTests()
 
     await parentSafe.deploy(user)
-    const daughterSafe = await Safe4337.withSigner(parentSafe.address, safeGlobalConfig)
+    const daughterSafe = Safe4337.withSigner(parentSafe.address, safeGlobalConfig)
 
     const accountBalance = ethers.parseEther('1.0')
     await user.sendTransaction({ to: daughterSafe.address, value: accountBalance })
@@ -215,6 +215,92 @@ describe('Safe4337Module - Reference EntryPoint', () => {
     expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance - transfer - deposits)
   })
 
+  it('should revert on invalid signature length - EOA signature', async () => {
+    const { user, relayer, safe, validator, entryPoint } = await setupTests()
+
+    const accountBalance = ethers.parseEther('1.0')
+    await user.sendTransaction({ to: safe.address, value: accountBalance })
+    expect(await ethers.provider.getBalance(safe.address)).to.be.eq(accountBalance)
+
+    const safeOp = buildSafeUserOpTransaction(
+      safe.address,
+      user.address,
+      ethers.parseEther('0.1'),
+      '0x',
+      `0`,
+      await entryPoint.getAddress(),
+      false,
+      false,
+      {
+        initCode: safe.getInitCode(),
+      },
+    )
+
+    const signature = buildSignatureBytes([await signSafeOp(user, await validator.getAddress(), safeOp, await chainId())])
+    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 on invalid signature length - Smart contract signature (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 = 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(),
+      false,
+      false,
+      {
+        initCode: daughterSafe.getInitCode(),
+      },
+    )
+
+    const opData = calculateSafeOperationData(await validator.getAddress(), safeOp, await chainId())
+    const signature = buildSignatureBytes([
+      {
+        signer: parentSafe.address,
+        data: await user.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')
+  })
+
   function isEventLog(log: Log): log is EventLog {
     return typeof (log as Partial<EventLog>).eventName === 'string'
   }

modules/4337/test/erc4337/Safe4337Module.spec.ts

@@ -268,6 +268,40 @@ describe('Safe4337Module', () => {
 
       expect(await safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.eq(packedValidationData)
     })
+
+    it('should indicate failed validation data when signature length contains additional bytes', async () => {
+      const { user, safeModule, validator, entryPoint } = await setupTests()
+
+      const validAfter = BigInt(ethers.hexlify(ethers.randomBytes(3)))
+      const validUntil = validAfter + BigInt(ethers.hexlify(ethers.randomBytes(3)))
+
+      const safeOp = buildSafeUserOpTransaction(
+        await safeModule.getAddress(),
+        user.address,
+        0,
+        '0x',
+        '0',
+        await entryPoint.getAddress(),
+        false,
+        false,
+        {
+          validAfter,
+          validUntil,
+        },
+      )
+
+      const safeOpHash = calculateSafeOperationHash(await validator.getAddress(), safeOp, await chainId())
+      const signature = buildSignatureBytes([await signHash(user, safeOpHash)]).concat('00')
+      const userOp = buildPackedUserOperationFromSafeUserOperation({
+        safeOp,
+        signature,
+      })
+      const packedValidationData = packValidationData(1, validUntil, validAfter)
+      const entryPointImpersonator = await ethers.getSigner(await entryPoint.getAddress())
+      const safeFromEntryPoint = safeModule.connect(entryPointImpersonator)
+
+      expect(await safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.eq(packedValidationData)
+    })
   })
 
   describe('execUserOp', () => {