feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] #62

adamegyed · 2024-05-21T18:12:15Z

Motivation

As described in erc6900/resources#4, supporting multiple validation functions has been a long-standing goal of ERC-6900.

Solution

This PR implements switching between different validation functions using a field in the user op signature for user op validation, and a calldata field for runtime validation.

It also lays the groundwork for, but does not implement, associating pre-validation hooks with validators, and per-hook/per-validation auth data (erc6900/resources#32).

To avoid needing a "default" or "primary" validation per selector, like in #52, this PR also changes the default auth flow through accounts. Previously, there were three validation flows through the account:

User op validation (+ any pre-validation hooks): runs through validateUserOp called by EntryPoint
Runtime validation (+ any pre-validation hooks): runs through fallback & native functions
Authorized caller check: runs through executeFromPlugin

This PR keeps the user op validation flow, and reorganizes the auth flows of the latter two:

Runtime validation (+ any pre-validation hooks): runs through executeWithAuthrization
Authorized caller check: runs through fallback & native functions

Note that the isPublic property of an execution function, introduced earlier in the stack at #61, is still applied when calling functions through the fallback and native function paths.

By making this organizational change, this solution does not require a "default" or "primary" validation per selector. Whenever a validation function needs to run, it is specified by the caller and checked by the account to be an installed validator for the given function.

Future work

The validation encoded in the user op signature (currently 21 bytes) would easily fit within the key portion of the user op nonce (24 bytes). However, due to potential future changes to how we identify validation functions likely coming with composable validation (erc6900/resources#36) and user-supplied install configs (erc6900/resources#9), I've avoided making that optimization for the time being. We should revisit the possibility of optimizing this once those are addressed.

Additionally, this PR does not make changes to pre-validation hook associations, which I believe multi-validation makes necessary. See #52 for more info on this.

src/account/AccountLoupe.sol

src/interfaces/IAccountLoupe.sol

src/account/UpgradeableModularAccount.sol

howydev · 2024-06-05T23:33:53Z

taking a look!

thoughts on adding something to the loupe to expose isPublic for a selector?

howydev · 2024-06-06T16:20:12Z

Overall this looks good!

Some thoughts/q's:

What/how is execute executeBatch used now?
I think this shifts some complexity into the SDK for user ops, but don’t see this being a huge lift from existing flows since validation plugin devs already should be specifying a format for {dummy signature format, signing format} somewhere
It adds some complexity into runtime calls, but that’s a path thats not very widely used today, so not a big downside too
IMO we should spend some time on what a validation grouping mechanism for efficiency might look like, since validations might be replicated across execute, executeBatch and executeWithAuthorization? (callout: I briefly looked at the default validation PR, saw it might be handled there)
What’s the difference between a plugin installing a new exec function vs using executeWithAuthorization beyond isPublic functions? Only 2 reasons I can think of rn:

a. security related - calls from plugins are less dangerous, e.g. account doing token.transfer vs plugin doing token.transfer, and installing a validation/prevalidation function stack on a new execution function is also less dangerous
b. standardization related - dapp wants a standardized format to do calls across 6900 and non6900 accounts and the common denominator would be to utilize the fallback routing mechanism (i.e. IDapp(account).dappAction())

Curious if there are more reasons here

jaypaik

Lgtm.

This got me thinking: are we comfortable with the idea of providing a single validation function for an executeBatch? I know we had toyed around with the idea of being able to specify a different one for each in the past.

src/account/AccountLoupe.sol

jaypaik · 2024-06-06T23:12:53Z

src/account/PluginManagerInternals.sol

-        if (_selectorData.validation.notEmpty()) {
+        // Fail on duplicate definitions - otherwise dependencies could shadow non-depdency
+        // validation functions, leading to partial uninstalls.
+        if (!_selectorData.validations.add(toSetValue(validationFunction))) {
            revert ValidationFunctionAlreadySet(selector, validationFunction);


We should come back this naming problem (validation vs. validation function vs. ?) later. Same with isPublic.

jaypaik · 2024-06-06T23:26:53Z

src/account/PluginManagerInternals.sol

-        if (_selectorData.validation.notEmpty()) {
+        // Fail on duplicate definitions - otherwise dependencies could shadow non-depdency
+        // validation functions, leading to partial uninstalls.
+        if (!_selectorData.validations.add(toSetValue(validationFunction))) {


I imagine we'd want to revisit this if we end up allowing multiple installations of the same plugin with different permissions? Still maybe makes sense to check for duplicates but I guess the key might include the permissions.

cc @howydev

Validation routing is done off-chain so definitely worth exploring moving execution selector routing off-chain too!

Re: session keys, i think the decision is between plugin + 1-key-per-plugin (or multiple keys that are managed by a dapp) v.s. 1 plugin + n keys per plugin?

I do prefer the security model of 1 key per plugin since we can use the account's permissions framework directly. That's preferable for 2 reasons - we don't need to trust the plugin to manage individual key permissions, and we dedupe permissions checking (in the other model, account allocates a global limit to the plugin, then the plugin allocates per-key limits). This plugin could be as lightweight as a signature validator if the native permissions framework is sufficiently granular/flexible

But in the 1-key-per-plugin model, we'll potentially have to deploy a new contract to add a new key (probably a proxy from a ProxyFactory, which is the safe/zodiac modules model). Unless the deployed plugin is ultra lightweight it should be more costly than the other approach

Yeah we definitely need some changes to support that workflow, but I wanted to keep each change scoped down to just what is needed per PR.

Also, I think having 1 key per plugin is too inefficient - we can treat the function id as a per-account, per-key item to perform that switching, like in the validator experiments.

"Key" is also the wrong term here, it's leaking the underlying validation logic into a higher layer of abstraction. They're really independent "validation functions" or "validators".

Also, I think having 1 key per plugin is too inefficient - we can treat the function id as a per-account, per-key item to perform that switching, like in the validator experiments.

Hmm I might be missing something, but here's an example I was thinking about - in the case of adding a new session key to the existing MA session key plugin with n permissions, the minimal state change required will be:

n x nonzero to nonzero SSTOREs via increment the ERC20 + native token spend limits from the account to the plugin

n x zero to nonzero SSTOREs via incrementing the ERC20 + native token spend limits of the plugin to the session key

Whereas deploying a new validator would be just n x zero to nonzero SSTOREs via incrementing the ERC20 + native token spend limits of the account to the new validator

I know in the old v0.7 model, we basically needed two layers of permissions - account <> plugin, and in the case of a key-based validation plugin, then permissions for each key.

If we switch to composable validation (aka, allow validation plugins to be installed multiple times), then we go back to just needing 1 set of permissions per "validation", which is a combination of both the address of the validator plugin and the ID of the key within it.

I think the model of doing proxies on plugins per account is wrong - the cost of any contract creation is already too. high (32k), not to mention the added forwarding cost of being a proxy.

src/account/PluginManagerInternals.sol

src/account/UpgradeableModularAccount.sol

jaypaik · 2024-06-07T00:04:26Z

src/account/UpgradeableModularAccount.sol

+        if (_storage.selectorData[msg.sig].denyExecutionCount > 0) {
+            revert AlwaysDenyRule();


Kinda want to revisit naming of AlwaysDenyRule and denyExecutionCount too.

Or we can get rid of the magic values in #63, if that is finalized

jaypaik · 2024-06-07T00:15:55Z

src/account/UpgradeableModularAccount.sol

@@ -262,6 +262,41 @@ contract UpgradeableModularAccount is
        return returnData;
    }

+    /// @inheritdoc IPluginExecutor
+    function executeWithAuthorization(bytes calldata data, bytes calldata authorization)


Again re: naming (we can punt) - this is still (runtime) validation, so a bit surprising to introduce new vocabulary authorization.

Yeah happy to revisit. For some background on why I started with this, I saw this field as analogous to the "signature" data in user ops, but we've overloaded that term in too many places already (including 1271), and it's just the authorization for one particular action.

huaweigu

This is great! Would this also unlock the support for runtime multisig?

huaweigu · 2024-06-07T17:04:26Z

src/account/UpgradeableModularAccount.sol

+    {
+        bytes4 execSelector = bytes4(data[0:4]);
+
+        FunctionReference runtimeValidationFunction = FunctionReference.wrap(bytes21(authorization[0:21]));


Nit: do we allow invalid runtimeValidationFunction? If not, should we consider validating here before reading into storage?

Could you please explain your question a bit more? The workflow here is currently:

The requested validation function is read from the data in authorization.

The account checks to make sure that this validation function is actually installed.

The account checks that the validation function applies to the selector this call is trying to run.

ah I just meant function references like bytes21(0) might not be valid, so we can probably fail fast here

Ah I see, since that step will fail anyways on the storage check later I don't know if it's worth failing early - it reduces gas in the failure case but increases gas in the success case, and generally I think optimizing for the success case is better.

huaweigu · 2024-06-07T17:05:01Z

src/account/UpgradeableModularAccount.sol

+        payable
+        returns (bytes memory)
+    {
+        bytes4 execSelector = bytes4(data[0:4]);


ditto on validity check

huaweigu · 2024-06-07T17:20:26Z

src/interfaces/IPluginExecutor.sol

+    /// @param data The calldata to send to the account.
+    /// @param authorization The authorization data to use for the call. The first 21 bytes specifies which runtime
+    /// validation to use, and the rest is sent as a parameter to runtime validation.
+    function executeWithAuthorization(bytes calldata data, bytes calldata authorization)


Do we also need to update https://github.com/erc6900/reference-implementation/blob/6d48a68021bff6f3a50a508873595af9fb1939a6/standard/ERCs/erc-6900.md in this PR or later on
(update: okay, I see it's moved to different interface in #65)

adamegyed · 2024-06-10T15:14:10Z

@huaweigu sorry I missed the earlier comment - yes this does make it possible to have a runtime path multisig. The signatures (or any other data related to authorization) can be put in the authorization field of the runtime validation function.

adamegyed · 2024-06-10T18:39:24Z

@howydev just saw this

thoughts on adding something to the loupe to expose isPublic for a selector?

Yeah that definitely makes sense! we should revisit loupe functions overall at the end of the major spec changes, this and probably more might need to change.

adamegyed changed the title ~~feat: [v0.8-develop, experimental] multi validation in user op signature~~ feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] May 21, 2024

adamegyed mentioned this pull request May 21, 2024

feat: [v0.8-develop, experimental]: sig validation in interface [2/N] #59

Merged

adamegyed marked this pull request as ready for review May 21, 2024 18:46

adamegyed requested a review from a team May 21, 2024 23:30

jaypaik reviewed May 29, 2024

View reviewed changes

src/account/AccountLoupe.sol Outdated Show resolved Hide resolved

src/interfaces/IAccountLoupe.sol Show resolved Hide resolved

adamegyed force-pushed the adam/simplify-rt-allowed-calls branch from c83ac72 to e871784 Compare May 29, 2024 21:01

adamegyed force-pushed the adam/multi-validation-uo-sig branch 2 times, most recently from 41b30e7 to f347833 Compare May 29, 2024 21:28

adamegyed force-pushed the adam/simplify-rt-allowed-calls branch 2 times, most recently from 77ce9ff to f48ccd7 Compare May 31, 2024 18:48

Base automatically changed from adam/simplify-rt-allowed-calls to v0.8-develop May 31, 2024 18:49

adamegyed added 5 commits May 31, 2024 14:53

refactor authorized caller and runtime validation flow

a3f4b80

add multi-validation

e39a3da

Add multi validation test

8b282cf

Update FunctionId naming

c8d2b45

rename loupe function

6d48a68

adamegyed force-pushed the adam/multi-validation-uo-sig branch from aefbc5f to 6d48a68 Compare May 31, 2024 18:53

Zer0dot reviewed Jun 4, 2024

View reviewed changes

src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved

adamegyed mentioned this pull request Jun 4, 2024

feat: [v0.8-develop] associate pre-validation hooks with validation functions #64

Merged

howydev approved these changes Jun 6, 2024

View reviewed changes

jaypaik approved these changes Jun 7, 2024

View reviewed changes

huaweigu approved these changes Jun 7, 2024

View reviewed changes

review fixes

def2267

adamegyed merged commit 3d54094 into v0.8-develop Jun 10, 2024
3 checks passed

adamegyed deleted the adam/multi-validation-uo-sig branch June 10, 2024 18:43

adamegyed mentioned this pull request Jun 10, 2024

feat: [v0.8-develop, experimental] multi-validation in calldata #52

Closed

adamegyed mentioned this pull request Jun 26, 2024

[Improvement] Multiple validation function support erc6900/resources#4

Closed

adamegyed added a commit that referenced this pull request Jul 26, 2024

feat: [v0.8-develop] multi validation in user op signature (#62)

4107f37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] #62

feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] #62

adamegyed commented May 21, 2024

howydev commented Jun 5, 2024

howydev commented Jun 6, 2024 •

edited

Loading

jaypaik left a comment

jaypaik Jun 6, 2024

jaypaik Jun 6, 2024

howydev Jun 7, 2024

adamegyed Jun 10, 2024

howydev Jun 10, 2024

adamegyed Jun 10, 2024

jaypaik Jun 7, 2024

adamegyed Jun 7, 2024

jaypaik Jun 7, 2024

adamegyed Jun 10, 2024

huaweigu left a comment

huaweigu Jun 7, 2024

adamegyed Jun 7, 2024

huaweigu Jun 7, 2024

adamegyed Jun 10, 2024

huaweigu Jun 7, 2024

huaweigu Jun 7, 2024 •

edited

Loading

adamegyed commented Jun 10, 2024

adamegyed commented Jun 10, 2024

		if (_storage.selectorData[msg.sig].denyExecutionCount > 0) {
		revert AlwaysDenyRule();

feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] #62

feat: [v0.8-develop, experimental] multi validation in user op signature [5/N] #62

Conversation

adamegyed commented May 21, 2024

Motivation

Solution

Future work

howydev commented Jun 5, 2024

howydev commented Jun 6, 2024 • edited Loading

jaypaik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaweigu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaweigu Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

adamegyed commented Jun 10, 2024

adamegyed commented Jun 10, 2024

howydev commented Jun 6, 2024 •

edited

Loading

huaweigu Jun 7, 2024 •

edited

Loading