From 229d1ff600eacc930bb1f426be44f90b57d18eab Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 22 Jul 2024 11:04:47 +0200 Subject: [PATCH 01/12] Add comment in _checkSignaturesLength regarding dynamic signatures --- modules/4337/contracts/Safe4337Module.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 44256bd1..d7e23abf 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -212,9 +212,17 @@ 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 A malicious bundler can pad additional bytes to the `signatures` data, causing the account to pay more gas + * than needed for user operation validation. 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. + * Safe encodes the Smart Contract signature length in the signature data. Since, the `signature` field in UserOp + * is not part of the UserOp hash a malicious bundler can manipulate the field 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 signature verifier to check for additional padded bytes to the + * signatures data. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise. From 36c0742db90438c850b1cf19403e0fb346418fec Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 23 Jul 2024 11:51:12 +0200 Subject: [PATCH 02/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index d7e23abf..584333e6 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -215,11 +215,14 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @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 A malicious bundler can pad additional bytes to the `signatures` data, causing the account to pay more gas - * than needed for user operation validation. 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. - * Safe encodes the Smart Contract signature length in the signature data. Since, the `signature` field in UserOp - * is not part of the UserOp hash a malicious bundler can manipulate the field storing the signature length and pad + * @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. Safe encodes the Smart Contract + * signature length in the signature data. A malicious bundler can pad additional bytes to the `signature` data, + * causing the account to pay more gas than needed for user operation validation and reach the + * verificationGasLimit if the verifier does not implement appropriate length checks. `_checkSignaturesLength` + * function checks for presence of any padded bytes to the `signature` data. However, there is an + * edge case that `_checkSignaturesLength` function cannot detect. Since, the `signature` field in UserOp is not + * part of the UserOp hash a malicious bundler can manipulate the field 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 signature verifier to check for additional padded bytes to the * signatures data. From a50f197ad3c46ecb36e61b15466ce958bab26167 Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 23 Jul 2024 16:24:38 +0200 Subject: [PATCH 03/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 584333e6..249e4d29 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -217,15 +217,16 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * 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. Safe encodes the Smart Contract - * signature length in the signature data. A malicious bundler can pad additional bytes to the `signature` data, - * causing the account to pay more gas than needed for user operation validation and reach the - * verificationGasLimit if the verifier does not implement appropriate length checks. `_checkSignaturesLength` - * function checks for presence of any padded bytes to the `signature` data. However, there is an - * edge case that `_checkSignaturesLength` function cannot detect. Since, the `signature` field in UserOp is not - * part of the UserOp hash a malicious bundler can manipulate the field 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 signature verifier to check for additional padded bytes to the - * signatures data. + * signature length in the signature data. 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` function checks for the presence of any padded bytes to the `signature` data. + * However, there is an edge case that `_checkSignaturesLength` function cannot detect. + * Since the `signature` field in UserOp is not part of the UserOp hash 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 signature verifier + * (which can be a Safe or any other contract) that supports ERC-1271 and is the owner of the Safe to check for + * additional padded bytes to the signatures data. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise. From bad4fa6dd1a9be4327f840248d7c3cb21242bcd9 Mon Sep 17 00:00:00 2001 From: Akshay Date: Tue, 23 Jul 2024 16:39:03 +0200 Subject: [PATCH 04/12] Fix lint issue --- modules/4337/contracts/Safe4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 249e4d29..c377abef 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -215,7 +215,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @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 + * @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. Safe encodes the Smart Contract * signature length in the signature data. 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 From f7f1214acbadeffa21dcaa6725ac5de126846360 Mon Sep 17 00:00:00 2001 From: Akshay Date: Thu, 8 Aug 2024 14:46:56 +0200 Subject: [PATCH 05/12] Update modules/4337/contracts/Safe4337Module.sol Co-authored-by: Nicholas Rodrigues Lordello --- modules/4337/contracts/Safe4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index c377abef..3eadd2cd 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -219,7 +219,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * fixed in size, the Smart Contract signature can be of arbitrary length. Safe encodes the Smart Contract * signature length in the signature data. 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. + * more gas than needed for user operation validation and reach the `verificationGasLimit`. * `_checkSignaturesLength` function checks for the presence of any padded bytes to the `signature` data. * However, there is an edge case that `_checkSignaturesLength` function cannot detect. * Since the `signature` field in UserOp is not part of the UserOp hash a malicious bundler can manipulate the From cb79059e6b81c91108803c230470736b268857b1 Mon Sep 17 00:00:00 2001 From: Akshay Date: Thu, 8 Aug 2024 14:47:24 +0200 Subject: [PATCH 06/12] Update modules/4337/contracts/Safe4337Module.sol Co-authored-by: Nicholas Rodrigues Lordello --- modules/4337/contracts/Safe4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 3eadd2cd..9540e608 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -224,7 +224,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * However, there is an edge case that `_checkSignaturesLength` function cannot detect. * Since the `signature` field in UserOp is not part of the UserOp hash 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 signature verifier + * make `_checkSignaturesLength` to return true. In such cases, it is the responsibility of the Safe signature validator * (which can be a Safe or any other contract) that supports ERC-1271 and is the owner of the Safe to check for * additional padded bytes to the signatures data. * @param signatures Signatures data. From 85c58224d4c3cd89723cdd743bc130c1aa537434 Mon Sep 17 00:00:00 2001 From: Akshay Date: Thu, 8 Aug 2024 14:47:38 +0200 Subject: [PATCH 07/12] Update modules/4337/contracts/Safe4337Module.sol Co-authored-by: Nicholas Rodrigues Lordello --- modules/4337/contracts/Safe4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 9540e608..9f1a584c 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -226,7 +226,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * 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 * (which can be a Safe or any other contract) that supports ERC-1271 and is the owner of the Safe to check for - * additional padded bytes to the signatures data. + * additional bytes to it smart contract signature data. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise. From 07f6e216efee71071df28c00b1f1cfb576a99f5d Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 26 Aug 2024 15:21:53 +0200 Subject: [PATCH 08/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 9f1a584c..cbb1a4a8 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -222,11 +222,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * more gas than needed for user operation validation and reach the `verificationGasLimit`. * `_checkSignaturesLength` function checks for the presence of any padded bytes to the `signature` data. * However, there is an edge case that `_checkSignaturesLength` function cannot detect. - * Since the `signature` field in UserOp is not part of the UserOp hash 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 - * (which can be a Safe or any other contract) that supports ERC-1271 and is the owner of the Safe to check for - * additional bytes to it smart contract 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 (which can be a Safe or any other contract) that supports + * ERC-1271 and is the owner of the Safe to check for additional bytes to it smart contract signature data. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise. From 3fad06560a3b9b87cabd95c3af1459e2c39d1cd8 Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 26 Aug 2024 15:27:20 +0200 Subject: [PATCH 09/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index cbb1a4a8..7c82270a 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -223,9 +223,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * `_checkSignaturesLength` function checks for the presence of any padded bytes to the `signature` data. * However, there is an edge case that `_checkSignaturesLength` function cannot detect. * 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 (which can be a Safe or any other contract) that supports - * ERC-1271 and is the owner of the Safe to check for additional bytes to it smart contract signature data. + * 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 to it smart contract signature data. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise. From 3dff65b5656955f1b83d40b27641f72c5d864e97 Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 26 Aug 2024 15:41:53 +0200 Subject: [PATCH 10/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 7c82270a..d7a460ff 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -216,12 +216,13 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * 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. Safe encodes the Smart Contract - * signature length in the signature data. 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` function checks for the presence of any padded bytes to the `signature` data. - * However, there is an edge case that `_checkSignaturesLength` function cannot detect. + * 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` function checks for the presence of any padded bytes to the + * `signature` data. 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 From 86b8c96c64bf700c5f3dcab99e3d391342b3fa39 Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 26 Aug 2024 15:50:55 +0200 Subject: [PATCH 11/12] Update comment --- modules/4337/contracts/Safe4337Module.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index d7a460ff..579c6ac0 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -219,8 +219,10 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * 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` function checks for the presence of any padded bytes to the - * `signature` data. However, there is an edge case that `_checkSignaturesLength` function cannot detect. + * `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 From 0220eec21bb7987c0d17bcf61eb3ad66febb5f73 Mon Sep 17 00:00:00 2001 From: Akshay Date: Mon, 26 Aug 2024 15:53:59 +0200 Subject: [PATCH 12/12] Fix comment --- modules/4337/contracts/Safe4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 579c6ac0..2a860db1 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -228,7 +228,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * 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 to it smart contract signature data. + * bytes. * @param signatures Signatures data. * @param threshold Signer threshold for the Safe account. * @return isValid True if length check passes, false otherwise.