safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.88k stars 927 forks source link

Signature verification does not enforce a maximum size on the signature bytes #754

Closed nlordell closed 2 weeks ago

nlordell commented 6 months ago

Description

The function checkNSignatures does 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 the signatures by padding them with additional data while still remaining valid and, since the signatures bytes get copied from calldata into memory, increase the total gas consumption of the checkNSignatures function. 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 the signatures bytes to include extra 0s causing the account to pay more in fees than it would have with an optimally encoded signatures bytes.

A workaround for existing Safes is to set a strict verificationGasLimit for 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:

nlordell commented 2 weeks ago

Closing as per comment in https://github.com/safe-global/safe-smart-account/pull/778#issuecomment-2213782916

Closing in favor of https://github.com/safe-global/safe-modules/pull/453

The context of the decision is:

  • This only affects 4337
  • We do not want to remove the ability to add additional contextual information for guards using the signature bytes
  • We do not want to break backwards compatibility with the SDK/Wallet.