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: add flag for user op validation and pack efficiently #160

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

adamegyed
Copy link
Contributor

Motivation

While it helps to group the 3 types of validations together (user op, runtime, and signature), they do have certain differences in how they work. For example, while a hook can relatively easily limit what addresses a validation can call from execute/executeBatch via user ops or runtime calls, it is not as straightforward for understanding the context of signatures within a hook. Thus, we've introduced flags into the base account to differentiate between validations capable of producing a 1271 signature, and those that aren't.

There is a similar difference between user op and runtime validation, and that is the implicit ability for user op validation to spend native tokens on gas. Additionally, if permissions aren't fully enforced, there may be some paymasters that user op validations can make use of that spend other account resources, like tokens or some offchain balance.

Thus, it would be useful to have a similar flag limiting a validation's ability to validate user ops. This could be used for setting something to only be an ephemeral signature validation for a specific login. Or, you may wish to more actively protect the account from modules with direct call access, like a cold storage module or an account recovery module.

Solution

Add a third validation flag, isUserOpValidation, to the ValidationConfig type and ValidationData in storage.

Check that this flag applies in _execUserOpValidation, revert otherwise.

Pack ValidationConfig into a bytes25, rather than a bytes26.

Add this to the loupe struct.

Add tests for isSignatureValidation and isUserOpValidation flags.

Update existing tests.

@adamegyed adamegyed requested a review from a team August 23, 2024 23:21
@adamegyed adamegyed force-pushed the adam/add-validation-uo-flag branch 2 times, most recently from c01df20 to ce67b50 Compare August 29, 2024 18:23
@adamegyed adamegyed mentioned this pull request Aug 29, 2024
@Zer0dot
Copy link
Contributor

Zer0dot commented Aug 29, 2024

Is there a reason we didn't introduce a runtime validation flag? As it seems right now, you can opt out of using a validation for UOs and 1271, but you can't opt out of runtime. Right?

@@ -5,7 +5,7 @@ import {ExecutionManifest} from "./IExecutionModule.sol";

type ModuleEntity is bytes24;

type ValidationConfig is bytes26;
type ValidationConfig is bytes25;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: would be helpful to describe these UDVTs inline with NatSpec comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will port over the comment blocks from the corresponding libraries

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another things I find that will be helpful is have a line comments on what the 25 bytes for in the IModularAccount. Each time I have to find the lib and figure out where each flag/data locate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! I think this includes what you're describing @fangting-alchemy

@adamegyed
Copy link
Contributor Author

you can opt out of using a validation for UOs and 1271, but you can't opt out of runtime. Right?

This is correct. I think logically, the way to opt-out of runtime validation is to have no selectors added and to not be a global validation - then, there's nothing the validation may call on the account. But if you have UO validation but not RT validation, it's still possible to make calls, in addition to also spending gas, so it doesn't seem valuable to remove the RT validation as a matter of permissions enforcement.

@Zer0dot
Copy link
Contributor

Zer0dot commented Aug 29, 2024

This is correct. I think logically, the way to opt-out of runtime validation is to have no selectors added and to not be a global validation - then, there's nothing the validation may call on the account. But if you have UO validation but not RT validation, it's still possible to make calls, in addition to also spending gas, so it doesn't seem valuable to remove the RT validation as a matter of permissions enforcement.

Makes sense, thanks! Only one other comment, is the benefit of using a uint8 flags value instead of just getting the individual flags codesize?

No strong opinion either way, I think flags retrieved as a uint8 are fine too. Beyond that, lgtm!

@adamegyed
Copy link
Contributor Author

is the benefit of using a uint8 flags value instead of just getting the individual flags codesize?

It was actually stack-too-deep 😅

@adamegyed adamegyed force-pushed the adam/add-validation-uo-flag branch from ce67b50 to e3d31f2 Compare August 29, 2024 21:24
@adamegyed adamegyed force-pushed the adam/add-validation-uo-flag branch from e3d31f2 to 22ff5e5 Compare August 29, 2024 21:29
@adamegyed adamegyed merged commit 3ca2c30 into develop Aug 29, 2024
3 checks passed
@adamegyed adamegyed deleted the adam/add-validation-uo-flag branch August 29, 2024 22:11
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