Skip to content

Commit

Permalink
chore: remove existing permissions system
Browse files Browse the repository at this point in the history
  • Loading branch information
@howydev
howydev committed Jun 7, 2024
1 parent 72be4f4 commit e069b7a
Show file tree
Hide file tree
Showing 11 changed files with 252 additions and 272 deletions.
14 changes: 0 additions & 14 deletions src/account/AccountStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ 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";

Expand All @@ -20,16 +19,6 @@ struct PluginData {
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 @@ -66,9 +55,6 @@ struct AccountStorage {
// Execution functions and their associated functions
mapping(bytes4 => SelectorData) selectorData;
mapping(FunctionReference => 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
80 changes: 1 addition & 79 deletions src/account/PluginManagerInternals.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,11 @@ 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 {AccountStorage, getAccountStorage, SelectorData, toSetValue} from "./AccountStorage.sol";

abstract contract PluginManagerInternals is IPluginManager {
using EnumerableSet for EnumerableSet.Bytes32Set;
Expand Down Expand Up @@ -186,11 +179,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 @@ -199,39 +187,6 @@ abstract contract PluginManagerInternals is IPluginManager {
_setExecutionFunction(selector, isPublic, allowSharedValidation, plugin);
}

// Add installed plugin and selectors this plugin can call
length = manifest.permittedExecutionSelectors.length;
for (uint256 i = 0; i < length; ++i) {
// If there are duplicates, this will just enable the flag again. This is not a problem, since the
// boolean will be set to false twice during uninstall, which is fine.
_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 @@ -325,39 +280,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;
}

length = manifest.executionFunctions.length;
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = manifest.executionFunctions[i].executionSelector;
Expand Down
36 changes: 1 addition & 35 deletions src/account/UpgradeableModularAccount.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,9 @@ 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)) {
Expand Down Expand Up @@ -221,36 +216,11 @@ contract UpgradeableModularAccount is
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) {
if (value > 0 && value > msg.value) {
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);
Expand Down Expand Up @@ -632,9 +602,5 @@ contract UpgradeableModularAccount is
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);
}
}
}
14 changes: 0 additions & 14 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 All @@ -79,14 +73,6 @@ struct PluginManifest {
ManifestAssociatedFunction[] validationFunctions;
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.
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
4 changes: 2 additions & 2 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 Down
Loading

0 comments on commit e069b7a

Please sign in to comment.