From 405a5badbe1d4ab3acad2eeb92421eb8800ad483 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Thu, 11 Jul 2024 18:28:31 +0200 Subject: [PATCH] Cleanup Post-Review --- modules/4337/contracts/Safe4337Module.sol | 9 +++++---- modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts | 2 +- modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts | 2 +- modules/4337/test/erc4337/Safe4337Module.spec.ts | 6 +++++- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index daba60be4..44256bd14 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -215,8 +215,8 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @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. + * @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) { @@ -271,7 +271,8 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); // 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`). + // 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()); @@ -279,7 +280,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle validSignature = false; } - // The timestamps are validated by the entry point, therefore we will not check them again + // 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/test/erc4337/ERC4337ModuleNew.spec.ts b/modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts index 148a3477a..4e3193e97 100644 --- a/modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts +++ b/modules/4337/test/erc4337/ERC4337ModuleNew.spec.ts @@ -208,7 +208,7 @@ describe('Safe4337Module - Newly deployed safe', () => { .withArgs(0, 'AA24 signature error') }) - it('should revert when signature pointer points to invalid part of signature data - Smart contract signature', async () => { + 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) diff --git a/modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts b/modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts index bf23b1d94..43fc70c95 100644 --- a/modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts +++ b/modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts @@ -301,7 +301,7 @@ describe('Safe4337Module - Reference EntryPoint', () => { .withArgs(0, 'AA24 signature error') }) - it('should revert when signature pointer points to invalid part of signature data - Smart contract signature', async () => { + it('should revert when signature offset points to invalid part of signature data - Smart contract signature', async () => { const { user, relayer, safe: parentSafe, validator, entryPoint, safeGlobalConfig } = await setupTests() await parentSafe.deploy(user) diff --git a/modules/4337/test/erc4337/Safe4337Module.spec.ts b/modules/4337/test/erc4337/Safe4337Module.spec.ts index a501046fc..1233e7f9c 100644 --- a/modules/4337/test/erc4337/Safe4337Module.spec.ts +++ b/modules/4337/test/erc4337/Safe4337Module.spec.ts @@ -359,7 +359,11 @@ describe('Safe4337Module', () => { const userOp = buildPackedUserOperationFromSafeUserOperation({ safeOp, - signature: ethers.hexlify(ethers.randomBytes(32)) + '00'.padStart(64, '0') + '00' + ethers.hexlify(ethers.randomBytes(65)).slice(2), + signature: ethers.concat([ + ethers.randomBytes(32), + ethers.toBeHex(0, 32), // point to start of the signatures bytes + '0x00', // contract signature type + ]), }) const packedValidationData = packValidationData(1, validUntil, validAfter) const entryPointImpersonator = await ethers.getSigner(await entryPoint.getAddress())