4337 signature flow #64
Conversation
nlordell
requested review from
rmeissner,
akshay-ap and
mmv08
and removed request for
a team
| In order to help protect accounts from any eventual vulnerabilities that may be discovered in the ERC-4337 _entrypoint_, The manager MUST validate `UserOperation` execution with the registry for the particular _entrypoint_ being used. Practically speaking this implies that the ERC-4337 `UserOperation` execution: | ||
| 1. SHOULD be implemented as a fallback handler module with an immutable associated _entrypoint_ contract | ||
| 2. MUST verify that the _entrypoint_ has been explicitely enabled by the account | ||
| 3. MUST verify that the _entrypoint_ has not been flagged |
There was a problem hiding this comment.
do the storage read rules permit this? Or how will it work?
There was a problem hiding this comment.
This is in executeUserOp (i.e. post-UserOperation validation in validateUserOp), so AFAIU there are no storage restrictions here.
What could happen if the Registry has flagged the entrypoint would be that:
- The user's
validateUserOpstill succeeds, and the account pays the prefund - The user operation would revert instead of executing
So, while this is not ideal (the gas is spent), the account is still slightly more protected in case of:
- A vulnerability in the
EntryPointallowing for arbitraryUserOperationexecution - A vulnerability in the 4337 plugin, that would allow for arbitrary
UserOperationexecution.
It isn't fully protected, however, because validateUserOp still transfers missingAccountFunds to msg.sender, but it does prevent the account from executing arbitrary transactions in case of a bug somewhere.
There was a problem hiding this comment.
Ah, understood, I missed the word "execution" in the specs
manager/4337/README.md
Outdated
|
|
||
| ### Staking | ||
|
|
||
| The account SHOULD implement a staked _factory_ in order to enable `UserOperations` with `initCode`. ERC-4337 requires a staked _factory_ for `UserOperation` validation that reads associated account storage when `initCode` is specified. Since this specification permits Manager implementations that read account associated storage during `UserOperation` validation, the account _factory_ needs to be staked in order for bundlers to accept `UserOperations` with `initCode`. |
There was a problem hiding this comment.
Could we add more context what storage is accessed that make this necessary? I.e. for validateUserOp it is outlined that the registry check is problematic. What would be problematic here?
There was a problem hiding this comment.
Added some more context in the section above about the registry storage access which is considered problematic.
Regarding factory staking, what storage in particular do you think we need to explicitly mention? My argument in this sentence is basically:
Account factories SHOULD be staked because the spec allows the Manager and 4337 support to access storage associated with the account which requires a staked factory
This is basically needed because there are additional restrictions to validateUserOp storage access when paired with initCode.
| One of the main value propositions of the Safe{Core} Protocol is account security by verifying modules in the Registry. However, because of storage access restrictions imposed by the ERC-4337 spec (see Compatibility section above), this is not possible when validating user operations. However, this restriction does not apply to `UserOperation` execution. | ||
|
|
||
| In order to help protect accounts from any eventual vulnerabilities that may be discovered in the ERC-4337 _entrypoint_, The manager MUST validate `UserOperation` execution with the registry for the particular _entrypoint_ being used. Practically speaking this implies that the ERC-4337 `UserOperation` execution: | ||
| 1. SHOULD be implemented as a fallback handler module with an immutable associated _entrypoint_ contract |
There was a problem hiding this comment.
Would a sequence diagram help here?
There was a problem hiding this comment.
I referenced the sequence diagram from the Execution Flow section, as it describes this.
|
@rmeissner - I’m merging this to not be blocked for the sprint. I also captured some additional open questions regarding signature validation use cases which I believe are important to be answered to better refine a signature flow in the spec: #63 |
Fixes #59
This PR documents the research around ERC-4337 flows, in particular including signature validation flow.
Most of the rationale for decisions captured in the specification itself (well, should be in principle 😅). Some of the working assumptions when implementing this specification are: