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

adamegyed · 2024-06-04T17:40:50Z

Motivation

#62 introduces switching between multiple validation functions, and called out the reasons we should associate pre-validation hooks with validation functions, but did not implement it. Further reasoning for why we should associate pre-validation hooks with validation functions instead of selectors can be found in #52.

To summarize:

If we continue associating pre validation hooks with a selector, then a single pre-validation hook would apply over multiple validation types. If the pre-validation hook reads from something like userOp.signature, then it could break composability.
The combination of N pre-validation hooks plus 1 validation function logically makes up 1 "unit", regardless of the action being validated. This helps enforce security guarantees like "this validation can't transfer my NFTs", because they can't be bypassed by the introduction of a new execution function.

Solution

Rearrange account storage to map validation function to a config containing whether or not it is a shared validation, whether it is a signature validation, and any hooks that may apply.
- (removes the old default & signature validation storage)
Move the installation of pre validation hooks to the temp-installation-bypass function installValidation introduced in feat: [v0.8-develop, experimental] default validation #63. This is needed to be able to apply the pre validation hooks across multiple validation functions. It also shows the need for a full implementation of user-supplied install configs - we shouldn't keep expanding the temp workflow, since we'll need to rearrange it later anyways.
Previously, we required that the magic value for "always deny" exist as a pre-validation hook with the possibility of overlapping installations. The intention of that mechanism was to block certain functions from being run at all, but if pre-validation hooks get associated with the validation functions instead, it can't really work for that. Instead of migrating this to a special-case execution hook, this PR drops them entirely - if a function wants to revert, it can just do so. This also reduces the install flow complexity of accounts, because they no longer need to handle magic values.
- With the removal of this magic value, we can drop the entire "manifest validity" test suite, because it becomes no longer possible for a manifest to represent and invalid combination of magic values and roles!

howydev · 2024-06-07T14:58:31Z

src/account/AccountStorage.sol

+    // Whether or not this validation can be used as a shared validation function.
+    bool isShared;
+    // Whether or not this validation is a signature validator.
+    bool isSignatureValidation;


is it possible to add defaultValidations as a bool field here?

Yep that's what the isShared flag is doing, it's whether or not it counts as a "default" validation. I think the word "default" implies that there's only one ("the default"), but in reality there can be many validations in the shared "default" pool, so I was kinda trying to avoid using that term. But I totally see how this is confusing, can change it or add a comment if it would help.

Would be helpful yeah! How about global validators? Global validators are able to perform validation across all native selectors + installed execution selectors that opt in to global validation

That's the same thing, right?

yup, just a name suggestion, my contribution it to the list of names for the naming meeting :p

We can punt, but I do like the idea of using "global" here. I think it's fine that "global validation functions" don't necessarily apply to all selectors, just like how global variables can be overriden.

We can punt, but I do like the idea of using "global" here. I think it's fine that "global validation functions" don't necessarily apply to all selectors, just like how global variables can be overriden.

yeah that's the behavior I expect as well. We often adhere to default vs overridden in our codebase, but the same idea.

howydev · 2024-06-07T15:08:18Z

src/account/AccountStorage.sol

-    // User operation validation and runtime validation share a function reference.
+    // The execution hooks for this function selector.
+    EnumerableSet.Bytes32Set executionHooks;
+    // Which validation functions are associated with this function selector.
    EnumerableSet.Bytes32Set validations;


Thoughts on removing this entirely? What I'm thinking of:

We just keep the global validation fns set, no more selector associated validation fns

Permissions framework (pre val hooks attached to validation fns) is responsible for what selectors a val fn can operate on

If I'm not wrong this should remove the need to do an existence check across the global validations set + the selector associated validation set and simplify a lot of the checking logic around validation

There's some flexibility in the shape of #2 - a simple MA could just do:

struct ValidationData { bool isShared bool isSignatureValidation mapping(bytes4 selector -> bool canValidate) canValidateSelector EnumerableSet.Bytes32Set preValidationHooks }

to support 2 modes of validation functions - those that can be used either globally, or just for a set of selectors

Without selector-associated validations, every validation would need a pre-hook just to do selector associations - it's one of those things that benefits from being builtin, but yes I agree we should be careful in what we do this for.

I actually thought about making the refactor to the format of mapping(bytes4 selector -> bool canValidate) canValidateSelector, but there's one reason we can't do it just yet - the current interface of our loupe functions.

Right now, we have function getValidations(bytes4 selector) external view returns (FunctionReference[] memory), which returns the validations per selector. Even if we made the canValidateSelector enumerable, we wouldn't be able to implement this interface efficiently, we'd need to search every single validation function.

But, if we change the view function to function getSelectors(FunctionReference validation) external view returns (bytes4[] memory), then we could make this refactor.

We should revisit this in the future, and decide which of these functions it actually makes sense to implement.

We should capture this conversation somewhere. It is very meaningful, but is not in this PR's scope. @howydev Seems you proposed it. Maybe even capture as a proposal to the issues list.

In this world where there are only global validations, how are native functions' validation handled?
Would those native functions just parts of canValidateSelector on some of the validations? (Note, right now, default/global validators can validate any native functions).

Looking at the code below, seems we don't really support default/global validators can validate any native functions natively in code. Users still need to setup by adding the validation functions to each native functions. Did I miss anything?

If the above is the case, then there is no reason to have two set of pools (default, and selector specific), we should combine them.

I discussed this with @fangting-alchemy in a call, documenting here for others:

Users still need to setup by adding the validation functions to each native functions.

This is not necessary, once a validation function is marked as isDefault, it will apply to any function allowed by default validation in _defaultValidationAllowed, which includes the native functions.

The ability to pass in selectors to installValidation is how to install the validation as a non-default validation function. This allows users to install a validation as both a default validation and as a selector-associated one for multiple selectors at once, but in practice I expect most users to do one or the other (either default, or associated with some selectors).

Related thought - have we considered making allowDefaultValidation configurable for native functions too? How would we reliably prevent validating into uninstallPlugin if we want to enforce a time delay? I guess we can just use a pre exec hook instead of a pre validation hook.

Is there ever a case where users might want to opt out of default validation for some subset of native functions? Hmm...

…ys deny magic value

src/account/AccountStorage.sol

fangting-alchemy · 2024-06-19T23:51:53Z

src/account/AccountStorage.sol

@@ -63,21 +65,14 @@ struct AccountStorage {
    mapping(address => PluginData) pluginData;
    // Execution functions and their associated functions
    mapping(bytes4 => SelectorData) selectorData;
+    mapping(FunctionReference => ValidationData) validationData;


This is the defaultValidation pool, right? Should we rename it?

Let's also start using named parameters for mappings to make things clearer. For example:

mapping(FunctionReference validationFunction => ValidationData) ...

It holds some other data per validation function, too. Specifically, whether or not the validation function is allowed to do signature validation (1271's isValidSignature) and the set of pre-validation hooks that apply to the validation function.

Also I added the named mapping parameter validationFunction.

fangting-alchemy · 2024-06-19T23:55:50Z

src/account/PluginManager2.sol

            }
+            _storage.validationData[validationFunction].isDefault = true;
        }

        for (uint256 i = 0; i < selectors.length; ++i) {
            bytes4 selector = selectors[i];
            if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) {


I remember native functions can be validated by any default validation.
If the selector is a native function, and the validation function is default, can we skip this step?

Yes, this step can be skipped in that case. For example, the new initialization method passes an empty bytes4[] selectors to this function:

reference-implementation/src/account/UpgradeableModularAccount.sol

Line 337 in 4d2390b

_installValidation(validationFunction, true, new bytes4[](0), installData, bytes(""));

jaypaik

Love the removal of magic values 🙌

jaypaik · 2024-06-20T13:26:46Z

src/account/AccountStorage.sol

+    // Whether or not this validation can be used as a shared validation function.
+    bool isShared;
+    // Whether or not this validation is a signature validator.
+    bool isSignatureValidation;


We can punt, but I do like the idea of using "global" here. I think it's fine that "global validation functions" don't necessarily apply to all selectors, just like how global variables can be overriden.

jaypaik · 2024-06-20T13:32:09Z

src/account/AccountStorage.sol

@@ -63,21 +65,14 @@ struct AccountStorage {
    mapping(address => PluginData) pluginData;
    // Execution functions and their associated functions
    mapping(bytes4 => SelectorData) selectorData;
+    mapping(FunctionReference => ValidationData) validationData;


Let's also start using named parameters for mappings to make things clearer. For example:

mapping(FunctionReference validationFunction => ValidationData) ...

src/account/PluginManager2.sol

jaypaik · 2024-06-20T13:51:01Z

src/account/PluginManager2.sol

+
+                if (initDatas[i].length > 0) {
+                    (address preValidationPlugin,) = FunctionReferenceLib.unpack(preValidationFunction);
+                    IPlugin(preValidationPlugin).onInstall(initDatas[i]);


So dependencies no longer need to be installed ahead of time? Do we expect the plugin to be able to handle multiple onInstalls with different data?

Side note: It's interesting how we're going back towards the updatePlugins pattern that we had before. 😅

This workflow kinda doesn't touch dependencies at all, since it's not reading from the manifest 😅 .

As for expectations around onInstall, this function currently allows the caller to decide whether or not to send it multiple times - if no data is provided, the onInstall call is skipped. Not sure if that should be the final design or not.

Re: dependencies - I meant in the sense that we had previously operated under the assumption that plugins can only use what is available from plugins installed already. This sort of breaks that and allows us to pull in arbitrary hooks from non-installed plugins.

Not saying the previous approach is right though. I've also been thinking about what it would look like to get rid of the idea of "installing" plugins, and just "using" them instead.

Ah, I see what you mean. In a sense, this function is doing the same for the validation function too - the process of "installing" just becomes setting the storage in the account to allow using it, and optionally sending the onInstall callback with some data.

It does create a discrepancy with how the account currently treats a few things related to plugin install state:

The implementing plugins for both the validation function and pre-validation hooks won't appear in the list of installed plugins from the loupe function getInstalledPlugins().

The validation functions can't be used as dependencies, because the implementing plugin isn't held on that list.

The manifest hash is not stored (nor is the manifest even checked)

These all seem like things we should follow up on. It may not be useful for us to even have a getInstalledPlugins function anymore, if they can be installed multiple times in different circumstances. And the extent of manifest contents and checking needs an overhaul in user-supplied install configs.

😅

I think all worth following up on for sure. We're no longer getting value out of the integrity of the manifest. Happy to have us revisit in a future PR. We're deep into this approach already.

jaypaik

Approving to unblock dependencies!

huaweigu · 2024-06-20T21:10:29Z

This helps enforce security guarantees like "this validation can't transfer my NFTs", because they can't be bypassed by the introduction of a new execution function.

Yep, that's the pain point we had with time lock plugin.

…unctions (#64)

adamegyed requested a review from a team June 4, 2024 17:40

adamegyed mentioned this pull request Jun 4, 2024

feat: [v0.8-develop] remove execute from plugin #65

Merged

adamegyed force-pushed the adam/pre-validation-hook-assoc branch from e21f450 to 72be4f4 Compare June 5, 2024 22:40

howydev reviewed Jun 7, 2024

View reviewed changes

adamegyed mentioned this pull request Jun 10, 2024

feat: [v0.8-develop, experimental] default validation #63

Merged

adamegyed force-pushed the adam/default-validation branch from 73f6768 to 84b44c4 Compare June 10, 2024 18:47

adamegyed force-pushed the adam/pre-validation-hook-assoc branch 3 times, most recently from c872746 to 83a2ddb Compare June 14, 2024 15:56

Base automatically changed from adam/default-validation to v0.8-develop June 19, 2024 14:58

associate pre-validation hooks with validation functions, remove alwa…

4d2390b

…ys deny magic value

adamegyed force-pushed the adam/pre-validation-hook-assoc branch from 83a2ddb to 4d2390b Compare June 19, 2024 15:06

fangting-alchemy reviewed Jun 19, 2024

View reviewed changes

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

fangting-alchemy reviewed Jun 19, 2024

View reviewed changes

jaypaik reviewed Jun 20, 2024

View reviewed changes

Update comments and naming

53b13e9

jaypaik approved these changes Jun 20, 2024

View reviewed changes

adamegyed merged commit 18261cb into v0.8-develop Jun 20, 2024
3 checks passed

adamegyed deleted the adam/pre-validation-hook-assoc branch June 20, 2024 20:35

adamegyed mentioned this pull request Jun 21, 2024

style: [v0.8-develop] propose renaming default to global #83

Merged

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

feat: [v0.8-develop] associate pre-validation hooks with validation f…

462285b

…unctions (#64)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adamegyed commented Jun 4, 2024

howydev Jun 7, 2024

adamegyed Jun 10, 2024

howydev Jun 10, 2024

adamegyed Jun 10, 2024

howydev Jun 10, 2024

jaypaik Jun 20, 2024

huaweigu Jun 20, 2024 •

edited

Loading

howydev Jun 7, 2024 •

edited

Loading

howydev Jun 7, 2024 •

edited

Loading

adamegyed Jun 10, 2024

fangting-alchemy Jun 19, 2024

fangting-alchemy Jun 20, 2024

adamegyed Jun 20, 2024

jaypaik Jun 20, 2024

fangting-alchemy Jun 19, 2024

jaypaik Jun 20, 2024

adamegyed Jun 20, 2024

fangting-alchemy Jun 19, 2024

adamegyed Jun 20, 2024

jaypaik left a comment

jaypaik Jun 20, 2024

jaypaik Jun 20, 2024

jaypaik Jun 20, 2024

adamegyed Jun 20, 2024

jaypaik Jun 20, 2024

jaypaik Jun 20, 2024

adamegyed Jun 20, 2024

jaypaik Jun 20, 2024

jaypaik left a comment

huaweigu commented Jun 20, 2024

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

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

Conversation

adamegyed commented Jun 4, 2024

Motivation

Solution

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 20, 2024 • edited Loading

Choose a reason for hiding this comment

howydev Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

howydev Jun 7, 2024 • edited Loading

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

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

jaypaik left a comment

Choose a reason for hiding this comment

huaweigu commented Jun 20, 2024

huaweigu Jun 20, 2024 •

edited

Loading

howydev Jun 7, 2024 •

edited

Loading

howydev Jun 7, 2024 •

edited

Loading