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

…stallPlugin and upgradeToAndCall
alchemyplatform · Dec 18, 2023 · c9bb5f8 · c9bb5f8
1 parent 9d2fc29
commit c9bb5f8
Show file tree

Hide file tree

Showing 2 changed files with 32 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,36 @@ contract UpgradeableModularAccountPluginManagerTest is Test {
         IStandardExecutor(account2).executeBatch(calls);
     }
 
+    function test_uninstallAndInstallInBatch_failwithOtherCalls() external {
+        // Test fail case for a special use case in `installPlugin`:
+        // 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.
+        // Here we test only the above is allowed, any other self-call is blocked
+
+        vm.startPrank(owner2);
+
+        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