Skip to content

Commit

Permalink
feat: [v0.8-develop] remove execute from plugin (#65)
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamegyed
adamegyed authored Jun 20, 2024
1 parent 18261cb commit 400e833
Show file tree
Hide file tree
Showing 16 changed files with 178 additions and 748 deletions.
16 changes: 0 additions & 16 deletions src/account/AccountStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,19 @@ pragma solidity ^0.8.25;

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

import {IPlugin} from "../interfaces/IPlugin.sol";
import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference} from "../interfaces/IPluginManager.sol";

// bytes = keccak256("ERC6900.UpgradeableModularAccount.Storage")
bytes32 constant _ACCOUNT_STORAGE_SLOT = 0x9f09680beaa4e5c9f38841db2460c401499164f368baef687948c315d9073e40;

struct PluginData {
bool anyExternalExecPermitted;
// boolean to indicate if the plugin can spend native tokens from the account.
bool canSpendNativeToken;
bytes32 manifestHash;
FunctionReference[] dependencies;
// Tracks the number of times this plugin has been used as a dependency function
uint256 dependentCount;
}

// Represents data associated with a plugin's permission to use `executeFromPluginExternal`
// to interact with contracts and addresses external to the account and its plugins.
struct PermittedExternalCallData {
// Is this address on the permitted addresses list? If it is, we either have a
// list of allowed selectors, or the flag that allows any selector.
bool addressPermitted;
bool anySelectorPermitted;
mapping(bytes4 => bool) permittedSelectors;
}

// Represents data associated with a specifc function selector.
struct SelectorData {
// The plugin that implements this execution function.
Expand Down Expand Up @@ -67,8 +53,6 @@ struct AccountStorage {
mapping(bytes4 => SelectorData) selectorData;
mapping(FunctionReference validationFunction => ValidationData) validationData;
mapping(address caller => mapping(bytes4 selector => bool)) callPermitted;
// key = address(calling plugin) || target address
mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls;
// For ERC165 introspection
mapping(bytes4 => uint256) supportedIfaces;
}
Expand Down
67 changes: 1 addition & 66 deletions src/account/PluginManagerInternals.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,12 @@ import {
ManifestFunction,
ManifestAssociatedFunctionType,
ManifestAssociatedFunction,
ManifestExternalCallPermission,
PluginManifest
} from "../interfaces/IPlugin.sol";
import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
import {
AccountStorage,
getAccountStorage,
SelectorData,
toSetValue,
PermittedExternalCallData
} from "./AccountStorage.sol";
import {KnownSelectors} from "../helpers/KnownSelectors.sol";
import {AccountStorage, getAccountStorage, SelectorData, toSetValue} from "./AccountStorage.sol";

abstract contract PluginManagerInternals is IPluginManager {
using EnumerableSet for EnumerableSet.Bytes32Set;
Expand Down Expand Up @@ -211,11 +204,6 @@ abstract contract PluginManagerInternals is IPluginManager {

// Update components according to the manifest.

// Mark whether or not this plugin may spend native token amounts
if (manifest.canSpendNativeToken) {
_storage.pluginData[plugin].canSpendNativeToken = true;
}

length = manifest.executionFunctions.length;
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = manifest.executionFunctions[i].executionSelector;
Expand All @@ -232,31 +220,6 @@ abstract contract PluginManagerInternals is IPluginManager {
_storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true;
}

// Add the permitted external calls to the account.
if (manifest.permitAnyExternalAddress) {
_storage.pluginData[plugin].anyExternalExecPermitted = true;
} else {
// Only store the specific permitted external calls if "permit any" flag was not set.
length = manifest.permittedExternalCalls.length;
for (uint256 i = 0; i < length; ++i) {
ManifestExternalCallPermission memory externalCallPermission = manifest.permittedExternalCalls[i];

PermittedExternalCallData storage permittedExternalCallData =
_storage.permittedExternalCalls[IPlugin(plugin)][externalCallPermission.externalAddress];

permittedExternalCallData.addressPermitted = true;

if (externalCallPermission.permitAnySelector) {
permittedExternalCallData.anySelectorPermitted = true;
} else {
uint256 externalContractSelectorsLength = externalCallPermission.selectors.length;
for (uint256 j = 0; j < externalContractSelectorsLength; ++j) {
permittedExternalCallData.permittedSelectors[externalCallPermission.selectors[j]] = true;
}
}
}
}

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
ManifestAssociatedFunction memory mv = manifest.validationFunctions[i];
Expand Down Expand Up @@ -350,34 +313,6 @@ abstract contract PluginManagerInternals is IPluginManager {
);
}

// remove external call permissions

if (manifest.permitAnyExternalAddress) {
// Only clear if it was set during install time
_storage.pluginData[plugin].anyExternalExecPermitted = false;
} else {
// Only clear the specific permitted external calls if "permit any" flag was not set.
length = manifest.permittedExternalCalls.length;
for (uint256 i = 0; i < length; ++i) {
ManifestExternalCallPermission memory externalCallPermission = manifest.permittedExternalCalls[i];

PermittedExternalCallData storage permittedExternalCallData =
_storage.permittedExternalCalls[IPlugin(plugin)][externalCallPermission.externalAddress];

permittedExternalCallData.addressPermitted = false;

// Only clear this flag if it was set in the constructor.
if (externalCallPermission.permitAnySelector) {
permittedExternalCallData.anySelectorPermitted = false;
} else {
uint256 externalContractSelectorsLength = externalCallPermission.selectors.length;
for (uint256 j = 0; j < externalContractSelectorsLength; ++j) {
permittedExternalCallData.permittedSelectors[externalCallPermission.selectors[j]] = false;
}
}
}
}

length = manifest.permittedExecutionSelectors.length;
for (uint256 i = 0; i < length; ++i) {
_storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = false;
Expand Down
85 changes: 1 addition & 84 deletions src/account/UpgradeableModularAccount.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol";
import {IValidation} from "../interfaces/IValidation.sol";
import {IValidationHook} from "../interfaces/IValidationHook.sol";
import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol";
import {AccountExecutor} from "./AccountExecutor.sol";
Expand All @@ -39,7 +38,6 @@ contract UpgradeableModularAccount is
BaseAccount,
IERC165,
IERC1271,
IPluginExecutor,
IStandardExecutor,
PluginManagerInternals,
PluginManager2,
Expand Down Expand Up @@ -185,88 +183,7 @@ contract UpgradeableModularAccount is
}
}

/// @inheritdoc IPluginExecutor
function executeFromPlugin(bytes calldata data) external payable override returns (bytes memory) {
bytes4 selector = bytes4(data[:4]);
address callingPlugin = msg.sender;

AccountStorage storage _storage = getAccountStorage();

if (!_storage.callPermitted[callingPlugin][selector]) {
revert ExecFromPluginNotPermitted(callingPlugin, selector);
}

address execFunctionPlugin = _storage.selectorData[selector].plugin;

if (execFunctionPlugin == address(0)) {
revert UnrecognizedFunction(selector);
}

PostExecToRun[] memory postExecHooks = _doPreExecHooks(selector, data);

(bool success, bytes memory returnData) = execFunctionPlugin.call(data);

if (!success) {
assembly ("memory-safe") {
revert(add(returnData, 32), mload(returnData))
}
}

_doCachedPostExecHooks(postExecHooks);

return returnData;
}

/// @inheritdoc IPluginExecutor
function executeFromPluginExternal(address target, uint256 value, bytes calldata data)
external
payable
returns (bytes memory)
{
bytes4 selector = bytes4(data);
AccountStorage storage _storage = getAccountStorage();

// Make sure plugin is allowed to spend native token.
if (value > 0 && value > msg.value && !_storage.pluginData[msg.sender].canSpendNativeToken) {
revert NativeTokenSpendingNotPermitted(msg.sender);
}

// Check the caller plugin's permission to make this call

// Check the target contract permission.
// This first checks that the intended target is permitted at all. If it is, then it checks if any selector
// is permitted. If any selector is permitted, then it skips the selector-level permission check.
// If only a subset of selectors are permitted, then it also checks the selector-level permission.
// By checking in the order of [address specified with any selector allowed], [any address allowed],
// [address specified and selector specified], along with the extra bool `permittedCall`, we can
// reduce the number of `sload`s in the worst-case from 3 down to 2.
bool targetContractPermittedCall = _storage.permittedExternalCalls[IPlugin(msg.sender)][target]
.addressPermitted
&& (
_storage.permittedExternalCalls[IPlugin(msg.sender)][target].anySelectorPermitted
|| _storage.permittedExternalCalls[IPlugin(msg.sender)][target].permittedSelectors[selector]
);

// If the target contract is not permitted, check if the caller plugin is permitted to make any external
// calls.
if (!(targetContractPermittedCall || _storage.pluginData[msg.sender].anyExternalExecPermitted)) {
revert ExecFromPluginExternalNotPermitted(msg.sender, target, value, data);
}

// Run any pre exec hooks for this selector
PostExecToRun[] memory postExecHooks =
_doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.data);

// Perform the external call
bytes memory returnData = _exec(target, value, data);

// Run any post exec hooks for this selector
_doCachedPostExecHooks(postExecHooks);

return returnData;
}

/// @inheritdoc IPluginExecutor
/// @inheritdoc IStandardExecutor
function executeWithAuthorization(bytes calldata data, bytes calldata authorization)
external
payable
Expand Down
6 changes: 1 addition & 5 deletions src/helpers/KnownSelectors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {IPaymaster} from "@eth-infinitism/account-abstraction/interfaces/IPaymas
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";
Expand All @@ -32,10 +31,7 @@ library KnownSelectors {
|| 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
|| selector == IStandardExecutor.executeWithAuthorization.selector
// check against IAccountLoupe methods
|| selector == IAccountLoupe.getExecutionFunctionHandler.selector
|| selector == IAccountLoupe.getValidations.selector
Expand Down
12 changes: 0 additions & 12 deletions src/interfaces/IPlugin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ struct ManifestExecutionHook {
bool isPostHook;
}

struct ManifestExternalCallPermission {
address externalAddress;
bool permitAnySelector;
bytes4[] selectors;
}

struct SelectorPermission {
bytes4 functionSelector;
string permissionDescription;
Expand Down Expand Up @@ -81,12 +75,6 @@ struct PluginManifest {
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.
bool permitAnyExternalAddress;
// Boolean to indicate whether the plugin needs access to spend native tokens of the account. If false, the
// 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;
// List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
// IPlugin's interface ID.
bytes4[] interfaceIds;
Expand Down
33 changes: 0 additions & 33 deletions src/interfaces/IPluginExecutor.sol
View file

This file was deleted.

10 changes: 10 additions & 0 deletions src/interfaces/IStandardExecutor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,14 @@ interface IStandardExecutor {
/// @param calls The array of calls.
/// @return An array containing the return data from the calls.
function executeBatch(Call[] calldata calls) 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)
external
payable
returns (bytes memory);
}
10 changes: 5 additions & 5 deletions test/account/AccountExecHooks.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ contract AccountExecHooksTest is AccountTestBase {
uint8 internal constant _POST_HOOK_FUNCTION_ID_2 = 2;
uint8 internal constant _BOTH_HOOKS_FUNCTION_ID_3 = 3;

PluginManifest public m1;
PluginManifest public m2;
PluginManifest internal _m1;
PluginManifest internal _m2;

event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies);
event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded);
Expand All @@ -35,7 +35,7 @@ contract AccountExecHooksTest is AccountTestBase {
function setUp() public {
_transferOwnershipToTest();

m1.executionFunctions.push(
_m1.executionFunctions.push(
ManifestExecutionFunction({
executionSelector: _EXEC_SELECTOR,
isPublic: true,
Expand Down Expand Up @@ -163,8 +163,8 @@ contract AccountExecHooksTest is AccountTestBase {
}

function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal {
m1.executionHooks.push(execHooks);
mockPlugin1 = new MockPlugin(m1);
_m1.executionHooks.push(execHooks);
mockPlugin1 = new MockPlugin(_m1);
manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest()));

vm.expectEmit(true, true, true, true);
Expand Down
10 changes: 5 additions & 5 deletions test/account/AccountReturnData.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,16 @@ contract AccountReturnDataTest is AccountTestBase {
assertEq(result2, keccak256("foo"));
}

// Tests the ability to read data via executeFromPlugin routing to fallback functions
// Tests the ability to read data via routing to fallback functions
function test_returnData_execFromPlugin_fallback() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultEFPFallback(keccak256("bar"));
bool result = ResultConsumerPlugin(address(account1)).checkResultFallback(keccak256("bar"));

assertTrue(result);
}

// Tests the ability to read data via executeFromPluginExternal
function test_returnData_execFromPlugin_execute() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultEFPExternal(
// Tests the ability to read data via executeWithAuthorization
function test_returnData_authorized_exec() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultExecuteWithAuthorization(
address(regularResultContract), keccak256("bar")
);

Expand Down
Loading

0 comments on commit 400e833

Please sign in to comment.