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.

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

[Protocol Manager] Evaluate Protocol support for ERC4337 #60

Closed
akshay-ap opened this issue Aug 14, 2023 · 1 comment
Closed

[Protocol Manager] Evaluate Protocol support for ERC4337 #60

akshay-ap opened this issue Aug 14, 2023 · 1 comment
Assignees

Comments

@akshay-ap
Copy link
Contributor

akshay-ap commented Aug 14, 2023

Create an example to evaluate if Safe{Core} Protocol can be used with ERC4337

Expected outcome:

  • Get an understanding if there is a blocker for 4337 support
@akshay-ap akshay-ap changed the title Protocol support for 4337 Protocol support for ERC4337 Aug 14, 2023
@akshay-ap akshay-ap self-assigned this Aug 21, 2023
@rmeissner rmeissner assigned mmv08 and unassigned akshay-ap Aug 28, 2023
@rmeissner rmeissner changed the title Protocol support for ERC4337 Evaluate Protocol support for ERC4337 Aug 28, 2023
@rmeissner rmeissner changed the title Evaluate Protocol support for ERC4337 [Protocol Manager] Evaluate Protocol support for ERC4337 Sep 29, 2023
@mmv08
Copy link
Member

mmv08 commented Oct 13, 2023

After prototyping an erc4337 support for the protocol in the form of a plugin and a function handler, it doesn't seem possible to support erc4337 through the protocol in the current form because of forbidden storage reads:

  • The manager has to do a SLOAD to get the registry address
  • The registry has to do a SLOAD to get the plugin info
  • The manager does a bunch of SLOADs to check if the plugin is enabled for an account, which doesn't fall into ERC4337's storage reads that are associated with an account, defined as:

Storage associated with an address
We define storage slots as “associated with an address” as all the slots that are uniquely related on this address, and cannot be related with any other address. In solidity, this includes all storage of the contract itself, and any storage of other contracts that use this contract address as a mapping key.
An address A is associated with:

  1. Slots of contract A address itself.
  2. Slot A on any other address.
  3. Slots of type keccak256(A || X) + n on any other address. (to cover mapping(address => value), which is usually used for balance in ERC-20 tokens). n is an offset value up to 128, to allow accessing fields in the format mapping(address => struct)
    The "X" can be any value. It is the "slot id" of a mapping, and in multi-dimensional mapping, it is the result of the previos mapping.

Particularly, this mapping is not compatible:

    /**
     * @notice Mapping of a mapping what stores information about plugins that are enabled per account.
     *         address (Account address) => address (module address) => EnabledPluginInfo
     */
    mapping(address => mapping(address => PluginAccessInfo)) public enabledPlugins;

When accessing plugin info for an account, the slot will be computed as:

 hash(plugin . hash(account_address . slot id)

One way to fix this would be to reverse the mapping to go from a plugin to an account, but this way, we'd lose getters for plugins enabled for an account. Also, this wouldn't eliminate registry storage reads.

Also, the mapping for function handlers doesn't seem to be compatible and would require adjustments:

    mapping(address => mapping(bytes4 => address)) public functionHandlers;

Potential solution:
We should research adding ERC4337 support directly to the manager itself, but IMO it may be challenging to tie this to the planned tokenomics, which were originally planned to be outsourced to the registry contract.

Alternatively, we could look into adjusting the mentioned mappings and somehow removing the registry storage reads

The code is available here: https://github.com/5afe/safe-core-protocol-demo/tree/feature/erc4337-plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants