Signature verification does not enforce a maximum size on the signature bytes #754
Assignees
Labels
Comments
akshay-ap
added a commit
that referenced
this issue
Jul 1, 2024
akshay-ap
added a commit
that referenced
this issue
Jul 1, 2024
akshay-ap
added a commit
that referenced
this issue
Jul 1, 2024
akshay-ap
added a commit
that referenced
this issue
Jul 2, 2024
…rt if more than signature count more than threshold
akshay-ap
added a commit
that referenced
this issue
Jul 2, 2024
…signer and revert message depends on order of signature type
akshay-ap
added a commit
that referenced
this issue
Jul 2, 2024
akshay-ap
added a commit
to safe-global/safe-modules
that referenced
this issue
Jul 12, 2024
Related to safe-global/safe-smart-account#754 ## Changes in PR - Add a test case that verifies that UserOp is reverted if signature contains additional bytes data - Add `_checkSignatureLength` function in `Safe4437Module` contract . This function retrieves threshold from Safe and iterates over signature data. - Add a test contract which is used in testing the internal function: - Add tests - Summarise `_checkSignatureLength` in FV specs. The prover was reporting error without giving call traces. - Unrelated to issue: [Fix script](https://github.com/safe-global/safe-modules/pull/453/files#diff-d6fef5446f8c9e62fd4180360854e7e830def1aa48a27dda677a99a74fcd6a44R24) in `package.json` ## Open for discussion Should `_checkSignatureLength` also do more stricter checks that are already there in `checkNSignature` function of `Safe`? If `Safe4337Module` does not perform below mentioned checks then it will be implicitly assumed to be done in `Safe` contract and this creates a requirement that all future `Safe` versions to have these checks if used with this `Safe4337Module`. 1. `if (signatures.length < offset)` this check is currently done twice. - In Safe4337Module: https://github.com/safe-global/safe-modules/pull/453/files#diff-8ec9433e2f98b74922f038206ac421a769cf3996997ad55ed210299abddb7908R223 - In Safe: https://github.com/safe-global/safe-smart-account/blob/8f80a8372d193be121dcdb52e869a258824e5c0f/contracts/Safe.sol#L279 2. Other potential check that can be repeated: https://github.com/safe-global/safe-smart-account/blob/8f80a8372d193be121dcdb52e869a258824e5c0f/contracts/Safe.sol#L233 ## Code size change ### This PR ``` Safe4337Module 8910 bytes (limit is 24576) ``` ### Main branch ``` Safe4337Module 8373 bytes (limit is 24576) ``` ## Impact on gas usage ### This PR ``` Gas Metering Safe Deployment + Enabling 4337 Module Used 420045 gas (Account or Paymaster) for >Safe with 4337 Module Deployment< Used 416973 gas (Transaction) for >Safe with 4337 Module Deployment< ✔ Safe with 4337 Module Deployment Safe Deployment + Enabling 4337 Module + Native Transfers Used 451408 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer< Used 449392 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer< ✔ Safe with 4337 Module Deployment + Native Transfer Used 193399 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer< Used 183844 gas (Transaction) for >Safe with 4337 Module Native Transfer< ✔ Safe with 4337 Module Native Transfer Safe Deployment + Enabling 4337 Module + Token Operations Used 436781 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer< Used 427911 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer< ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer Used 471043 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer< Used 469710 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer< ✔ Safe with 4337 Module Deployment + ERC721 Token Minting Token Operations Only Used 177996 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer< Used 162338 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer< ✔ Safe with 4337 Module ERC20 Token Transfer Used 213064 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Token Minting< Used 204125 gas (Transaction) for >Safe with 4337 Module ERC721 Token Minting< ✔ Safe with 4337 Module ERC721 Token Minting ``` ### Main branch ``` Gas Metering Safe Deployment + Enabling 4337 Module Used 419096 gas (Account or Paymaster) for >Safe with 4337 Module Deployment< Used 414864 gas (Transaction) for >Safe with 4337 Module Deployment< ✔ Safe with 4337 Module Deployment Safe Deployment + Enabling 4337 Module + Native Transfers Used 450537 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + Native Transfer< Used 447308 gas (Transaction) for >Safe with 4337 Module Deployment + Native Transfer< ✔ Safe with 4337 Module Deployment + Native Transfer Used 192424 gas (Account or Paymaster) for >Safe with 4337 Module Native Transfer< Used 182182 gas (Transaction) for >Safe with 4337 Module Native Transfer< ✔ Safe with 4337 Module Native Transfer Safe Deployment + Enabling 4337 Module + Token Operations Used 435685 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC20 Transfer< Used 425687 gas (Transaction) for >Safe with 4337 Module Deployment + ERC20 Transfer< ✔ Safe with 4337 Module Deployment + ERC20 Token Transfer Used 470086 gas (Account or Paymaster) for >Safe with 4337 Module Deployment + ERC721 Transfer< Used 467491 gas (Transaction) for >Safe with 4337 Module Deployment + ERC721 Transfer< ✔ Safe with 4337 Module Deployment + ERC721 Token Minting Token Operations Only Used 176846 gas (Account or Paymaster) for >Safe with 4337 Module ERC20 Transfer< Used 160584 gas (Transaction) for >Safe with 4337 Module ERC20 Transfer< ✔ Safe with 4337 Module ERC20 Token Transfer Used 212013 gas (Account or Paymaster) for >Safe with 4337 Module ERC721 Token Minting< Used 202376 gas (Transaction) for >Safe with 4337 Module ERC721 Token Minting< ✔ Safe with 4337 Module ERC721 Token Minting ``` --------- Co-authored-by: Nicholas Rodrigues Lordello <nick@safe.global> Co-authored-by: Shebin John <admin@remedcu.com>
|
Closing as per comment in #778 (comment)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
The function
checkNSignaturesdoes not enforce a size limit on the signature bytes. This means that they can be padded with arbitrary data. This can be used by manipulating thesignaturesby padding them with additional data while still remaining valid and, since thesignaturesbytes get copied from calldata into memory, increase the total gas consumption of thecheckNSignaturesfunction. This is an issue when the Safe is combined with the ERC-4337 module, where gas costs for the ERC-4337 user operation are paid by the account. A malicious relayer can grief the account by padding thesignaturesbytes to include extra0s causing the account to pay more in fees than it would have with an optimally encodedsignaturesbytes.A workaround for existing Safes is to set a strict
verificationGasLimitfor ERC-4337 user operations, which would set a strict upper bound on how much gas can be paid during signature verification and limit the potential additional fees.Shoutout to @adamegyed for submitting this issue as part of the bug bounty program.
Solution
The solution is to require stricter encoding of Safe signatures bytes and disallow padding. This can be implemented by:
svalue to be a specific offset (equal to the length of the fixed signature part65*nplus the length of the previous contract signatures that have been verified)The text was updated successfully, but these errors were encountered: