From 120220b3ea08e04b5e2ea921dc013d2298313cab Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 27 Aug 2024 10:27:12 +0200 Subject: [PATCH] Add comment in _checkSignaturesLength regarding dynamic signatures (#471) This PR adds a comment on dynamic signatures and signature length manipulation in `_checkSignaturesLength` Natspec --------- Co-authored-by: Nicholas Rodrigues Lordello --- modules/4337/contracts/Safe4337Module.sol | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 44256bd14..2a860db1d 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -212,9 +212,23 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle } /** - * @dev Checks if the signatures length is correct and does not contain additional bytes. The function does not + * @notice 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}. + * @dev Safe account has two types of signatures: EOA and Smart Contract signatures. While the EOA signature is + * fixed in size, the Smart Contract signature can be of arbitrary length. If appropriate length checks are not + * performed during the signature verification then a malicious bundler can pad additional bytes to the signatures + * data and make the account pay more gas than needed for user operation validation and reach the + * `verificationGasLimit`. _checkSignaturesLength ensures that the signatures data cannot be longer than the + * canonical encoding of Safe signatures, thus setting a strict upper bound on how long the signatures bytes can + * be, greatly limiting a malicious bundler's ability to pad signature bytes. However, there is an edge case that + * `_checkSignaturesLength` function cannot detect. + * Signatures data for Smart Contracts contains a dynamic part that is encoded as: + * {32-bytes signature length}{bytes signature data} + * A malicious bundler can manipulate the field(s) storing the signature length and pad additional bytes to the + * dynamic part of the signatures which will make `_checkSignaturesLength` to return true. In such cases, it is + * the responsibility of the Safe signature validator implementation, as an account owner, to check for additional + * bytes. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise.