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.

Sign up for GitHub

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

Jump to bottom

Signature length check #453

Merged
merged 52 commits into from
Jul 12, 2024
Merged

Signature length check #453

merged 52 commits into from
Jul 12, 2024

Conversation

akshay-ap
Copy link
Member

@akshay-ap akshay-ap commented Jul 1, 2024 •
edited
Loading

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 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.

  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

@akshay-ap akshay-ap self-assigned this Jul 1, 2024
@akshay-ap akshay-ap requested a review from a team as a code owner July 1, 2024 13:01
@akshay-ap akshay-ap requested review from nlordell, mmv08 and remedcu and removed request for a team July 1, 2024 13:01
@akshay-ap akshay-ap marked this pull request as draft July 1, 2024 13:02
@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9792273303

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9792380947

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9793821539

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9793865859

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9794610805

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9794652252

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9794710699

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9794912233

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@akshay-ap akshay-ap changed the title WIP: Signature length check Signature length check Jul 4, 2024
@akshay-ap akshay-ap force-pushed the fix-754-signature-length-check branch from 1ac5075 to c72d37f Compare July 4, 2024 13:23
@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9795158553

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9795443863

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9795720515

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@akshay-ap akshay-ap force-pushed the fix-754-signature-length-check branch from 5d87857 to 2754975 Compare July 4, 2024 14:06
@coveralls
Copy link

coveralls commented Jul 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9795764381

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls

@akshay-ap akshay-ap requested a review from nlordell July 10, 2024 11:28
@akshay-ap akshay-ap force-pushed the fix-754-signature-length-check branch from f3807d2 to c54c89d Compare July 11, 2024 10:57
akshay-ap and others added 7 commits July 11, 2024 14:05
@akshay-ap @nlordell
Update modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts
db5fcba 
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
@akshay-ap @nlordell
Update modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts
ab66109 
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
@akshay-ap @nlordell
Update modules/4337/test/erc4337/ReferenceEntryPoint.spec.ts
31798cb 
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
@akshay-ap
[#754] Update test
42b216e
@akshay-ap
[#754] Update tests
0306c39
@nlordell
Refactor Length Check Implementation
16e7b14 
Here, we refactor the length check to not use assembly (as we didn't
have a compelling argument to do so). Additionally, we added an escape
hatch on the length check validation in order to allow empty signature
(a reasonable "default" value) to not revert on `validateUserOp` for
better developer experience with 4337, which required an additional test
for full coverage.
@nlordell
Cleanup Post-Review
405a5ba
@akshay-ap akshay-ap force-pushed the fix-754-signature-length-check branch from ab38827 to 09c4000 Compare July 12, 2024 09:56
@akshay-ap akshay-ap merged commit c3a4d06 into main Jul 12, 2024
13 checks passed
@akshay-ap akshay-ap deleted the fix-754-signature-length-check branch July 12, 2024 10:06
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nlordell nlordell nlordell approved these changes

@remedcu remedcu remedcu approved these changes

@mmv08 mmv08 Awaiting requested review from mmv08

Assignees

@akshay-ap akshay-ap

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@akshay-ap @coveralls @nlordell @mmv08 @remedcu