Allow direct plugin calls with validation & permission hooks #90
Conversation
Zer0dot
force-pushed
the
zer0dot/direct-plugin-calls
branch
from
b5f4430 to
1e8344f
Compare
adamegyed
force-pushed
the
adam/sample-allowlist-hook
branch
from
78ae1eb to
5c10b0d
Compare
Zer0dot
force-pushed
the
zer0dot/direct-plugin-calls
branch
from
66e41e6 to
b11b7b4
Compare
|
Migrated installation to pluginManager2's |
|
For context, installing a validation with the function reference of |
Zer0dot
changed the title
| address plugin = validationConfig.plugin(); | ||
| IPlugin(plugin).onInstall(installData); | ||
| if (validationConfig.functionId() != _SELF_PERMIT_VALIDATION_FUNCTIONID) { | ||
| // Only allow global validations and signature validations if they're not direct-call validations. |
There was a problem hiding this comment.
Is this necessary to prevent an access control issue? Or is it just an anti-footgun measure?
There was a problem hiding this comment.
It's just an anti-footgun, though I've not evaluated the consequences of having global validation for a direct call-- signature validation either.
There was a problem hiding this comment.
Ok sounds good. It might help to support global/sig validation in some very niche cases where the direct call validation is used to create an "owner" EOA, but that seems small enough to ignore for now.
There was a problem hiding this comment.
Gotcha, happy to revisit this down the line if we see the feature's important!
…ecking storage to handle self-calls
| override | ||
| returns (bytes memory) | ||
| { | ||
| require(sender == address(this), "mock direct call pre permission hook failed"); |
There was a problem hiding this comment.
Ugh this is an annoying linter warning because it's in a test. Maybe add an exclusion for the rule reason-string in tests?
There was a problem hiding this comment.
Addressed in 0f04d85!
Doubled line length
Addressed in d263656! |
| @@ -136,7 +135,7 @@ contract UpgradeableModularAccount is | |||
| revert UnrecognizedFunction(msg.sig); | |||
| } | |||
|
|
|||
| _checkPermittedCallerIfNotFromEP(); | |||
| _checkPermittedCallerAndAssociatedHooks(); | |||
There was a problem hiding this comment.
What if a signer of the account call an execFunction? Would it fail here?
There was a problem hiding this comment.
I.e. if a signer, as set in the storage of SingleSignerValidation, tries to call the account? In that case, this check would fail, the signer would need to use executeWithAuthorization to trigger runtime validation.
Alternatively, if an account is only used from an EOA, you could add an EOA itself as an allowed caller by installing the EOA address + the direct call magic value as a validation. But, this wouldn't be usable via user op validation.
There was a problem hiding this comment.
Aren't we executing the pre-execution hooks currently twice in case a permitted caller is calling through fallback? Within _checkPermittedCallerAndAssociatedHooks() in L664 we do the pre-execution hooks of the function selector, but again in the following fallback() flow in L143.
Also, are we not forgetting to do the post-permission hooks in the fallback flow? 👀
There was a problem hiding this comment.
@0xrubes you are correct... we'll get a fix PR up soon.
Motivation
In order to support workflows where an action should be allowed only if a plugin allows it (e.g. cold storage: block all transfers unless the plugin authorizes it), we need a way for plugins to directly call into account functions.
However, having the flexibility to still attach validation (and permission) hooks would allow for account-decided limited scopes for each plugin allowed to do this, in contrast with the previously removed permitted call workflow.
Solution
Add a field for allowed direct calls to account storage. This is a mapping from caller to selector to a new struct which contains a boolean and an array of validation hooks.
In
_checkPermittedCallerIfNotFromEPcalled fromwrapNativeFunction, we can then check if the call is allowed to occur and execute its respective validation hooks.TODO/TBD
Depends on user-supplied install configs for plugins to determine how this storage is set on installation (manifest, encoded in user-supplied data?)Do we want to redesign the system to not need additional storage? Can we do so without complicating plugin developers' mental models of the system?Update: Modified to use
installValidation()which uses user-provided configurations.