-
Notifications
You must be signed in to change notification settings - Fork 25
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 user op signature [5/N] #62
Changes from all commits
a3f4b80
e39a3da
8b282cf
c8d2b45
6d48a68
def2267
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,11 +86,12 @@ abstract contract PluginManagerInternals is IPluginManager { | |
{ | ||
SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; | ||
|
||
if (_selectorData.validation.notEmpty()) { | ||
// Fail on duplicate validation functions. Otherwise, dependency validation functions could shadow | ||
// non-depdency validation functions. Then, if a either plugin is uninstall, it would cause a partial | ||
// uninstall of the other. | ||
if (!_selectorData.validations.add(toSetValue(validationFunction))) { | ||
revert ValidationFunctionAlreadySet(selector, validationFunction); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should come back this naming problem (validation vs. validation function vs. ?) later. Same with |
||
} | ||
|
||
_selectorData.validation = validationFunction; | ||
} | ||
|
||
function _removeValidationFunction(bytes4 selector, FunctionReference validationFunction) | ||
|
@@ -99,7 +100,9 @@ abstract contract PluginManagerInternals is IPluginManager { | |
{ | ||
SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; | ||
|
||
_selectorData.validation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; | ||
// May ignore return value, as the manifest hash is validated to ensure that the validation function | ||
// exists. | ||
_selectorData.validations.remove(toSetValue(validationFunction)); | ||
} | ||
|
||
function _addExecHooks( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,7 @@ contract UpgradeableModularAccount is | |
// Wraps execution of a native function with runtime validation and hooks | ||
// Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin | ||
modifier wrapNativeFunction() { | ||
_doRuntimeValidationIfNotFromEP(); | ||
_checkPermittedCallerIfNotFromEP(); | ||
|
||
PostExecToRun[] memory postExecHooks = _doPreExecHooks(msg.sig, msg.data); | ||
|
||
|
@@ -133,7 +133,7 @@ contract UpgradeableModularAccount is | |
revert UnrecognizedFunction(msg.sig); | ||
} | ||
|
||
_doRuntimeValidationIfNotFromEP(); | ||
_checkPermittedCallerIfNotFromEP(); | ||
|
||
PostExecToRun[] memory postExecHooks; | ||
// Cache post-exec hooks in memory | ||
|
@@ -262,6 +262,42 @@ contract UpgradeableModularAccount is | |
return returnData; | ||
} | ||
|
||
/// @inheritdoc IPluginExecutor | ||
function executeWithAuthorization(bytes calldata data, bytes calldata authorization) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again re: naming (we can punt) - this is still (runtime) validation, so a bit surprising to introduce new vocabulary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah happy to revisit. For some background on why I started with this, I saw this field as analogous to the "signature" data in user ops, but we've overloaded that term in too many places already (including 1271), and it's just the authorization for one particular action. |
||
external | ||
payable | ||
returns (bytes memory) | ||
{ | ||
bytes4 execSelector = bytes4(data[:4]); | ||
|
||
// Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. | ||
FunctionReference runtimeValidationFunction = FunctionReference.wrap(bytes21(authorization[:21])); | ||
|
||
AccountStorage storage _storage = getAccountStorage(); | ||
|
||
// check if that runtime validation function is allowed to be called | ||
if (_storage.selectorData[execSelector].denyExecutionCount > 0) { | ||
revert AlwaysDenyRule(); | ||
} | ||
if (!_storage.selectorData[execSelector].validations.contains(toSetValue(runtimeValidationFunction))) { | ||
revert RuntimeValidationFunctionMissing(execSelector); | ||
} | ||
|
||
_doRuntimeValidation(runtimeValidationFunction, data, authorization[21:]); | ||
|
||
// If runtime validation passes, execute the call | ||
|
||
(bool success, bytes memory returnData) = address(this).call(data); | ||
|
||
if (!success) { | ||
assembly ("memory-safe") { | ||
revert(add(returnData, 32), mload(returnData)) | ||
} | ||
} | ||
|
||
return returnData; | ||
} | ||
|
||
/// @inheritdoc IPluginManager | ||
function installPlugin( | ||
address plugin, | ||
|
@@ -360,18 +396,28 @@ contract UpgradeableModularAccount is | |
revert AlwaysDenyRule(); | ||
} | ||
|
||
FunctionReference userOpValidationFunction = getAccountStorage().selectorData[selector].validation; | ||
// Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. | ||
FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); | ||
adamegyed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
validationData = _doUserOpValidation(selector, userOpValidationFunction, userOp, userOpHash); | ||
if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(userOpValidationFunction))) | ||
{ | ||
revert UserOpValidationFunctionMissing(selector); | ||
} | ||
|
||
validationData = | ||
_doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[21:], userOpHash); | ||
} | ||
|
||
// To support gas estimation, we don't fail early when the failure is caused by a signature failure | ||
function _doUserOpValidation( | ||
bytes4 selector, | ||
FunctionReference userOpValidationFunction, | ||
PackedUserOperation calldata userOp, | ||
PackedUserOperation memory userOp, | ||
bytes calldata signature, | ||
bytes32 userOpHash | ||
) internal returns (uint256 validationData) { | ||
userOp.signature = signature; | ||
|
||
if (userOpValidationFunction.isEmpty()) { | ||
// If the validation function is empty, then the call cannot proceed. | ||
revert UserOpValidationFunctionMissing(selector); | ||
|
@@ -412,50 +458,40 @@ contract UpgradeableModularAccount is | |
} | ||
} | ||
|
||
function _doRuntimeValidationIfNotFromEP() internal { | ||
AccountStorage storage _storage = getAccountStorage(); | ||
|
||
if (_storage.selectorData[msg.sig].denyExecutionCount > 0) { | ||
revert AlwaysDenyRule(); | ||
} | ||
|
||
if (msg.sender == address(_ENTRY_POINT)) return; | ||
|
||
FunctionReference runtimeValidationFunction = _storage.selectorData[msg.sig].validation; | ||
function _doRuntimeValidation( | ||
FunctionReference runtimeValidationFunction, | ||
bytes calldata callData, | ||
bytes calldata authorizationData | ||
) internal { | ||
// run all preRuntimeValidation hooks | ||
EnumerableSet.Bytes32Set storage preRuntimeValidationHooks = | ||
getAccountStorage().selectorData[msg.sig].preValidationHooks; | ||
getAccountStorage().selectorData[bytes4(callData[:4])].preValidationHooks; | ||
|
||
uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); | ||
for (uint256 i = 0; i < preRuntimeValidationHooksLength; ++i) { | ||
bytes32 key = preRuntimeValidationHooks.at(i); | ||
FunctionReference preRuntimeValidationHook = toFunctionReference(key); | ||
|
||
(address plugin, uint8 functionId) = preRuntimeValidationHook.unpack(); | ||
(address hookPlugin, uint8 hookFunctionId) = preRuntimeValidationHook.unpack(); | ||
try IValidationHook(hookPlugin).preRuntimeValidationHook( | ||
hookFunctionId, msg.sender, msg.value, callData | ||
) | ||
// forgefmt: disable-start | ||
// solhint-disable-next-line no-empty-blocks | ||
try IValidationHook(plugin).preRuntimeValidationHook(functionId, msg.sender, msg.value, msg.data) {} | ||
catch (bytes memory revertReason) { | ||
revert PreRuntimeValidationHookFailed(plugin, functionId, revertReason); | ||
{} catch (bytes memory revertReason) { | ||
// forgefmt: disable-end | ||
revert PreRuntimeValidationHookFailed(hookPlugin, hookFunctionId, revertReason); | ||
} | ||
} | ||
|
||
// Identifier scope limiting | ||
{ | ||
if (_storage.selectorData[msg.sig].isPublic) { | ||
// If the function is public, we don't need to check the runtime validation function. | ||
return; | ||
} | ||
|
||
if (runtimeValidationFunction.isEmpty()) { | ||
revert RuntimeValidationFunctionMissing(msg.sig); | ||
} | ||
(address plugin, uint8 functionId) = runtimeValidationFunction.unpack(); | ||
|
||
(address plugin, uint8 functionId) = runtimeValidationFunction.unpack(); | ||
// solhint-disable-next-line no-empty-blocks | ||
try IValidation(plugin).validateRuntime(functionId, msg.sender, msg.value, msg.data) {} | ||
catch (bytes memory revertReason) { | ||
revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason); | ||
} | ||
try IValidation(plugin).validateRuntime(functionId, msg.sender, msg.value, callData, authorizationData) | ||
// forgefmt: disable-start | ||
// solhint-disable-next-line no-empty-blocks | ||
{} catch (bytes memory revertReason) { | ||
// forgefmt: disable-end | ||
revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason); | ||
} | ||
} | ||
|
||
|
@@ -536,4 +572,20 @@ contract UpgradeableModularAccount is | |
|
||
// solhint-disable-next-line no-empty-blocks | ||
function _authorizeUpgrade(address newImplementation) internal override {} | ||
|
||
function _checkPermittedCallerIfNotFromEP() internal view { | ||
AccountStorage storage _storage = getAccountStorage(); | ||
|
||
if (_storage.selectorData[msg.sig].denyExecutionCount > 0) { | ||
revert AlwaysDenyRule(); | ||
Comment on lines
+579
to
+580
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kinda want to revisit naming of AlwaysDenyRule and denyExecutionCount too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we can get rid of the magic values in #63, if that is finalized |
||
} | ||
if ( | ||
msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) | ||
|| _storage.selectorData[msg.sig].isPublic | ||
) return; | ||
|
||
if (!_storage.callPermitted[msg.sender][msg.sig]) { | ||
revert ExecFromPluginNotPermitted(msg.sender, msg.sig); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,4 +20,14 @@ interface IPluginExecutor { | |
external | ||
payable | ||
returns (bytes memory); | ||
|
||
/// @notice Execute a call using a specified runtime validation, as given in the first 21 bytes of | ||
/// `authorization`. | ||
/// @param data The calldata to send to the account. | ||
/// @param authorization The authorization data to use for the call. The first 21 bytes specifies which runtime | ||
/// validation to use, and the rest is sent as a parameter to runtime validation. | ||
function executeWithAuthorization(bytes calldata data, bytes calldata authorization) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also need to update https://github.com/erc6900/reference-implementation/blob/6d48a68021bff6f3a50a508873595af9fb1939a6/standard/ERCs/erc-6900.md in this PR or later on |
||
external | ||
payable | ||
returns (bytes memory); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we'd want to revisit this if we end up allowing multiple installations of the same plugin with different permissions? Still maybe makes sense to check for duplicates but I guess the key might include the permissions.
cc @howydev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation routing is done off-chain so definitely worth exploring moving execution selector routing off-chain too!
Re: session keys, i think the decision is between plugin + 1-key-per-plugin (or multiple keys that are managed by a dapp) v.s. 1 plugin + n keys per plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we definitely need some changes to support that workflow, but I wanted to keep each change scoped down to just what is needed per PR.
Also, I think having 1 key per plugin is too inefficient - we can treat the function id as a per-account, per-key item to perform that switching, like in the validator experiments.
"Key" is also the wrong term here, it's leaking the underlying validation logic into a higher layer of abstraction. They're really independent "validation functions" or "validators".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I might be missing something, but here's an example I was thinking about - in the case of adding a new session key to the existing MA session key plugin with
n
permissions, the minimal state change required will be:n
x nonzero to nonzeroSSTORE
s via increment the ERC20 + native token spend limits from the account to the pluginn
x zero to nonzeroSSTORE
s via incrementing the ERC20 + native token spend limits of the plugin to the session keyWhereas deploying a new validator would be just
n
x zero to nonzeroSSTORE
s via incrementing the ERC20 + native token spend limits of the account to the new validatorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know in the old v0.7 model, we basically needed two layers of permissions - account <> plugin, and in the case of a key-based validation plugin, then permissions for each key.
If we switch to composable validation (aka, allow validation plugins to be installed multiple times), then we go back to just needing 1 set of permissions per "validation", which is a combination of both the address of the validator plugin and the ID of the key within it.
I think the model of doing proxies on plugins per account is wrong - the cost of any contract creation is already too. high (32k), not to mention the added forwarding cost of being a proxy.