safe-smart-account icon indicating copy to clipboard operation
safe-smart-account copied to clipboard

Add stricter checks on signature length

Open akshay-ap opened this issue 7 months ago • 6 comments

Fixes #754

This PR enforces stricter checks on the signature length during verification. The checkNSignatures now checks that after completing the signature verification, the offset points to the end of the signature data. This ensures that no additional bytes are present than required for the verification to work.

Without this change, currently there is no restriction on length of signature submitted for verification due to which an attacker can possibly append additional bytes when using Safe + 4337 module and hit verificationGasLimit. This can cause Safe to pay more for verification than needed.

Note: A transaction will fail with GS028 or GS021 based on the how signatures are submitted when signatures contain additional approvals than required threshold. Wallet and other applications have to consider this during error handling if relevant.

Changes in PR

  • Safe contract checks if signature data does not contain additional bytes data than required
  • Add test cases:
    • Signature verification reverts if additional bytes are added to signatures than required
    • Signature verification reverts signature count exceeds required threshold
  • Fix tests
    • In Ethers.js defaultAbiCoder pads the bytes data to nearest word which appends additional zeros to signature data. This lead to failing test
    • Submit only required number of signatures in Compatibility Fallback Handler tests and benchmarking
    • New error GS028

Codesize change

This PR

Safe 21877 bytes (limit is 24576)
SafeL2 22657 bytes (limit is 24576)

Main branch

Safe 21832 bytes (limit is 24576)
SafeL2 22612 bytes (limit is 24576)

Gas implications

TODO

Problem

If the signatures payload contains more approvals from owners than required threshold, the signature validation will fail. This is a breaking change for wallet

akshay-ap avatar Jul 01 '24 18:07 akshay-ap