[#754] Add signature length checks in Safe4337Module

modules/4337/contracts/Safe4337Module.sol

@@ -211,6 +211,47 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
         operationHash = keccak256(operationData);
     }
 
+    /**
+     * @dev Checks if the signatures length is correct and does not contain addtional bytes.
+     *      The code uses scratch space to store s and v values of the signatures.
+     *      0x00 stores s value and 0x20 stores v value.
+     *      As checkSignatures is expected to perform, it is skipped here.
+     * @param signatures signatures data
+     * @param threshold Indicates the number of iterations to perform in the loop.
+     * @return bool True if length check passes, false otherwise.
+     */
+    function _checkSignatureLength(bytes calldata signatures, uint256 threshold) internal pure returns (bool) {
+        uint256 offset = threshold * 0x41;
+
+        for (uint256 i = 0; i < threshold; i++) {
+            /* solhint-disable no-inline-assembly */
+            /// @solidity memory-safe-assembly
+            assembly {
+                let signaturePos := mul(0x41, i)
+                calldatacopy(0x00, add(signatures.offset, add(signaturePos, 0x20)), 0x20)
+                let s := mload(0x00)
+                calldatacopy(0x20, add(signatures.offset, add(signaturePos, 0x40)), 0x20)
+                let v := byte(0, mload(0x20))
+
+                if iszero(v) {
+                    // Require that the signature data pointer is pointing to the expected location, at the end of processed contract signatures.
+                    if iszero(eq(s, offset)) {
+                        // If they are not equal, return false
+                        mstore(0x00, 0)
+                        return(0x00, 0x20)
+                    }
+                    // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
+                    let contractSignatureLen := mload(0x40)
+                    calldatacopy(contractSignatureLen, add(signatures.offset, s), 0x20)
+                    // Update the required position of the next offset.
+                    offset := add(add(s, 0x20), mload(contractSignatureLen))
+                }
+            }
+        }
+        if (signatures.length != offset) return false;
+        return true;
+    }
+
     /**
      * @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data
      * of `validAfter || validUntil || authorizer`:
@@ -222,11 +263,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) {
-            // The timestamps are validated by the entry point, therefore we will not check them again
-            validationData = _packValidationData(false, validUntil, validAfter);
-        } catch {
+
+        // address[] memory owners = ISafe(payable(userOp.sender)).getOwners();
+        uint256 threshold = ISafe(payable(userOp.sender)).getThreshold();
+        bool success = _checkSignatureLength(signatures, threshold);
+
+        if (!success) {
             validationData = _packValidationData(true, validUntil, validAfter);
+        } else {
+            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);
+            }
         }
     }
 

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/contracts/test/TestSafe4337Module.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity >=0.8.0;
+import {Safe4337Module} from "../Safe4337Module.sol";
+
+contract TestSafe4337Module is Safe4337Module {
+    constructor(address entryPoint) Safe4337Module(entryPoint) {}
+
+    function checkSignatureLength(bytes calldata signature, uint256 threshold) external pure returns (bool) {
+        return _checkSignatureLength(signature, threshold);
+    }
+}

modules/4337/src/deploy/module.ts

@@ -14,12 +14,6 @@ const deploy: DeployFunction = async ({ deployments, getNamedAccounts }) => {
     log: true,
     deterministicDeployment: true,
   })
-
-  await deploy('SafeL2Mod', {
-    from: deployer,
-    log: true,
-    deterministicDeployment: true,
-  })
 }
 
 deploy.dependencies = ['entrypoint']

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

@@ -215,6 +215,60 @@ describe('Safe4337Module - Reference EntryPoint', () => {
     expect(await ethers.provider.getBalance(daughterSafe.address)).to.be.eq(accountBalance - transfer - deposits)
   })
 
+  it('should revert on invalid signature length (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(),
+      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'), // invalid 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/TestSafe4337Module.spec.ts

@@ -0,0 +1,124 @@
+import { expect } from 'chai'
+import { deployments, ethers } from 'hardhat'
+import { buildSignatureBytes, signHash } from '../../src/utils/execution'
+
+describe('TestSafe4337Module', () => {
+  const setupTests = deployments.createFixture(async () => {
+    const module = await ethers.deployContract('TestSafe4337Module', [ethers.hexlify(ethers.randomBytes(20))])
+    return { module }
+  })
+
+  it('Safe with 1 EOA owner', async () => {
+    const { module } = await setupTests()
+    const [user] = await ethers.getSigners()
+
+    const safeOpHash = ethers.hexlify(ethers.randomBytes(32))
+
+    const signature = buildSignatureBytes([await signHash(user, safeOpHash)])
+
+    const threshold = 1
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.true
+  })
+
+  it('Safe with 1 Safe as owner', async () => {
+    const { module } = await setupTests()
+
+    const signature = buildSignatureBytes([
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+    ])
+
+    const threshold = 1
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.true
+  })
+
+  it('Safe with 1 EOA and 1 Safe as owners', async () => {
+    const { module } = await setupTests()
+    const [user] = await ethers.getSigners()
+
+    const safeOpHash = ethers.hexlify(ethers.randomBytes(32))
+
+    const signature = buildSignatureBytes([
+      await signHash(user, safeOpHash),
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+    ])
+
+    const threshold = 2
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.true
+  })
+
+  it('Safe with 2 EOA and 1 Safe as owners', async () => {
+    const { module } = await setupTests()
+    const [user] = await ethers.getSigners()
+
+    let safeOpHash = ethers.hexlify(ethers.randomBytes(32))
+
+    const signature = buildSignatureBytes([
+      await signHash(user, safeOpHash),
+      await signHash(user, safeOpHash),
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+    ])
+
+    const threshold = 3
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.true
+  })
+
+  it('Safe with 1 EOA and 2 Safe as owners', async () => {
+    const { module } = await setupTests()
+    const [user] = await ethers.getSigners()
+
+    let safeOpHash = ethers.hexlify(ethers.randomBytes(32))
+
+    const signature = buildSignatureBytes([
+      await signHash(user, safeOpHash),
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+    ])
+
+    const threshold = 3
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.true
+  })
+
+  it('Should revert when signature contains additional bytes', async () => {
+    const { module } = await setupTests()
+    const [user] = await ethers.getSigners()
+
+    let safeOpHash = ethers.hexlify(ethers.randomBytes(32))
+
+    const signature = buildSignatureBytes([
+      await signHash(user, safeOpHash),
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+      {
+        signer: ethers.ZeroAddress,
+        data: ethers.hexlify(ethers.randomBytes(65)),
+        dynamic: true,
+      },
+    ]).concat('00')
+
+    const threshold = 3
+    expect(await module.checkSignatureLength(signature, threshold)).to.be.false
+  })
+})