Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stricter checks on signature length #778

Closed
wants to merge 8 commits into from

Conversation

akshay-ap
Copy link
Member

@akshay-ap akshay-ap commented Jul 1, 2024

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 akshay-ap self-assigned this Jul 1, 2024
@akshay-ap akshay-ap marked this pull request as draft July 1, 2024 18:05
@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9748983112

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 92.806%

Totals Coverage Status
Change from base Build 9710052157: -0.09%
Covered Lines: 391
Relevant Lines: 403

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9749133521

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 92.806%

Totals Coverage Status
Change from base Build 9710052157: -0.09%
Covered Lines: 391
Relevant Lines: 403

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9749523093

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 92.806%

Totals Coverage Status
Change from base Build 9710052157: -0.09%
Covered Lines: 391
Relevant Lines: 403

💛 - Coveralls

…rt if more than signature count more than threshold
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9757675234

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 92.23%

Files with Coverage Reduction New Missed Lines %
contracts/SafeL2.sol 5 0.0%
Totals Coverage Status
Change from base Build 9710052157: -0.7%
Covered Lines: 386
Relevant Lines: 403

💛 - Coveralls

…signer and revert message depends on order of signature type
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9757956098

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 92.95%

Totals Coverage Status
Change from base Build 9710052157: 0.05%
Covered Lines: 391
Relevant Lines: 403

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9761738591

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 92.95%

Totals Coverage Status
Change from base Build 9710052157: 0.05%
Covered Lines: 391
Relevant Lines: 403

💛 - Coveralls

@nlordell
Copy link
Collaborator

nlordell commented Jul 8, 2024

Closing in favor of safe-global/safe-modules#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.

@nlordell nlordell closed this Jul 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signature verification does not enforce a maximum size on the signature bytes
3 participants