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

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

Closed
wants to merge 1 commit into from

Conversation

adam-alchemy
Copy link
Contributor

Motivation

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

Proposed Solution

This PR implements the first suggestion described in the issue above, implementing a new native function called executeWithValidation(FunctionReference validation, bytes calldata data).

Note that this does not implement the function executeWithContext, where additional data can be made available to validation plugins, because the current validation interface does not support out-of-band extra data, beyond what's in the user operation for UO validation and calldata for RT validation.

Account behavior changes:

  • Validation is now an enumerable set, rather than a single field.
    • Internal installation functions are updated to add and remove from the set.
    • The manifest format stays the same in this PR.
  • When the function executeWithValidation is put into userOp.callData, user op validation will rely on the provided validation function, if it is installed and made available to the actual execution function selector being called within the data parameter.
  • When the function executeWithValidation is called directly, runtime validation will rely on the provided validation function, if it is installed and made available to the actual execution function selector being called within the data parameter.
  • Then, executeWithvalidation forwards the call in data to itself with a self-call (not self-delegatecall).
  • To correctly implement this functionality, this PR also changes account behavior with self-calls: now, if the call is from self, runtime validation will be bypassed (validation is assumed to have passed in a different context).
    • This is necessary to support multi-validation for native functions if we don't implement an internal switching function and make all functions public. Doing this would involve a large account code change and overhead to implementers, particularly for semi-modular accounts, so I don't prefer it as a solution.
    • Note that this changes the security properties of validation over execute and executeBatch - no change is made to those functions here, if we wish to pursue this form of multi-validation, we will have to address that eventually.
  • The account loupe is updated to add a function that supports retrieving all possible applicable validation functions for a selector.
  • This PR does not change how pre-validation hooks are assigned, they remain associated with the execution selector. For reasons outlined below, I believe this needs to change, but this PR does not do that yet.
  • Add a test to show switching between multiple installed validation functions: MultiValidation.t.sol.

Additional discussion

Reasons for choosing a calldata-based encoding of validation selection

  • If we wish to support switching validation methods with a user-specified field for runtime validation, the switching logic must exist in calldata, because other user operation fields are not present when directly calling the account.

Ramifications of choosing calldata to specify validation switching

  • While we can support switching validation methods for runtime validation, we likely also want to support calling functions via runtime validation without wrapping the call in the executeWithValidation function format.
  • The reason calling functions without nesting them into the data field of executeWithValidation is useful, is that this is how we make use of contract interfaces that the account supports through execution function. I.e. in tests, we may wrap the account with a different interface like SingleOwnerPlugin(account1).transferOwnership(...), or frontends may use the plugin abi to generate function calls on the account.
  • To support those "direct call" flows, we must have a "default validator" to use for validation when the validation-switching function is not used.
    • In other words, runtime validation + execution functions implementing interaces necesitate a "default validation" to exist.
  • Note that this is a "default validation" in a different sense than [Improvement] Enshrine account default validation feature resources#37 describes: the issue describes the ability for validation to apply over some number of execution functions, despite not manually being added to each. In this PR, I used "default validation" to refer to which validation to run when one isn't manually specified via executeWithValidation.
  • In the implementation of this PR, the first validation added for an execution function becomes the default. The behavior gets weird with the OZ enumerbleset if the first added one is removed and others exist. If we wish to pursue this mechanism for validation switching, we will need to expose setting this field to the end user somehow.

Ramifications of multi-validation in general

  • You can't install two instances of the same plugin twice, if they define any execution functions. This happens because, upon trying to install the second instance, they will have two definitions for execution functions, and collisions are not permitted for execution functions.
    • See the test cases in MultiValidation.t.sol: we have to define a new version of SingleOwnerPlugin with the execution function names changed: transferOwnership -> transferOwnership2, owner -> owner2.
    • Does this warrant breaking the invariant previously established where we assert "target of execute/executeBatch is not a plugin?
    • Should the decision to install execution function handlers be user-controlled, as part of [Improvement] User-supplied install config resources#9?
  • Pre-validation hooks are currently associated to selectors. Question: should they be associated with selectors, validation functions, or validation functions + selectors?
    • If we continue associating them only 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.
    • If we attach it to both selector + validation functions, and the intent of the pre-validation hook is to limit the capabilities of a given validation function, then the pre-validation hook can be bypassed whenever the validation function is added to a new function via a dependency.
      • (this is also the most gas-expensive option)
    • This leaves associating pre-validation hooks with validation functions directly as the last remaining option. Generally, I think this makes the most sense - the combination of N pre-validation hooks plus 1 validation function logically makes up 1 "unit", regardless of the action being validated.
      • This does still have some degree of composability risk: if the pre-validation hook is inspecting the contents of calldata, and the validation function it is attached to gets added to an execution function that it is unaware of the calldata encoding for, then it must deny execution, otherwise risk a breach of access control.
        • If we support installing the same validation function multiple times, with different pre-validation hooks and domain of access, this problem can be avoided.
      • This pattern is also difficult to use with the current install flow, if the pre-validation hook defining plugin is intended to be composable. The mechanism used for composability right now is dependencies, but only of a fixed number. To allow associating pre-validation hooks with an arbitrary number of validation functions, possibly including validation functions defined later, would require a different model of install state than what we currently have.
  • Does multi-validation necessitate creating specializations of hooks that only apply to specific validation functions? Previously, there was an invariant held that 1 exec selector had 1 validation function, which influenced stateful hook design.

Conclusion

Doing multi-validation while supporting runtime validation in its current form introduces some added complexities that we had previously not discovered: the requirement to put validation selection into calldata, which itself leads to an additional requirement of having a default validation per selector. We may want to consider changing how we handle runtime validation to make multi-validation support easier.

Additionally, multi-validation introduces association complexities to pre-validation hooks, regardless of the switching mechanism chosen. Pre-validation hooks make the most sense to be associated with validation functions, rather than execution selectors; however, doing so is difficult with the current parameter format for installPlugin dependencies, and requires changes there as well.

And finally, the use of execution functions as the exclusive way to do for plugin state management makes it impossible to install two instances of the same validation plugin at the same time. This is a big composability break, and we should consider removing the check in standard execute functions to be non-plugin contracts.

@adam-alchemy adam-alchemy requested a review from a team May 8, 2024 20:06
@adam-alchemy adam-alchemy marked this pull request as draft May 8, 2024 20:08
@adam-alchemy adam-alchemy force-pushed the adam/multi-validation-calldata branch from e210379 to 56e9e72 Compare May 8, 2024 23:02
Base automatically changed from adam/simplify-hooks-2 to v0.8-develop May 8, 2024 23:10
@adam-alchemy adam-alchemy force-pushed the adam/multi-validation-calldata branch from 56e9e72 to 2a73e35 Compare May 8, 2024 23:15
@@ -38,7 +39,7 @@ abstract contract AccountLoupe is IAccountLoupe {
config.plugin = _storage.selectorData[selector].plugin;
}

config.validationFunction = _storage.selectorData[selector].validation;
config.defaultValidationFunction = toFunctionReference(_storage.selectorData[selector].validations.at(0));
Copy link
Collaborator

@howydev howydev May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(small nit) maybe primaryValidationFunction?

@0xrubes
Copy link

0xrubes commented May 13, 2024

Awesome I like it!! I wonder if we should perhaps embed the concept of the default validation function a bit deeper into the standard, i.e. it is not implicitly specified by being the first entry in the list, but explicitly through a new storage field in the SelectorData struct for each selector. This way we would avoid weirdnesses with the EnumerableSet` for the validation functions (e.g. in case we are uninstalling the current default validation function, the lastly added validation function would automatically become the new default validation function).

If we would also add a new native function updateDefaultValidationFunction(bytes4 selector, FunctionReference validationFunction)to set the default validation function for a selector, we could also explicitly handle some edge cases. E.g. users could be required to replace the current default validation function before the plugin providing the current default validation function can be uninstalled. This could be achieved by having some counter for each plugin to track for how many instances default validation function it provides, similarly to the PluginData.dependentCount.

Perhaps the standard default validation function for function selectors a plugin introduces can be specified by the plugin manifest. Also, in case a plugin installs the first validation function, e.g. for a native selector on initial configuration, it could be automatically promoted to the default validation function.

The EnumerableSet validations could then also only contain non-default validation functions, while the default one is stored in the SelectorData.defaultValidationFunction.

Could flesh this out more, but for now wondering if I’m heading towards over-engineering it or if I’m being reasonable here.

Edit: Ah, just realized this idea has some overlap with erc6900/resources#37, only that I'm proposing having the default validation configurable for each selector, not on a plugin level.

@0xrubes
Copy link

0xrubes commented May 13, 2024

Also, as far as I'm seeing it, the security implications do not just affect execute() and executeBatch(), but also executeFromPluginExternal() and all plugins that can theoretically call the account itself. A check for (target == address(this)) is missing in the reference implementation's version of executeFromPluginExternal() 👀

@adamegyed
Copy link
Contributor

Closing, superseded by #62.

@adamegyed adamegyed closed this Jun 10, 2024
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

Successfully merging this pull request may close these issues.

4 participants