fix: [spearbit-94] runtime validation bypass special to only allow in…

…stallPlugin and upgradeToAndCall
alchemyplatform · Dec 15, 2023 · 54b41fc · 54b41fc
1 parent 9d2fc29
commit 54b41fc
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 2 deletions.
diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol
@@ -619,8 +619,8 @@ contract UpgradeableModularAccount is
                         && (
                             (
                                 msg.sig != IPluginManager.installPlugin.selector
-                                    || msg.sig != UUPSUpgradeable.upgradeToAndCall.selector
-                            ) && msg.sender != address(this)
+                                    && msg.sig != UUPSUpgradeable.upgradeToAndCall.selector
+                            ) || msg.sender != address(this)
                         )
                 ) {
                     // Runtime calls cannot be made against functions with no

diff --git a/test/account/UpgradeableModularAccountPluginManager.t.sol b/test/account/UpgradeableModularAccountPluginManager.t.sol
@@ -858,6 +858,38 @@ contract UpgradeableModularAccountPluginManagerTest is Test {
         IStandardExecutor(account2).executeBatch(calls);
     }
 
+    function test_uninstallAndInstallInBatch_failwithOtherCalls() external {
+        // Check that we can uninstall the `MultiOwnerPlugin`, leaving no
+        // validator on `installPlugin`, and then install a different plugin
+        // immediately after as part of the same batch execution. This is a
+        // special case: normally an execution function with no runtime
+        // validator cannot be runtime-called.
+        vm.startPrank(owner2);
+
+        ComprehensivePlugin plugin = new ComprehensivePlugin();
+        bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest()));
+
+        Call[] memory calls = new Call[](2);
+        calls[0] = Call({
+            target: address(account2),
+            value: 0,
+            data: abi.encodeCall(IPluginManager.uninstallPlugin, (address(multiOwnerPlugin), "", "", new bytes[](0)))
+        });
+        calls[1] = Call({
+            target: address(account2),
+            value: 0,
+            data: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, ""))
+        });
+
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                UpgradeableModularAccount.RuntimeValidationFunctionMissing.selector,
+                UpgradeableModularAccount.execute.selector
+            )
+        );
+        IStandardExecutor(account2).executeBatch(calls);
+    }
+
     function test_noNonSelfInstallAfterUninstall() external {
         // A companion to the previous test, ensuring that `installPlugin` can't
         // be called directly (e.g. not via `execute` or `executeBatch`) if it