Refactor Length Check Implementation

Here, we refactor the length check to not use assembly (as we didn't
have a compelling argument to do so). Additionally, we added an escape
hatch on the length check validation in order to allow empty signature
(a reasonable "default" value) to not revert on `validateUserOp` for
better developer experience with 4337, which required an additional test
for full coverage.

modules/4337/contracts/Safe4337Module.sol

@@ -220,35 +220,42 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
      * @return isValid True if length check passes, false otherwise.
      */
     function _checkSignaturesLength(bytes calldata signatures, uint256 threshold) internal pure returns (bool isValid) {
-        uint256 expectedOffset = threshold * 0x41;
+        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++) {
-            bool pointsAtEnd = true;
-            /* solhint-disable no-inline-assembly */
-            /// @solidity memory-safe-assembly
-            assembly {
-                let signaturePos := mul(0x41, i)
-                // read the Safe signature type byte from the signatures bytes, this is the 65th byte per signature.
-                // The fixed part of the signature is encoded as {32-bytes signature verifier}{32-bytes dynamic data position}{1-byte signature type}
-                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 second word of static part points to the start of the signature data in the signatures i.e. dynamic part
-                    // The value of the pointer is relative to `signatures`.
-                    let signatureStartPointer := calldataload(add(signatures.offset, add(signaturePos, 0x20)))
-                    pointsAtEnd := eq(signatureStartPointer, expectedOffset)
-                    let contractSignatureLen := calldataload(add(signatures.offset, signatureStartPointer))
-                    // Update the expected offset of the next contract signature. This is the previous expected offset plus the total length of the current contract signature.
-                    // Note that we add 0x20 to the contract signature length to account for the encoded length value.
-                    expectedOffset := add(expectedOffset, add(0x20, contractSignatureLen))
-                }
+            // 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;
             }
-            /* solhint-enable no-inline-assembly */
-            // If the signature data pointer is not pointing to the expected location, return false.
-            if (!pointsAtEnd) return false;
         }
-        isValid = signatures.length <= expectedOffset;
+
+        isValid = signatures.length <= maxLength;
     }
 
     /**

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

@@ -269,6 +269,39 @@ describe('Safe4337Module', () => {
       expect(await safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.eq(packedValidationData)
     })
 
+    it('should fail signature validation when signatures are too short', 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 userOp = buildPackedUserOperationFromSafeUserOperation({
+        safeOp,
+        signature: "0x",
+      })
+      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)
+    })
+
     it('should indicate failed validation data when signature length contains additional bytes', async () => {
       const { user, safeModule, validator, entryPoint } = await setupTests()