Add SignatureValidatorManager contract

[akshay-ap]

Fixes #108

Changes in PR:

• Create SignatureValidatorManager contract
• Introduce new interfaces for ISafeProtocolSignatureValidatorManager, ISafeProtocolSignatureValidator, and ISafeProtocolSignatureValidatorHooks
• Introduce new module types for:
  SignatureValidator = 16
SignatureValidatorHooks = 8
• SignatureValidatorManager supports two validation flows:
  - default flow which depends on account implementation
  - Domain specific flow
  Above two flows are differentiated based on 4 bytes of signature selector value
• Document layout of bytes for signature validation
• Tests for all possible flows
• Update Registry implementation to support new module types (tests included)
• Update test executor so that default validation flow can be supported
• Introduce ISafeAccount interface as this is required in the default validation flow
• Update constants files in solidity and javascript
• Add mock builder file for testing signature validator flow
• Natspec documentation

To be addressed:

• 1. Should the handle(...) in SafeProtocolValdiatorManager be a view function?
  @akshay-ap Preference: No, because validation hooks could be state changing
• 2. Should default validation flow create hash with Safe's domain separator with a typehash? e.g., here
  @akshay-ap Preference: Yes, because if this is not considered, any signed arbitrary signed message would be approved by the Safe account.
• 3.SignatureValidatorManager inherits RegistryManager, meaning it has its own storage for registry address for looking up listed and non-flagged contracts. This can lead to state drift in Manager's registry address and SignatureValidatorManager's registry address.
  How to address this? -
  a. a contract that updates registry address of both SignatureValidatorManager and SafeProtocolManager in the same transaction to the same address value
  b. Both the contract refer to a fixed address that stores the latest registry address (additional gas overhead in every tx, not preferred)
  c. No need to handle this for now. Not that critical

Notes about current implementation:

• An account can set a domain specific validator that matches domain value defined by SignatureValidatorManager . I see no harm in allowing to do so (unless signature validator is malicious which is not possible assuming registry is securely handled).
• SignatureValidatorManager 's handle(...) implementation does not check if first 4 bytes of data are 0x1626ba7e
• SignatureValidatorManager does not check if call is coming from SafeProtocolManager . A loose coupling between them allows other protocol manager implementations to use same SignatureValidatorManager.

[github-actions]

Pull Request Test Coverage Report for Build 6638718888

• 48 of 48 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 6624532325:	0.0%
Covered Lines:	227
Relevant Lines:	227

---

💛 - Coveralls

[mmv08]

SignatureValidatorManager inherits RegistryManager, meaning it has its own storage for registry address for looking up listed and non-flagged contracts. This can lead to state drift in Manager's registry address and

Do I understand correctly that it was done to get around the bytecode size limit?

[akshay-ap]

SignatureValidatorManager inherits RegistryManager, meaning it has its own storage for registry address for looking up listed and non-flagged contracts. This can lead to state drift in Manager's registry address and

Do I understand correctly that it was done to get around the bytecode size limit?

yes. We need a separate SignatureValidatorManager for getting around bytecode size limit and also follow separation of concerns design principle

[mmv08]

follow separation of concerns design principle

Wouldn't we need a separate manager for each module type, then? 😄

[akshay-ap]

follow separation of concerns design principle

Wouldn't we need a separate manager for each module type, then? 😄

yes, ideally we do. this is a step towards it.

But the reason for SafeProtocolManager to act as PluginManager, HooksManager manager is that:

• PluginManager is dependent HooksManager as it needs hooks contract address. If these contracts are separated then it will increase gas cost to fetch required address.

We can possibly have a separate FunctionHandlerManager contract which can work independently.

But then all of them will have a separate storage for registry address adding the overhead to make sure that all refer to appropriate registry.

In the protocol implementation we already have separate contracts for hooks manager, function handler manager. Which are inherited by SafeProtocolManager (which started off as a Plugin manager). So segregation per module type won't be a big major rework

[akshay-ap]

Should we also add chainId?

Actually, chainId can be part of domainSeparator. This is already the case for Safe accounts.
We just need to put it in specs.