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

[draft] feat: [v0.8-develop] composable validation example #89

Closed
wants to merge 5 commits into from

Conversation

adamegyed
Copy link
Contributor

Motivation

It would be useful to have a demonstration of a composable validation workflow using existing account behavior and interfaces.

Solution

Ports over example contracts from the closed PR #84, and modify them to work without any account changes.

The two ported validation plugins are MultisigPlugin and ECDSAValidationPlugin. There is a test contract ComposableValidation.t.sol that has examples of:

  • Using the ECDSA validation on its own.
  • Using the composable multisig plugin to point to two different validation ids within the ECDSA validation plugin.
  • Using the composable mutlisig plugin to point to:
    • One ECDSA validation id
    • One multisig plugin validation id, that points to two other ECDSA validation ids

This example uses userOp.sender as the address to look up, and userOp.signature as the signature field. In the validator-experiments repo, these were pulled out as separate fields instead.

Also note, this only demonstrates composable user op validation. Runtime validation and signature validation are not composable here because their interface doesn't have a field for the account - if that were added, they could be composable.

@adamegyed adamegyed force-pushed the adam/example-composable-validation branch from 3018306 to 2e49357 Compare June 28, 2024 20:17
Comment on lines +59 to +63
// TODO: not composable here, need to add a param to `validateRuntime` to pass in the account.
if (sender != owners[functionId][msg.sender]) {
revert NotAuthorized();
}
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The risk of allow a param and have this function be called outside of account can be huge. However does it ensure the caller is sending the correct Account and sender since there is no UO or sig attached?
Example, a caller can sender over AccountA with SenderA for accountB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think as long as this function is a view function (no state modifications), it's fine to parameterize the account instead of relying on msg.sender. If a validation function calls out to another validation function, it will be part of the internal logic of the outer validation function, and the account will still only trust the output of the function actually installted.

E.g. consider the scenario

account --calls--> validation function A --calls--> validation function B

In this setup, validation function A and B can both take in a parameterized sender. The account will send its own address, and validation function A should send the same account address. But even if it doesn't, there's no security vulnerability or escalation - the account trusts A, but does not trust B to be a validation function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. I think in RI, we are covered. Could there be out of context usage of validateRuntime?
Maybe a word of caution is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we block it from being installed, because it's a known selector.

Other than that, I think we should be good? If someone calls it directly with execute/executeBatch, it shouldn't do anything.

override
returns (uint256)
{
OwnerInfo storage info = ownerInfo[functionId][userOp.sender];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, wouldn't this violate the storage access rules of 4337?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The account address is the innermost mapping lookup, so the storage should be associated. If we did ownerInfo[userOp.sender][functionId] it would not be associated.

As for switching from msg.sender to userOp.sender, It should be OK as long as the account, and any validation functions installed on the account, choose to send the correct account address during validation. When going from the account to the outermost validation function, it will always be the correct address, and each inner validation function call needs to also propagate the correct address. If it doesn't, then it would be a bug in the validation plugin that makes it not usable via a regular bundler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we did ownerInfo[userOp.sender][functionId] it would not be associated.

Is this a quirk? How would switching the order make a difference?
ownerInfo[userOp.sender][functionId] => keccak256(abi.encode(functionId, keccak256(abi.encode(userOp.sender, p))))
v.s.
ownerInfo[functionId][userOp.sender] => keccak256(abi.encode(userOp.sender, keccak256(abi.encode(functionId, p))))

The docs are not very clear. Based on what they say there, both above should be valid since the slot is uniquely related this address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs state that the following is one way to get an associated storage slot:

keccak256(A || X) + n

Where A is the associated address, X is any value, and n is a number between 0 and 128. By solidity's convention, X will be the "virtual" storage slot of the mapping, which doesn't actually hold anything.

In the first case, where it is not associated, the hashing to get the slot is:

keccak256(abi.encode(functionId, keccak256(abi.encode(userOp.sender, p))))

The function ID is not the associated address, so the output of this expression will not be an associated storage slot. The address gets included in the nested keccak256 call, but that expression becomes X, not A, in the associated storage calculation.

I found this resource to be helpful in understanding solidity's builtin calculation of mapping storage slots: https://programtheblockchain.com/posts/2018/03/09/understanding-ethereum-smart-contract-storage/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed keccak256(A || X) + n from the docs.
Good catch The address gets included in the nested keccak256 call, but that expression becomes X, not A, in the associated storage calculation..

Have any idea why keccak256(X || f(A)) + n not included? If the goal is uniqueness, it satisfies. Might be a bit hard to enforce the rule in the client/bundler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if you include both, then it loses uniqueness. For example, a mapping(address outer => mapping(address inner => uint256)) value; would be associated with both the outer and inner addresses. This happens with a lot of ERC-20 allowances - by default, the OZ ERC20 contract will have allowances associated with the spender, instead of the token holder.

As for why they didn't choose keccak256(X || f(A)) + n instead of the actual, I think it's just easier on the tracer code within the bundler to detect. But it does make it more annoying for solidity devs, lol.

bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
bytes4 internal constant _1271_INVALID = 0xffffffff;

mapping(uint8 id => mapping(address account => address)) public owners;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having an id only on the plugin won't expand the plugin validation function to be able to be installed on the account more than once without the account essentially stores validations differently (not use FunctionReference as a key).

Plugins can build the storage whatever way they want as long as they dont violate the rules.

Like we discussed in the sync today, using an ID or not does not change if a validation plugin can be a layered validation (to be used by account's other validation functions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the functionId (or validation id, entity id, whichever name we choose) does not specify a new storage space in the plugin, then I think that would prevent multiple installs. On the second install, the storage would overlap any previous installation, and prevent unique configurations for the installations in different places.

And while you technically can compose validation (layer it) without allowing multiple installations at once, it means you would need to deploy a new contract instance of the validation plugin each time you want to install it again, which is more expensive than allowing multiple installations via an ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is that having the ID only on plugins won't enable the feature (multiple installation with the same validation plugin).
The account need to change how it organizes validation functions to enable this feature.
In summary, this feature will require account changes and how account and validation plugin communicates (during installation). One example is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, yes that will need to not be pulled from the manifest. The account-controlled auto assign mechanism works, or we can make it user-specified.

@adamegyed adamegyed force-pushed the adam/refactor-validation-mapping branch from 509ba03 to 1bfd893 Compare July 10, 2024 15:46
Base automatically changed from adam/refactor-validation-mapping to v0.8-develop July 10, 2024 15:55
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.

2 participants