fix: remove explicit self-dependency check in favor of storing plugin…

… data later
alchemyplatform · Jan 4, 2024 · 4b06b9a · 4b06b9a
1 parent 25a87f8
commit 4b06b9a
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 30 deletions.
diff --git 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
@@ -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,