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] default validation #63

Merged
merged 4 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion foundry.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[profile.default]
solc = '0.8.25'
solc = '0.8.26'
via_ir = false
src = 'src'
test = 'test'
Expand Down
4 changes: 4 additions & 0 deletions src/account/AccountStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ struct SelectorData {
// Note that even if this is set to true, user op validation will still be required, otherwise anyone could
// drain the account of native tokens by wasting gas.
bool isPublic;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field being true also indicates the function is not state-change, right? Might be worth adding it to the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, it can still be state-changing, it's just that usually this will only be used for view functions. See this PR comment for more context: #61

// Whether or not a default validation function may be used to validate this function.
bool allowDefaultValidation;
// How many times a `PRE_HOOK_ALWAYS_DENY` has been added for this function.
// Since that is the only type of hook that may overlap, we can use this to track the number of times it has
// been applied, and whether or not the deny should apply. The size `uint48` was chosen somewhat arbitrarily,
Expand Down Expand Up @@ -68,6 +70,8 @@ struct AccountStorage {
mapping(bytes4 => uint256) supportedIfaces;
// Installed plugins capable of signature validation.
EnumerableSet.Bytes32Set signatureValidations;
// Todo: merge this with other validation storage?
EnumerableSet.Bytes32Set defaultValidations;
howydev marked this conversation as resolved.
Show resolved Hide resolved
fangting-alchemy marked this conversation as resolved.
Show resolved Hide resolved
}

// TODO: Change how pre-validation hooks work to allow association with validation, rather than selector.
Expand Down
68 changes: 68 additions & 0 deletions src/account/PluginManager2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {IPlugin} from "../interfaces/IPlugin.sol";
import {FunctionReference} from "../interfaces/IPluginManager.sol";
import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol";
import {AccountStorage, getAccountStorage, toSetValue} from "./AccountStorage.sol";

// Temporary additional functions for a user-controlled install flow for validation functions.
abstract contract PluginManager2 {
huaweigu marked this conversation as resolved.
Show resolved Hide resolved
using EnumerableSet for EnumerableSet.Bytes32Set;

error DefaultValidationAlreadySet(address plugin, uint8 functionId);
error ValidationAlreadySet(bytes4 selector, address plugin, uint8 functionId);
error ValidationNotSet(bytes4 selector, address plugin, uint8 functionId);

function _installValidation(
address plugin,
huaweigu marked this conversation as resolved.
Show resolved Hide resolved
uint8 functionId,
bool isDefault,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? Is the goal to separate validation installation from other plugin installations?

Would it be easier to only separate default validation installation from all other plugin installations (include non default validation installations).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not final interfaces, they're side doors to the install path because we haven't fully implemented user-supplied install configs yet. More detail in IPluginManager: https://github.com/erc6900/reference-implementation/blob/adam/default-validation/src/interfaces/IPluginManager.sol

bytes4[] memory selectors,
bytes calldata installData
) internal {
FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId);

AccountStorage storage _storage = getAccountStorage();

if (isDefault) {
if (!_storage.defaultValidations.add(toSetValue(validationFunction))) {
revert DefaultValidationAlreadySet(plugin, functionId);
}
}

for (uint256 i = 0; i < selectors.length; ++i) {
bytes4 selector = selectors[i];
if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) {
revert ValidationAlreadySet(selector, plugin, functionId);
}
}

IPlugin(plugin).onInstall(installData);
}

function _uninstallValidation(
howydev marked this conversation as resolved.
Show resolved Hide resolved
address plugin,
uint8 functionId,
bytes4[] calldata selectors,
bytes calldata uninstallData
) internal {
FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId);

AccountStorage storage _storage = getAccountStorage();

// Ignore return value - remove if present, do nothing otherwise.
_storage.defaultValidations.remove(toSetValue(validationFunction));
adamegyed marked this conversation as resolved.
Show resolved Hide resolved

for (uint256 i = 0; i < selectors.length; ++i) {
bytes4 selector = selectors[i];
if (!_storage.selectorData[selector].validations.remove(toSetValue(validationFunction))) {
revert ValidationNotSet(selector, plugin, functionId);
}
}

IPlugin(plugin).onUninstall(uninstallData);
}
}
7 changes: 5 additions & 2 deletions src/account/PluginManagerInternals.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ abstract contract PluginManagerInternals is IPluginManager {

// Storage update operations

function _setExecutionFunction(bytes4 selector, bool isPublic, address plugin)
function _setExecutionFunction(bytes4 selector, bool isPublic, bool allowDefaultValidation, address plugin)
internal
notNullPlugin(plugin)
{
Expand All @@ -71,13 +71,15 @@ abstract contract PluginManagerInternals is IPluginManager {

_selectorData.plugin = plugin;
_selectorData.isPublic = isPublic;
_selectorData.allowDefaultValidation = allowDefaultValidation;
}

function _removeExecutionFunction(bytes4 selector) internal {
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

_selectorData.plugin = address(0);
_selectorData.isPublic = false;
_selectorData.allowDefaultValidation = false;
}

function _addValidationFunction(bytes4 selector, FunctionReference validationFunction)
Expand Down Expand Up @@ -221,7 +223,8 @@ abstract contract PluginManagerInternals is IPluginManager {
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = manifest.executionFunctions[i].executionSelector;
bool isPublic = manifest.executionFunctions[i].isPublic;
_setExecutionFunction(selector, isPublic, plugin);
bool allowDefaultValidation = manifest.executionFunctions[i].allowDefaultValidation;
_setExecutionFunction(selector, isPublic, allowDefaultValidation, plugin);
}

// Add installed plugin and selectors this plugin can call
Expand Down
96 changes: 87 additions & 9 deletions src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
} from "./AccountStorage.sol";
import {AccountStorageInitializable} from "./AccountStorageInitializable.sol";
import {PluginManagerInternals} from "./PluginManagerInternals.sol";
import {PluginManager2} from "./PluginManager2.sol";

contract UpgradeableModularAccount is
AccountExecutor,
Expand All @@ -41,6 +42,7 @@ contract UpgradeableModularAccount is
IPluginExecutor,
IStandardExecutor,
PluginManagerInternals,
PluginManager2,
UUPSUpgradeable
{
using EnumerableSet for EnumerableSet.Bytes32Set;
Expand Down Expand Up @@ -77,6 +79,7 @@ contract UpgradeableModularAccount is
error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator);
error UnrecognizedFunction(bytes4 selector);
error UserOpValidationFunctionMissing(bytes4 selector);
error ValidationDoesNotApply(bytes4 selector, address plugin, uint8 functionId, bool isDefault);

// Wraps execution of a native function with runtime validation and hooks
// Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin
Expand Down Expand Up @@ -155,6 +158,7 @@ contract UpgradeableModularAccount is
}

/// @inheritdoc IStandardExecutor
/// @notice May be validated by a default validation.
function execute(address target, uint256 value, bytes calldata data)
external
payable
Expand All @@ -166,6 +170,7 @@ contract UpgradeableModularAccount is
}

/// @inheritdoc IStandardExecutor
/// @notice May be validated by a default validation function.
function executeBatch(Call[] calldata calls)
external
payable
Expand Down Expand Up @@ -279,11 +284,12 @@ contract UpgradeableModularAccount is
if (_storage.selectorData[execSelector].denyExecutionCount > 0) {
revert AlwaysDenyRule();
}
if (!_storage.selectorData[execSelector].validations.contains(toSetValue(runtimeValidationFunction))) {
revert RuntimeValidationFunctionMissing(execSelector);
}

_doRuntimeValidation(runtimeValidationFunction, data, authorization[21:]);
// Check if the runtime validation function is allowed to be called
bool isDefaultValidation = uint8(authorization[21]) == 1;
_checkIfValidationApplies(execSelector, runtimeValidationFunction, isDefaultValidation);

_doRuntimeValidation(runtimeValidationFunction, data, authorization[22:]);

// If runtime validation passes, execute the call

Expand All @@ -299,6 +305,7 @@ contract UpgradeableModularAccount is
}

/// @inheritdoc IPluginManager
/// @notice May be validated by a default validation.
function installPlugin(
address plugin,
bytes32 manifestHash,
Expand All @@ -309,6 +316,7 @@ contract UpgradeableModularAccount is
}

/// @inheritdoc IPluginManager
/// @notice May be validated by a default validation.
function uninstallPlugin(address plugin, bytes calldata config, bytes calldata pluginUninstallData)
external
override
Expand All @@ -325,6 +333,42 @@ contract UpgradeableModularAccount is
_uninstallPlugin(plugin, manifest, pluginUninstallData);
}

/// @notice Initializes the account with a validation function added to the default pool.
/// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall workflow
/// with user install configs.
/// @dev This function is only callable once, and only by the EntryPoint.

function initializeDefaultValidation(address plugin, uint8 functionId, bytes calldata installData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

external
initializer
{
_installValidation(plugin, functionId, true, new bytes4[](0), installData);
emit ModularAccountInitialized(_ENTRY_POINT);
}

/// @inheritdoc IPluginManager
/// @notice May be validated by a default validation.
function installValidation(
adamegyed marked this conversation as resolved.
Show resolved Hide resolved
address plugin,
uint8 functionId,
bool isDefault,
bytes4[] calldata selectors,
bytes calldata installData
) external wrapNativeFunction {
_installValidation(plugin, functionId, isDefault, selectors, installData);
}

/// @inheritdoc IPluginManager
/// @notice May be validated by a default validation.
function uninstallValidation(
address plugin,
uint8 functionId,
bytes4[] calldata selectors,
bytes calldata uninstallData
) external wrapNativeFunction {
_uninstallValidation(plugin, functionId, selectors, uninstallData);
}

/// @notice ERC165 introspection
/// @dev returns true for `IERC165.interfaceId` and false for `0xFFFFFFFF`
/// @param interfaceId interface id to check against
Expand All @@ -341,6 +385,7 @@ contract UpgradeableModularAccount is
}

/// @inheritdoc UUPSUpgradeable
/// @notice May be validated by a default validation.
function upgradeToAndCall(address newImplementation, bytes memory data)
public
payable
Expand Down Expand Up @@ -398,14 +443,12 @@ contract UpgradeableModularAccount is

// Revert if the provided `authorization` less than 21 bytes long, rather than right-padding.
FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21]));
bool isDefaultValidation = uint8(userOp.signature[21]) == 1;

if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(userOpValidationFunction)))
{
revert UserOpValidationFunctionMissing(selector);
}
_checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation);

validationData =
_doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[21:], userOpHash);
_doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[22:], userOpHash);
}

// To support gas estimation, we don't fail early when the failure is caused by a signature failure
Expand Down Expand Up @@ -573,6 +616,41 @@ contract UpgradeableModularAccount is
// solhint-disable-next-line no-empty-blocks
function _authorizeUpgrade(address newImplementation) internal override {}

function _checkIfValidationApplies(bytes4 selector, FunctionReference validationFunction, bool isDefault)
internal
view
{
AccountStorage storage _storage = getAccountStorage();

// Check that the provided validation function is applicable to the selector
if (isDefault) {
if (
!_defaultValidationAllowed(selector)
|| !_storage.defaultValidations.contains(toSetValue(validationFunction))
) {
revert UserOpValidationFunctionMissing(selector);
}
} else {
// Not default validation, but per-selector
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, during the installation, it seems like we also install the default validation

for (uint256 i = 0; i < selectors.length; ++i) {
            bytes4 selector = selectors[i];
            if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) {
                revert ValidationAlreadySet(selector, plugin, functionId);
            }
        }

should we check getAccountStorage().selectorData[selector].validations.contains() for both default and non-default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That path in the installation is used if the user wishes to install a validation both as a default validation and for some specific selectors. If the user manually adds selectors that are already usable from default validation (e.g. execute), then the cost savings of default validation go away. Similarly, checking the per-selector storage for the default validation increases costs due to the storage reads without providing a new form of checking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, that makes sense!

if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(validationFunction))) {
revert UserOpValidationFunctionMissing(selector);
}
}
}

function _defaultValidationAllowed(bytes4 selector) internal view returns (bool) {
if (
adamegyed marked this conversation as resolved.
Show resolved Hide resolved
selector == this.execute.selector || selector == this.executeBatch.selector
|| selector == this.installPlugin.selector || selector == this.uninstallPlugin.selector
|| selector == this.installValidation.selector || selector == this.uninstallValidation.selector
|| selector == this.upgradeToAndCall.selector
) {
return true;
}

return getAccountStorage().selectorData[selector].allowDefaultValidation;
}

function _checkPermittedCallerIfNotFromEP() internal view {
AccountStorage storage _storage = getAccountStorage();

Expand Down
24 changes: 13 additions & 11 deletions src/interfaces/IPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ struct ManifestExecutionFunction {
bytes4 executionSelector;
// If true, the function won't need runtime validation, and can be called by anyone.
bool isPublic;
// If true, the function can be validated by a default validation function.
bool allowDefaultValidation;
}

/// @dev For functions of type `ManifestAssociatedFunctionType.DEPENDENCY`, the MSCA MUST find the plugin address
Expand Down Expand Up @@ -77,15 +79,12 @@ struct PluginMetadata {

/// @dev A struct describing how the plugin should be installed on a modular account.
struct PluginManifest {
// List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
// IPlugin's interface ID.
bytes4[] interfaceIds;
// If this plugin depends on other plugins' validation functions, the interface IDs of those plugins MUST be
// provided here, with its position in the array matching the `dependencyIndex` members of `ManifestFunction`
// structs used in the manifest.
bytes4[] dependencyInterfaceIds;
// Execution functions defined in this plugin to be installed on the MSCA.
ManifestExecutionFunction[] executionFunctions;
ManifestAssociatedFunction[] validationFunctions;
ManifestAssociatedFunction[] preValidationHooks;
ManifestExecutionHook[] executionHooks;
uint8[] signatureValidationFunctions;
// Plugin execution functions already installed on the MSCA that this plugin will be able to call.
bytes4[] permittedExecutionSelectors;
// Boolean to indicate whether the plugin can call any external address.
Expand All @@ -94,10 +93,13 @@ struct PluginManifest {
// plugin MUST still be able to spend up to the balance that it sends to the account in the same call.
bool canSpendNativeToken;
ManifestExternalCallPermission[] permittedExternalCalls;
ManifestAssociatedFunction[] validationFunctions;
ManifestAssociatedFunction[] preValidationHooks;
ManifestExecutionHook[] executionHooks;
uint8[] signatureValidationFunctions;
// List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
// IPlugin's interface ID.
bytes4[] interfaceIds;
// If this plugin depends on other plugins' validation functions, the interface IDs of those plugins MUST be
// provided here, with its position in the array matching the `dependencyIndex` members of `ManifestFunction`
// structs used in the manifest.
bytes4[] dependencyInterfaceIds;
}

interface IPlugin is IERC165 {
Expand Down
Loading
Loading