feat: [v0.8-develop] per validation hook data #66

adamegyed · 2024-06-07T19:15:48Z

Motivation

To better compose plugins, it is useful to allow extra data to be passed to hooks, to prevent having to do non-standard calldata encoding or require re-defining execution functions.

This is necessary for certain types of permissions-enforcing hooks to be used in a composable way – for example, an access control plugin implemented via a merkle tree requires proofs to be passed along with each call.

Solution

This PR implements per-hook data for pre-validation hooks only.

Use the userOp.signature field to pass the extra data into pre user op validation hooks, and adds an authorization field to pre runtime validation hooks to pass in extra data.
- Note that this means that pre user op validation hooks and the user op validation function can no longer see the signature data of each other. AFAICT this is an acceptable change to make, but please review this.
To more easily address individual hooks by index, change the storage of pre-validation hooks to be a storage array, rather than an enumerable set. This has no changes to the external interface of the account.
Because not all hooks require data, and because ABI-encoding nested arrays is expensive, define a format for sparse calldata segments. This format consists of a data length, an index, and the data itself, with the requirement that the indices are in ascending order. Non-used indices are skipped in the encoding.
- To minimize space, the indices use a single byte. This caps the total number of pre-validation hooks at 255, which also serves as an effective DOS mitigation, to prevent too many hooks from being installed.
Use the spare calldata segments to perform the calls to pre-validation hooks and validation functions.
Add a test case with a mock plugin that uses per-hook data. This is intended to mimic the merkle proof workflow, but does not actually use the merkle proof library - instead it just asserts that an address is present in calldata.

Zer0dot · 2024-06-12T12:51:25Z

Just to reiterate some points from previous discussions, my main concern lies in this being part of the signature field. We've discussed that similar designs in the past have had some issues but it's difficult to find a better spot.

What's the downside of including it in the beginning of the calldata field instead?

adamegyed · 2024-06-12T14:29:25Z

We could include it in calldata, that would help us by ensuring the data is signed over, making it non-malleable. It would come with some additional considerations:

We would probably want to put it at the end of calldata, rather than the beginning, to prevent the data from interfering with solidity's function dispatcher
We would need to strip it out before forwarding to execution functions in the fallback, otherwise plugins may interpret it as something else.
It would be tricky to determine how much data is actually provided, if any. Because the account doesn't know the parameter format of installed execution functions, it's not clear to me how it would distinguish between data for hooks and data for the actual execution function.
- For native functions, it would be possible, but expensive, as we'd have to re-implement the ABI decoding that solidity does automatically for every external function, then look past-the-end of the field encoded last.

Do you know of any way we could encode it in calldata such that the account could parse it out? We're constrained by the fact that the account doesn't know the parameter format of plugin-defined execution functions.

adamegyed · 2024-06-12T14:31:50Z

I do agree that using the signature is error prone. I just realized there's an issue with the current implementation, you can express zero-length hook data in two different ways: specifying the index and setting a length of zero, or skipping the encoding. I'll update the PR to enforce 1 canonical encoding format.

adamegyed · 2024-06-14T16:07:19Z

Updated to enforce canonical encoding, it is no longer possible to add zero-length segments to the signature. Instead, zero length segments must be omitted. It is up to the validation function and hooks to enforce that only the expected amount of data is provided.

src/account/PluginManager2.sol

howydev · 2024-06-21T19:46:46Z

Taking a look, overall thoughts so far:

Sparse calldata lib is super cool!
Overall, think this is in line with the v0.8 changes to improving efficiency by moving storage to calldata. Want to call out that this creates more burden on SDKs and potentially requires some other source of data storage.
For pre UO validation hooks, this makes sense. But for the pre runtime validation path, specifically, I would not expect other plugins to use executeWithAuthorization with extra calldata for the standard call flow of account -> plugin -> account. Mostly because the plugin would need to store this extra calldata to pass back into the account, so any savings from moving from calldata -> storage is lost

huaweigu · 2024-06-24T19:06:43Z

src/account/UpgradeableModularAccount.sol

+                }
+
+                // Load the next per-hook data segment
+                (authSegment, authorizationData) = authorizationData.getNextSegment();


The recursion implemented here is neat! This appears to be the the most efficient approach, aside from key-value structure, which is not feasible within internal functions.

src/helpers/SparseCalldataSegmentLib.sol

src/interfaces/IValidationHook.sol

src/helpers/SparseCalldataSegmentLib.sol

huaweigu · 2024-06-24T19:22:26Z

src/account/UpgradeableModularAccount.sol

@@ -430,9 +468,13 @@ contract UpgradeableModularAccount is
            }
        }

+        if (authSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) {


We can probably move _RESERVED_VALIDATION_DATA_INDEX into the library and expose a helper function, something like hasNext()

How would that look? The getIndex function is used to determine if the next segment applies for the current function (pre-validation hook or validation function), or for a later function. I think a hasNext function would also need to return the index, so it seems pretty similar to the existing setup.

Zer0dot · 2024-06-25T13:36:31Z

src/account/UpgradeableModularAccount.sol

-        for (uint256 i = 0; i < preUserOpValidationHooksLength; ++i) {
-            bytes32 key = preUserOpValidationHooks.at(i);
-            FunctionReference preUserOpValidationHook = toFunctionReference(key);
+        for (uint256 i = 0; i < preUserOpValidationHooks.length; ++i) {


mega smol but could we rename "i" to preValidationHookIndex since we use it for a few comparisons?

Zer0dot · 2024-06-25T13:37:06Z

src/account/UpgradeableModularAccount.sol

-        for (uint256 i = 0; i < preRuntimeValidationHooksLength; ++i) {
-            bytes32 key = preRuntimeValidationHooks.at(i);
-            FunctionReference preRuntimeValidationHook = toFunctionReference(key);
+        for (uint256 i = 0; i < preRuntimeValidationHooks.length; ++i) {


Echoing my previous comment to potentially rename this to preValiationHookIndex or something-- not a hard req by any means.

Zer0dot · 2024-06-25T13:38:45Z

I do agree that using the signature is error prone. I just realized there's an issue with the current implementation, you can express zero-length hook data in two different ways: specifying the index and setting a length of zero, or skipping the encoding. I'll update the PR to enforce 1 canonical encoding format.

I overall agree with the change, but what abuses could this lead to (forgive me if we talked about this already)?

Edit: followup wrt abuses of this being in the signature field-- if it's only used to provide actual signature and signature-like information for authorization, I think it should be fine. Correct me if I'm wrong. Developers will need to keep this in mind when building validation hooks though.

Zer0dot · 2024-06-25T14:17:41Z

Are we sure we only want to call onInstall() and onUninstall() when there is nonzero data to be passed? What if I want to pass an empty array but still trigger this?

(Edit: This is specifically for PluginManager2 temp functions, in PluginManagerInternals we do call this even if the bytes array has a length of zero)

test/mocks/plugins/MockAccessControlHookPlugin.sol

src/account/UpgradeableModularAccount.sol

jaypaik · 2024-06-28T13:05:35Z

src/helpers/SparseCalldataSegmentLib.sol

+/// than inline as part of the tuple returned by `getNextSegment`.
+library SparseCalldataSegmentLib {
+    /// @notice Splits out a segment of calldata, sparsely-packed
+    /// @param source The calldata to extract the segment from


Could be useful to document the expected format of the source, e.g., [ len(segment0) - 1] [ segment0 ] ... [ len(segmentN) - 1 ][ segmentN ]

Also maybe could be easier to reason about if the length of the segment included the index byte?

I think there might have been a versioning issue - previously I didn't include the index in the segment length, but I've since updated it to include it. And sounds good on the comment, will add it in as a "spec" for the encoding.

jaypaik · 2024-06-28T13:22:00Z

src/account/UpgradeableModularAccount.sol

-            currentValidationData = IValidationHook(plugin).preUserOpValidationHook(functionId, userOp, userOpHash);
+            (address plugin, uint8 functionId) = preUserOpValidationHooks[i].unpack();
+            uint256 currentValidationData =
+                IValidationHook(plugin).preUserOpValidationHook(functionId, userOp, userOpHash);


Thoughts on introducing an additional parameter here for both preUserOpValidationHook and validateUserOp similar to the runtime path? That would allow us to keep the signature the same. What are the downsides?

It just adds to the gas cost, and I'm not sure we have a good use case for the entire sig at the moment. With the runtime path, there wasn't anywhere else we could put it.

jaypaik

Approving to unblock. Mind making a note of the unresolved comments to come back to later (if they aren't addressed before merge)?

huaweigu

lgtm

adamegyed requested a review from a team June 7, 2024 19:15

adamegyed force-pushed the adam/remove-ipluginexecutor branch from b680f75 to 7cb19a4 Compare June 10, 2024 18:53

adamegyed force-pushed the adam/per-hook-data branch 2 times, most recently from cab3262 to 62344f2 Compare June 10, 2024 18:58

adamegyed force-pushed the adam/remove-ipluginexecutor branch from 07b307a to bfdf5b5 Compare June 12, 2024 18:44

adamegyed force-pushed the adam/per-hook-data branch from 62344f2 to 0e67b55 Compare June 12, 2024 18:48

adamegyed force-pushed the adam/remove-ipluginexecutor branch from bfdf5b5 to 67b7088 Compare June 14, 2024 15:56

adamegyed force-pushed the adam/per-hook-data branch from 0e67b55 to c2da4d0 Compare June 14, 2024 15:57

adamegyed force-pushed the adam/remove-ipluginexecutor branch from 67b7088 to 29055a9 Compare June 19, 2024 15:06

adamegyed force-pushed the adam/per-hook-data branch from 64a9917 to 23cdc39 Compare June 19, 2024 15:06

fangting-alchemy reviewed Jun 20, 2024

View reviewed changes

src/account/PluginManager2.sol Show resolved Hide resolved

fangting-alchemy reviewed Jun 20, 2024

View reviewed changes

src/account/PluginManager2.sol Show resolved Hide resolved

adamegyed force-pushed the adam/remove-ipluginexecutor branch from 29055a9 to 0aefb0f Compare June 20, 2024 17:53

adamegyed force-pushed the adam/per-hook-data branch from 23cdc39 to 4599a2d Compare June 20, 2024 17:53

adamegyed force-pushed the adam/remove-ipluginexecutor branch from 0aefb0f to c9fcabf Compare June 20, 2024 20:37

adamegyed force-pushed the adam/per-hook-data branch from 4599a2d to 8a09d31 Compare June 20, 2024 20:37

Base automatically changed from adam/remove-ipluginexecutor to v0.8-develop June 20, 2024 20:49

adamegyed force-pushed the adam/per-hook-data branch 2 times, most recently from d402361 to 60916fa Compare June 20, 2024 21:01

huaweigu reviewed Jun 24, 2024

View reviewed changes

Zer0dot reviewed Jun 25, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from 2afe76b to 94b7592 Compare June 26, 2024 16:22

adamegyed force-pushed the adam/per-hook-data branch 2 times, most recently from e118421 to 94a3764 Compare June 27, 2024 20:07

jaypaik reviewed Jun 27, 2024

View reviewed changes

test/mocks/plugins/MockAccessControlHookPlugin.sol Outdated Show resolved Hide resolved

jaypaik reviewed Jun 28, 2024

View reviewed changes

jaypaik approved these changes Jun 28, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from 94a3764 to e4526cc Compare June 28, 2024 15:29

huaweigu approved these changes Jun 28, 2024

View reviewed changes

adamegyed force-pushed the adam/per-hook-data branch from e4526cc to 68a5847 Compare June 28, 2024 18:38

adamegyed mentioned this pull request Jul 8, 2024

[Improvement] Dynamic input for multiple hooks erc6900/resources#32

Open

adamegyed linked an issue Jul 8, 2024 that may be closed by this pull request

[Improvement] Dynamic input for multiple hooks erc6900/resources#32

Open

adamegyed mentioned this pull request Jul 8, 2024

[Improvement] Support for temporary runtime permissions erc6900/resources#33

Open

per validation hook data

b579102

adamegyed force-pushed the adam/per-hook-data branch from 68a5847 to b579102 Compare July 9, 2024 20:46

adamegyed merged commit de28c60 into v0.8-develop Jul 9, 2024
3 checks passed

adamegyed deleted the adam/per-hook-data branch July 9, 2024 20:51

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

feat: [v0.8-develop] per validation hook data (#66)

c0b107e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [v0.8-develop] per validation hook data #66

feat: [v0.8-develop] per validation hook data #66

adamegyed commented Jun 7, 2024

Zer0dot commented Jun 12, 2024

adamegyed commented Jun 12, 2024

adamegyed commented Jun 12, 2024

adamegyed commented Jun 14, 2024

howydev commented Jun 21, 2024

huaweigu Jun 24, 2024

huaweigu Jun 24, 2024

adamegyed Jun 26, 2024

Zer0dot Jun 25, 2024

Zer0dot Jun 25, 2024

Zer0dot commented Jun 25, 2024 •

edited

Loading

Zer0dot commented Jun 25, 2024 •

edited

Loading

jaypaik Jun 28, 2024

adamegyed Jun 28, 2024

jaypaik Jun 28, 2024

adamegyed Jun 28, 2024

jaypaik left a comment

huaweigu left a comment

feat: [v0.8-develop] per validation hook data #66

feat: [v0.8-develop] per validation hook data #66

Conversation

adamegyed commented Jun 7, 2024

Motivation

Solution

Zer0dot commented Jun 12, 2024

adamegyed commented Jun 12, 2024

adamegyed commented Jun 12, 2024

adamegyed commented Jun 14, 2024

howydev commented Jun 21, 2024

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

Zer0dot commented Jun 25, 2024 • edited Loading

Zer0dot commented Jun 25, 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

jaypaik left a comment

Choose a reason for hiding this comment

huaweigu left a comment

Choose a reason for hiding this comment

Zer0dot commented Jun 25, 2024 •

edited

Loading

Zer0dot commented Jun 25, 2024 •

edited

Loading