diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 1bd8ab7c4..66f42b40c 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -359,11 +359,8 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { // Check the dependency interface id over the address of the dependency. (address dependencyAddr,) = dependencies[i].unpack(); - // Check that the dependency is installed. + // Check that the dependency is installed. This also blocks self-dependencies. if (storage_.pluginData[dependencyAddr].manifestHash == bytes32(0)) { - if (plugin == dependencyAddr) { - revert InvalidDependenciesProvided(); - } revert MissingPluginDependency(dependencyAddr); } @@ -380,17 +377,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } } - // Add the plugin metadata to the account - storage_.pluginData[plugin].manifestHash = manifestHash; - storage_.pluginData[plugin].dependencies = dependencies; - // Update components according to the manifest. - // All conflicts should revert. - - // Mark whether or not this plugin may spend native token amounts - if (manifest.canSpendNativeToken) { - storage_.pluginData[plugin].canSpendNativeToken = true; - } // Install execution functions length = manifest.executionFunctions.length; @@ -455,8 +442,9 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { for (uint256 i = 0; i < length;) { InjectedHook memory hook = injectedHooks[i]; - if (plugin == hook.providingPlugin) { - revert InvalidDependenciesProvided(); + // Check that the dependency is installed. This also blocks self-dependencies. + if (storage_.pluginData[hook.providingPlugin].manifestHash == bytes32(0)) { + revert MissingPluginDependency(hook.providingPlugin); } injectedHooksArray[i] = StoredInjectedHook({ @@ -470,10 +458,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { // Increment the dependent count for the plugin providing the hook. storage_.pluginData[hook.providingPlugin].dependentCount += 1; - if (!storage_.plugins.contains(CastLib.toSetValue(hook.providingPlugin))) { - revert MissingPluginDependency(hook.providingPlugin); - } - _addPermittedCallHooks( hook.selector, plugin, @@ -613,6 +597,15 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } } + // Add the plugin metadata to the account + storage_.pluginData[plugin].manifestHash = manifestHash; + storage_.pluginData[plugin].dependencies = dependencies; + + // Mark whether or not this plugin may spend native token amounts + if (manifest.canSpendNativeToken) { + storage_.pluginData[plugin].canSpendNativeToken = true; + } + // Call injected hooks' onHookApply after all setup, this is before calling plugin onInstall length = injectedHooks.length; for (uint256 i = 0; i < length;) { diff --git a/test/account/UpgradeableModularAccountPluginManager.t.sol b/test/account/UpgradeableModularAccountPluginManager.t.sol index e576512ec..28793de5e 100644 --- a/test/account/UpgradeableModularAccountPluginManager.t.sol +++ b/test/account/UpgradeableModularAccountPluginManager.t.sol @@ -241,18 +241,34 @@ contract UpgradeableModularAccountPluginManagerTest is Test { } function test_installPlugin_invalidDependency() external { + vm.startPrank(owner2); + manifest.dependencyInterfaceIds.push(type(IPlugin).interfaceId); MockPlugin newPlugin = new MockPlugin(manifest); - bytes32 manifestHash = keccak256(abi.encode(newPlugin.pluginManifest())); + // Add invalid function reference that points to the plugin being installed, rather than // an existing dependency. FunctionReference[] memory dependencies = new FunctionReference[](1); dependencies[0] = FunctionReferenceLib.pack(address(newPlugin), 0); + vm.expectRevert( + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(newPlugin)) + ); + IPluginManager(account2).installPlugin({ + plugin: address(newPlugin), + manifestHash: manifestHash, + pluginInitData: "", + dependencies: dependencies, + injectedHooks: new IPluginManager.InjectedHook[](0) + }); - vm.prank(owner2); - - vm.expectRevert(PluginManagerInternals.InvalidDependenciesProvided.selector); + // Add invalid function reference that points to a plugin that is not yet installed (and also is not the + // one currently being installed). + MockPlugin newPlugin2 = new MockPlugin(manifest); + dependencies[0] = FunctionReferenceLib.pack(address(newPlugin2), 0); + vm.expectRevert( + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(newPlugin2)) + ); IPluginManager(account2).installPlugin({ plugin: address(newPlugin), manifestHash: manifestHash, @@ -781,20 +797,37 @@ contract UpgradeableModularAccountPluginManagerTest is Test { }); } - function test_injectHooksInvalidDependency() external { + function test_injectHooksMissingDependency() external { + vm.startPrank(owner2); + MockPlugin newPlugin = new MockPlugin(manifest); bytes32 manifestHash = keccak256(abi.encode(newPlugin.pluginManifest())); - // Add invalid injected hook that points to the plugin being installed, rather than - // an existing dependency. + // Add invalid injected hook that points to the plugin being installed, rather than an existing dependency. IPluginManager.InjectedHook[] memory hooks = new IPluginManager.InjectedHook[](1); hooks[0] = IPluginManager.InjectedHook( address(newPlugin), IPluginExecutor.executeFromPluginExternal.selector, injectedHooksInfo, "" ); + vm.expectRevert( + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(newPlugin)) + ); + IPluginManager(account2).installPlugin({ + plugin: address(newPlugin), + manifestHash: manifestHash, + pluginInitData: "", + dependencies: new FunctionReference[](0), + injectedHooks: hooks + }); - vm.prank(owner2); - - vm.expectRevert(PluginManagerInternals.InvalidDependenciesProvided.selector); + // Add invalid injected hook that points to a plugin that is not yet installed (and also is not the one + // currently being installed). + MockPlugin newPlugin2 = new MockPlugin(manifest); + hooks[0] = IPluginManager.InjectedHook( + address(newPlugin2), IPluginExecutor.executeFromPluginExternal.selector, injectedHooksInfo, "" + ); + vm.expectRevert( + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(newPlugin2)) + ); IPluginManager(account2).installPlugin({ plugin: address(newPlugin), manifestHash: manifestHash,