From 67f67b80948bf1b6039c3dd2facd79770cd8c3ff Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 17 Jun 2024 15:48:12 -0700 Subject: [PATCH] add various native function checks for plugin installation --- src/account/PluginManagerInternals.sol | 24 ++++++++++ src/helpers/KnownSelectors.sol | 63 ++++++++++++++++++++++++++ test/libraries/KnowSelectors.t.sol | 23 ++++++++++ 3 files changed, 110 insertions(+) create mode 100644 src/helpers/KnownSelectors.sol create mode 100644 test/libraries/KnowSelectors.t.sol diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index cfdbef16..1df1599b 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -23,6 +23,7 @@ import { toSetValue, PermittedExternalCallData } from "./AccountStorage.sol"; +import {KnownSelectors} from "../helpers/KnownSelectors.sol"; abstract contract PluginManagerInternals is IPluginManager { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -30,10 +31,13 @@ abstract contract PluginManagerInternals is IPluginManager { using FunctionReferenceLib for FunctionReference; error ArrayLengthMismatch(); + error Erc4337FunctionNotAllowed(bytes4 selector); error ExecutionFunctionAlreadySet(bytes4 selector); error InvalidDependenciesProvided(); error InvalidPluginManifest(); + error IPluginFunctionNotAllowed(bytes4 selector); error MissingPluginDependency(address dependency); + error NativeFunctionNotAllowed(bytes4 selector); error NullFunctionReference(); error NullPlugin(); error PluginAlreadyInstalled(address plugin); @@ -69,6 +73,26 @@ abstract contract PluginManagerInternals is IPluginManager { revert ExecutionFunctionAlreadySet(selector); } + // Make sure incoming execution function does not collide with any native functions (data are stored on the + // account implementation contract) + if (KnownSelectors.isNativeFunction(selector)) { + revert NativeFunctionNotAllowed(selector); + } + + // Make sure incoming execution function is not a function in IPlugin + if (KnownSelectors.isIPluginFunction(selector)) { + revert IPluginFunctionNotAllowed(selector); + } + + // Also make sure it doesn't collide with functions defined by ERC-4337 + // and called by the entry point. This prevents a malicious plugin from + // sneaking in a function with the same selector as e.g. + // `validatePaymasterUserOp` and turning the account into their own + // personal paymaster. + if (KnownSelectors.isErc4337Function(selector)) { + revert Erc4337FunctionNotAllowed(selector); + } + _selectorData.plugin = plugin; _selectorData.isPublic = isPublic; } diff --git a/src/helpers/KnownSelectors.sol b/src/helpers/KnownSelectors.sol new file mode 100644 index 00000000..6d737172 --- /dev/null +++ b/src/helpers/KnownSelectors.sol @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; +import {IAccount} from "@eth-infinitism/account-abstraction/interfaces/IAccount.sol"; +import {IAggregator} from "@eth-infinitism/account-abstraction/interfaces/IAggregator.sol"; +import {IPaymaster} from "@eth-infinitism/account-abstraction/interfaces/IPaymaster.sol"; + +import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; +import {IExecutionHook} from "../interfaces/IExecutionHook.sol"; +import {IPlugin} from "../interfaces/IPlugin.sol"; +import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; +import {IPluginManager} from "../interfaces/IPluginManager.sol"; +import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; +import {IValidation} from "../interfaces/IValidation.sol"; +import {IValidationHook} from "../interfaces/IValidationHook.sol"; + +/// @dev Library to help to check if a selector is a know function selector of the modular account or ERC-4337 +/// contract. +library KnownSelectors { + function isNativeFunction(bytes4 selector) internal pure returns (bool) { + return + // check against IAccount methods + selector == IAccount.validateUserOp.selector + // check against IPluginManager methods + || selector == IPluginManager.installPlugin.selector || selector == IPluginManager.uninstallPlugin.selector + // check against IERC165 methods + || selector == IERC165.supportsInterface.selector + // check against UUPSUpgradeable methods + || selector == UUPSUpgradeable.proxiableUUID.selector + || selector == UUPSUpgradeable.upgradeToAndCall.selector + // check against IStandardExecutor methods + || selector == IStandardExecutor.execute.selector || selector == IStandardExecutor.executeBatch.selector + // check against IPluginExecutor methods + || selector == IPluginExecutor.executeFromPlugin.selector + || selector == IPluginExecutor.executeFromPluginExternal.selector + || selector == IPluginExecutor.executeWithAuthorization.selector + // check against IAccountLoupe methods + || selector == IAccountLoupe.getExecutionFunctionHandler.selector + || selector == IAccountLoupe.getValidations.selector + || selector == IAccountLoupe.getExecutionHooks.selector + || selector == IAccountLoupe.getPreValidationHooks.selector + || selector == IAccountLoupe.getInstalledPlugins.selector; + } + + function isErc4337Function(bytes4 selector) internal pure returns (bool) { + return selector == IAggregator.validateSignatures.selector + || selector == IAggregator.validateUserOpSignature.selector + || selector == IAggregator.aggregateSignatures.selector + || selector == IPaymaster.validatePaymasterUserOp.selector || selector == IPaymaster.postOp.selector; + } + + function isIPluginFunction(bytes4 selector) internal pure returns (bool) { + return selector == IPlugin.onInstall.selector || selector == IPlugin.onUninstall.selector + || selector == IPlugin.pluginManifest.selector || selector == IPlugin.pluginMetadata.selector + || selector == IExecutionHook.preExecutionHook.selector + || selector == IExecutionHook.postExecutionHook.selector || selector == IValidation.validateUserOp.selector + || selector == IValidation.validateRuntime.selector || selector == IValidation.validateSignature.selector + || selector == IValidationHook.preUserOpValidationHook.selector + || selector == IValidationHook.preRuntimeValidationHook.selector; + } +} diff --git a/test/libraries/KnowSelectors.t.sol b/test/libraries/KnowSelectors.t.sol new file mode 100644 index 00000000..893b831b --- /dev/null +++ b/test/libraries/KnowSelectors.t.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {Test} from "forge-std/Test.sol"; +import {IAccount} from "@eth-infinitism/account-abstraction/interfaces/IAccount.sol"; +import {IPaymaster} from "@eth-infinitism/account-abstraction/interfaces/IPaymaster.sol"; + +import {KnownSelectors} from "../../src/helpers/KnownSelectors.sol"; +import {IPlugin} from "../../src/interfaces/IPlugin.sol"; + +contract KnownSelectorsTest is Test { + function test_isNativeFunction() public { + assertTrue(KnownSelectors.isNativeFunction(IAccount.validateUserOp.selector)); + } + + function test_isErc4337Function() public { + assertTrue(KnownSelectors.isErc4337Function(IPaymaster.validatePaymasterUserOp.selector)); + } + + function test_isIPluginFunction() public { + assertTrue(KnownSelectors.isIPluginFunction(IPlugin.pluginMetadata.selector)); + } +}