Add stricter checks on signature length

akshay-ap commented 2 months ago

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

coveralls commented 2 months ago

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
Change from base Build 9710052157:	-0.09%
Covered Lines:	391
Relevant Lines:	403

💛 - Coveralls

coveralls commented 2 months ago

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
Change from base Build 9710052157:	-0.09%
Covered Lines:	391
Relevant Lines:	403

💛 - Coveralls

coveralls commented 2 months ago

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
Change from base Build 9710052157:	-0.09%
Covered Lines:	391
Relevant Lines:	403

💛 - Coveralls

coveralls commented 2 months ago

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%
<!--	Total:	5		-->

Totals
Change from base Build 9710052157:	-0.7%
Covered Lines:	386
Relevant Lines:	403

💛 - Coveralls

coveralls commented 2 months ago

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
Change from base Build 9710052157:	0.05%
Covered Lines:	391
Relevant Lines:	403

💛 - Coveralls

coveralls commented 2 months ago

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
Change from base Build 9710052157:	0.05%
Covered Lines:	391
Relevant Lines:	403

💛 - Coveralls

nlordell commented 2 months ago

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.

safe-global / safe-smart-account