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 2 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 shared validation function may be used to validate this function.
bool allowSharedValidation;
adamegyed marked this conversation as resolved.
Show resolved Hide resolved
// 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
67 changes: 67 additions & 0 deletions src/account/PluginManager2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// 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";

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 shared,
bytes4[] memory selectors,
bytes calldata installData
) internal {
FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId);

AccountStorage storage _storage = getAccountStorage();

if (shared) {
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 allowSharedValidation, address plugin)
internal
notNullPlugin(plugin)
{
Expand All @@ -71,13 +71,15 @@ abstract contract PluginManagerInternals is IPluginManager {

_selectorData.plugin = plugin;
_selectorData.isPublic = isPublic;
_selectorData.allowSharedValidation = allowSharedValidation;
}

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

_selectorData.plugin = address(0);
_selectorData.isPublic = false;
_selectorData.allowSharedValidation = 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 allowSharedValidation = manifest.executionFunctions[i].allowSharedValidation;
_setExecutionFunction(selector, isPublic, allowSharedValidation, plugin);
}

// Add installed plugin and selectors this plugin can call
Expand Down
89 changes: 80 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 shared);

// Wraps execution of a native function with runtime validation and hooks
// Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin
Expand Down Expand Up @@ -279,11 +282,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 isSharedValidation = uint8(authorization[21]) == 1;
_checkIfValidationApplies(execSelector, runtimeValidationFunction, isSharedValidation);

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

// If runtime validation passes, execute the call

Expand Down Expand Up @@ -325,6 +329,40 @@ 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
function installValidation(
adamegyed marked this conversation as resolved.
Show resolved Hide resolved
address plugin,
uint8 functionId,
bool shared,
bytes4[] calldata selectors,
bytes calldata installData
) external wrapNativeFunction {
_installValidation(plugin, functionId, shared, selectors, installData);
}

/// @inheritdoc IPluginManager
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 Down Expand Up @@ -398,14 +436,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 isSharedValidation = uint8(userOp.signature[21]) == 1;

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

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 +609,41 @@ contract UpgradeableModularAccount is
// solhint-disable-next-line no-empty-blocks
function _authorizeUpgrade(address newImplementation) internal override {}

function _checkIfValidationApplies(bytes4 selector, FunctionReference validationFunction, bool shared)
huaweigu marked this conversation as resolved.
Show resolved Hide resolved
internal
view
{
AccountStorage storage _storage = getAccountStorage();

// Check that the provided validation function is applicable to the selector
if (shared) {
howydev marked this conversation as resolved.
Show resolved Hide resolved
if (
!_sharedValidationAllowed(selector)
|| !_storage.defaultValidations.contains(toSetValue(validationFunction))
) {
revert UserOpValidationFunctionMissing(selector);
}
} else {
// Not shared validation, but per-selector
if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(validationFunction))) {
revert UserOpValidationFunctionMissing(selector);
}
}
}

function _sharedValidationAllowed(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].allowSharedValidation;
}

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 shared validation function.
bool allowSharedValidation;
}

/// @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
34 changes: 33 additions & 1 deletion src/interfaces/IPluginManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,40 @@ interface IPluginManager {
FunctionReference[] calldata dependencies
) external;

/// @notice Temporary install function - pending a different user-supplied install config & manifest validation
/// path.
/// Installs a validation function across a set of execution selectors, and optionally mark it as a default
/// validation.
/// TODO: remove or update.
/// @dev This does not validate anything against the manifest - the caller must ensure validity.
/// @param plugin The plugin to install.
/// @param functionId The function ID of the validation function to install.
/// @param shared Whether the validation function is shared across all selectors in the default pool.
/// @param selectors The selectors to install the validation function for.
/// @param installData Optional data to be decoded and used by the plugin to setup initial plugin state.
function installValidation(
address plugin,
uint8 functionId,
bool shared,
bytes4[] calldata selectors,
bytes calldata installData
) external;

/// @notice Uninstall a validation function from a set of execution selectors.
/// TODO: remove or update.
/// @param plugin The plugin to uninstall.
/// @param functionId The function ID of the validation function to uninstall.
/// @param selectors The selectors to uninstall the validation function for.
/// @param uninstallData Optional data to be decoded and used by the plugin to clear plugin data for the
/// account.
function uninstallValidation(
address plugin,
uint8 functionId,
bytes4[] calldata selectors,
bytes calldata uninstallData
) external;

/// @notice Uninstall a plugin from the modular account.
/// @dev Uninstalling owner plugins outside of a replace operation via executeBatch risks losing the account!
/// @param plugin The plugin to uninstall.
/// @param config An optional, implementation-specific field that accounts may use to ensure consistency
/// guarantees.
Expand Down
21 changes: 15 additions & 6 deletions src/plugins/TokenReceiverPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,21 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC1155Receiver {
PluginManifest memory manifest;

manifest.executionFunctions = new ManifestExecutionFunction[](3);
manifest.executionFunctions[0] =
ManifestExecutionFunction({executionSelector: this.onERC721Received.selector, isPublic: true});
manifest.executionFunctions[1] =
ManifestExecutionFunction({executionSelector: this.onERC1155Received.selector, isPublic: true});
manifest.executionFunctions[2] =
ManifestExecutionFunction({executionSelector: this.onERC1155BatchReceived.selector, isPublic: true});
manifest.executionFunctions[0] = ManifestExecutionFunction({
executionSelector: this.onERC721Received.selector,
isPublic: true,
allowSharedValidation: false
});
manifest.executionFunctions[1] = ManifestExecutionFunction({
executionSelector: this.onERC1155Received.selector,
isPublic: true,
allowSharedValidation: false
});
manifest.executionFunctions[2] = ManifestExecutionFunction({
executionSelector: this.onERC1155BatchReceived.selector,
isPublic: true,
allowSharedValidation: false
});

manifest.interfaceIds = new bytes4[](2);
manifest.interfaceIds[0] = type(IERC721Receiver).interfaceId;
Expand Down
Loading
Loading