Added Sample 4337 Module

[nlordell]

⚠️ DO NO MERGE

Note to reviewers: Since this is intended as a PoC to evaluate feasibility and not necessarily merged here, do not look over the changes in detail, just the overall structure and findings. Additional notes and findings are documented in #125 (comment)

Fixes #125

This PR adds a sample ERC-4337 integration as a plugin/function handler as described in the spec (see 5afe/safe-core-protocol-specs#64).

• Adds a new SafeProtocol4337Module.sol module implementation acting both as a plugin and function handler.
• Adds a test that verifies that an account setup with this module can be successfully called from the reference ERC-4337 EntryPoint implementation
• An integration test using the reference contract and bundler source. This can be run with:

  # using the docker-compose.yaml file from 5afe/playground-4337
  docker compose up -d
  
  npx hardhat deploy --network localhost
  npx hardhat run scripts/user_operation.ts --network localhost

There was one notable change needed in order to get the Protocol contracts working with this plugin:

1. Add special escape hatch is needed for paying prefunds that skip the plugin check with the registry

[github-actions]

Pull Request Test Coverage Report for Build 6847999103

• 28 of 31 (90.32%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-2.8%) to 97.183%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/modules/erc4337/SafeProtocol4337Module.sol	26	29	89.66%

Totals
Change from base Build 6800133305:	-2.8%
Covered Lines:	255
Relevant Lines:	258

---

💛 - Coveralls

[mmv08]

whats 0x3a871cdd?

[nlordell]

It is the selector of validateUserOp. This is veeeeery hacky (I talk about it in my findings I documented in #125 (comment)).

This is needed because doing the prefund transfer from a plugin requires a lookup in the registry which is not allowed for two reasons:

1. 4337 bundlers do not accept such user operations
2. The Safe{Core} Protocol spec forbids it 😛

I left a note in the finding to clean this up

[mmv08]

This value variable is unused; does it make sense to omit the check and remove the argument?

[nlordell]

This is part of the interface of the ISafeProtocolFunctionHandler. I think we could change it to not have a value, but that would be strange to me:

1. I think fallbackhandlers should be able to emulate non-payable functions (so revert on value != 0
2. payable fallback handlers should be able to read the amount of Ether that was sent with the transaction to the account.

I would also argue this is a bit out of the scope of this PR.

[mmv08]

I noticed that there's an inconsistency between file names

Also, we have a separate repo for demos - https://github.com/5afe/safe-core-protocol-demo. Would it make sense to place it there?

[mmv08]

Would it make sense to use an actual safe account here?

[nlordell]

Probably, but it made the signing more complicated 😅. You could argue that the PoC was about ensuring that some account implementation paired with the Safe{Core} Protocol reference implementation could work with ERC-4337, so I don’t think its strictly necessary.

[nlordell]

@mmv08 - fixed the file naming inconsistencies.

[nlordell]

Created an issue to track porting this code to the https://github.com/5afe/safe-core-protocol-demo repository.

I’m going to close this PR for now as:

• We will focus on ERC-4337 with the current Safe contracts, and the Safe{Core} Protocol spec is likely to evolve in that time
• The expected outcome of the task was to document the findings from implementing this PoC which has been done in [Protocol Manager] Evaluate 4337 Compatible Validator Implementation #125 (comment)
• The transferPrefund escape hatch should probably be further discussed and documented before it is accepted