search

safe-global / safe-modules

A collection of modules that can be used with the Safe contract
GNU Lesser General Public License v3.0
147 stars 73 forks source link

Signature length check #453

Closed akshay-ap closed 3 months ago

akshay-ap commented 3 months ago

Related to https://github.com/safe-global/safe-smart-account/issues/754

Changes in PR

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
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9792273303

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9792380947

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9793821539

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9793865859

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9794610805

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9794652252

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9794710699

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9794912233

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9795158553

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9795443863

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9795720515

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9795764381

Details

Totals Coverage Status
Change from base Build 9710872352: 0.0%
Covered Lines: 48
Relevant Lines: 48
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9796729829

Details

Totals Coverage Status
Change from base Build 9710872352: -2.5%
Covered Lines: 51
Relevant Lines: 51
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9799315446

Details

Totals Coverage Status
Change from base Build 9710872352: -2.5%
Covered Lines: 51
Relevant Lines: 51
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9906007311

Details

Totals Coverage Status
Change from base Build 9890531912: 0.0%
Covered Lines: 51
Relevant Lines: 51
💛 - Coveralls
akshay-ap commented 3 months ago

I think the tests don't have full coverage - All tests fail with the final check (i.e. whether or not the signature length matches the final expected offset), but AFAICT, none of the fail on the if (!pointsToEnd) check by inserting additional bytes in between the dynamic signatures.

https://github.com/safe-global/safe-modules/pull/453/commits/1ec98133eabbfd10ee60ba4186ab0d52241586a2