From db9960fd428cb5066e414d53aa59dbc9fe8e4086 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:49 -0400 Subject: [PATCH 1/2] chore: remove existing permissions system --- src/account/AccountStorage.sol | 3 +-- src/account/PluginManagerInternals.sol | 13 ------------- src/account/UpgradeableModularAccount.sol | 8 +++----- src/interfaces/IPlugin.sol | 2 -- test/account/UpgradeableModularAccount.t.sol | 4 +--- test/mocks/plugins/PermittedCallMocks.sol | 4 ---- test/mocks/plugins/ReturnDataPluginMocks.sol | 3 --- 7 files changed, 5 insertions(+), 32 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0a992a33..5a3b8c12 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,8 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference validationFunction => ValidationData) validationData; - mapping(address caller => mapping(bytes4 selector => bool)) callPermitted; + mapping(FunctionReference => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 2ec06f81..05943e09 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -212,14 +212,6 @@ abstract contract PluginManagerInternals is IPluginManager { _setExecutionFunction(selector, isPublic, allowDefaultValidation, 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; - } - length = manifest.validationFunctions.length; for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; @@ -313,11 +305,6 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - 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; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 169b17a3..0c67ac81 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -555,11 +555,9 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); if ( - msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) - || _storage.selectorData[msg.sig].isPublic - ) return; - - if (!_storage.callPermitted[msg.sender][msg.sig]) { + msg.sender != address(_ENTRY_POINT) && msg.sender != address(this) + && !_storage.selectorData[msg.sig].isPublic + ) { revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } } diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 3241e497..22d74546 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -73,8 +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; // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. bytes4[] interfaceIds; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e3d09e7f..46ea1d5f 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -11,7 +11,7 @@ import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; -import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; +import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -262,8 +262,6 @@ contract UpgradeableModularAccountTest is AccountTestBase { vm.startPrank(address(entryPoint)); PluginManifest memory m; - m.permittedExecutionSelectors = new bytes4[](1); - m.permittedExecutionSelectors[0] = IPlugin.onInstall.selector; MockPlugin mockPluginWithBadPermittedExec = new MockPlugin(m); bytes32 manifestHash = keccak256(abi.encode(mockPluginWithBadPermittedExec.pluginManifest())); diff --git a/test/mocks/plugins/PermittedCallMocks.sol b/test/mocks/plugins/PermittedCallMocks.sol index 450a70fb..77548225 100644 --- a/test/mocks/plugins/PermittedCallMocks.sol +++ b/test/mocks/plugins/PermittedCallMocks.sol @@ -22,10 +22,6 @@ contract PermittedCallerPlugin is BasePlugin { manifest.executionFunctions[i].isPublic = true; } - // Request permission only for "foo", but not "bar", from ResultCreatorPlugin - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index dae2c8e4..485b8456 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -138,9 +138,6 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { allowDefaultValidation: false }); - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } From 538375b5bb78d75a87a7b8f2b39a897f7890240a Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:05:19 -0400 Subject: [PATCH 2/2] fix: revert change --- src/account/AccountStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5a3b8c12..5d08ab6d 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,7 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference => ValidationData) validationData; + mapping(FunctionReference validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; }