move runtime validation always allow to execution function definition

src/account/AccountStorage.sol

@@ -35,6 +35,10 @@ struct SelectorData {
     // The plugin that implements this execution function.
     // If this is a native function, the address must remain address(0).
     address plugin;
+    // Whether or not the function needs runtime validation, or can be called by anyone.
+    // Note that even if this is set to true, user op validation will still be required, otherwise anyone could
+    // drain the account of native tokens by wasting gas.
+    bool isPublic;
     // How many times a `PRE_HOOK_ALWAYS_DENY` has been added for this function.
     // Since that is the only type of hook that may overlap, we can use this to track the number of times it has
     // been applied, and whether or not the deny should apply. The size `uint48` was chosen somewhat arbitrarily,

src/account/PluginManagerInternals.sol

@@ -59,20 +59,25 @@ abstract contract PluginManagerInternals is IPluginManager {
 
     // Storage update operations
 
-    function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(plugin) {
+    function _setExecutionFunction(bytes4 selector, bool isPublic, address plugin)
+        internal
+        notNullPlugin(plugin)
+    {
         SelectorData storage _selectorData = getAccountStorage().selectorData[selector];
 
         if (_selectorData.plugin != address(0)) {
             revert ExecutionFunctionAlreadySet(selector);
         }
 
         _selectorData.plugin = plugin;
+        _selectorData.isPublic = isPublic;
     }
 
     function _removeExecutionFunction(bytes4 selector) internal {
         SelectorData storage _selectorData = getAccountStorage().selectorData[selector];
 
         _selectorData.plugin = address(0);
+        _selectorData.isPublic = false;
     }
 
     function _addValidationFunction(bytes4 selector, FunctionReference validationFunction)
@@ -211,7 +216,9 @@ abstract contract PluginManagerInternals is IPluginManager {
 
         length = manifest.executionFunctions.length;
         for (uint256 i = 0; i < length; ++i) {
-            _setExecutionFunction(manifest.executionFunctions[i], plugin);
+            bytes4 selector = manifest.executionFunctions[i].executionSelector;
+            bool isPublic = manifest.executionFunctions[i].isPublic;
+            _setExecutionFunction(selector, isPublic, plugin);
         }
 
         // Add installed plugin and selectors this plugin can call
@@ -253,10 +260,7 @@ abstract contract PluginManagerInternals is IPluginManager {
             _addValidationFunction(
                 mv.executionSelector,
                 _resolveManifestFunction(
-                    mv.associatedFunction,
-                    plugin,
-                    dependencies,
-                    ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW
+                    mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE
                 )
             );
         }
@@ -378,10 +382,7 @@ abstract contract PluginManagerInternals is IPluginManager {
             _removeValidationFunction(
                 mv.executionSelector,
                 _resolveManifestFunction(
-                    mv.associatedFunction,
-                    plugin,
-                    dependencies,
-                    ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW
+                    mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE
                 )
             );
         }
@@ -421,7 +422,8 @@ abstract contract PluginManagerInternals is IPluginManager {
 
         length = manifest.executionFunctions.length;
         for (uint256 i = 0; i < length; ++i) {
-            _removeExecutionFunction(manifest.executionFunctions[i]);
+            bytes4 selector = manifest.executionFunctions[i].executionSelector;
+            _removeExecutionFunction(selector);
         }
 
         length = manifest.interfaceIds.length;
@@ -465,13 +467,6 @@ abstract contract PluginManagerInternals is IPluginManager {
                 revert InvalidPluginManifest();
             }
             return dependencies[manifestFunction.dependencyIndex];
-        } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW)
-        {
-            if (allowedMagicValue == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) {
-                return FunctionReferenceLib._RUNTIME_VALIDATION_ALWAYS_ALLOW;
-            } else {
-                revert InvalidPluginManifest();
-            }
         } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) {
             if (allowedMagicValue == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) {
                 return FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY;

src/account/UpgradeableModularAccount.sol

@@ -372,10 +372,8 @@ contract UpgradeableModularAccount is
         PackedUserOperation calldata userOp,
         bytes32 userOpHash
     ) internal returns (uint256 validationData) {
-        if (userOpValidationFunction.isEmptyOrMagicValue()) {
+        if (userOpValidationFunction.isEmpty()) {
             // If the validation function is empty, then the call cannot proceed.
-            // Alternatively, the validation function may be set to the RUNTIME_VALIDATION_ALWAYS_ALLOW magic
-            // value, in which case we also revert.
             revert UserOpValidationFunctionMissing(selector);
         }
 
@@ -443,18 +441,20 @@ contract UpgradeableModularAccount is
 
         // Identifier scope limiting
         {
-            if (!runtimeValidationFunction.isEmptyOrMagicValue()) {
-                (address plugin, uint8 functionId) = runtimeValidationFunction.unpack();
-                // solhint-disable-next-line no-empty-blocks
-                try IValidation(plugin).validateRuntime(functionId, msg.sender, msg.value, msg.data) {}
-                catch (bytes memory revertReason) {
-                    revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason);
-                }
-            } else {
-                if (runtimeValidationFunction.isEmpty()) {
-                    revert RuntimeValidationFunctionMissing(msg.sig);
-                }
-                // If _RUNTIME_VALIDATION_ALWAYS_ALLOW, just let the function finish.
+            if (_storage.selectorData[msg.sig].isPublic) {
+                // If the function is public, we don't need to check the runtime validation function.
+                return;
+            }
+
+            if (runtimeValidationFunction.isEmpty()) {
+                revert RuntimeValidationFunctionMissing(msg.sig);
+            }
+
+            (address plugin, uint8 functionId) = runtimeValidationFunction.unpack();
+            // solhint-disable-next-line no-empty-blocks
+            try IValidation(plugin).validateRuntime(functionId, msg.sender, msg.value, msg.data) {}
+            catch (bytes memory revertReason) {
+                revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason);
             }
         }
     }

src/helpers/FunctionReferenceLib.sol

@@ -6,9 +6,6 @@ import {FunctionReference} from "../interfaces/IPluginManager.sol";
 library FunctionReferenceLib {
     // Empty or unset function reference.
     FunctionReference internal constant _EMPTY_FUNCTION_REFERENCE = FunctionReference.wrap(bytes21(0));
-    // Magic value for runtime validation functions that always allow access.
-    FunctionReference internal constant _RUNTIME_VALIDATION_ALWAYS_ALLOW =
-        FunctionReference.wrap(bytes21(uint168(1)));
     // Magic value for hooks that should always revert.
     FunctionReference internal constant _PRE_HOOK_ALWAYS_DENY = FunctionReference.wrap(bytes21(uint168(2)));
 
@@ -30,10 +27,6 @@ library FunctionReferenceLib {
         return FunctionReference.unwrap(fr) != bytes21(0);
     }
 
-    function isEmptyOrMagicValue(FunctionReference fr) internal pure returns (bool) {
-        return FunctionReference.unwrap(fr) <= bytes21(uint168(2));
-    }
-
     function eq(FunctionReference a, FunctionReference b) internal pure returns (bool) {
         return FunctionReference.unwrap(a) == FunctionReference.unwrap(b);
     }

src/interfaces/IPlugin.sol

@@ -13,11 +13,6 @@ enum ManifestAssociatedFunctionType {
     SELF,
     // Function belongs to an external plugin provided as a dependency during plugin installation.
     DEPENDENCY,
-    // Resolves to a magic value to always bypass runtime validation for a given function.
-    // This is only assignable on runtime validation functions. If it were to be used on a user op validation function,
-    // it would risk burning gas from the account. When used as a hook in any hook location, it is equivalent to not
-    // setting a hook and is therefore disallowed.
-    RUNTIME_VALIDATION_ALWAYS_ALLOW,
     // Resolves to a magic value to always fail in a hook for a given function.
     // This is only assignable to pre execution hooks. It should not be used on validation functions themselves, because
     // this is equivalent to leaving the validation functions unset. It should not be used in post-exec hooks, because
@@ -26,6 +21,14 @@ enum ManifestAssociatedFunctionType {
 }
 // forgefmt: disable-end
 
+struct ManifestExecutionFunction {
+    // TODO(erc6900 spec): These fields can be packed into a single word
+    // The selector to install
+    bytes4 executionSelector;
+    // If true, the function won't need runtime validaiton, and can be called by anyone.
+    bool isPublic;
+}
+
 /// @dev For functions of type `ManifestAssociatedFunctionType.DEPENDENCY`, the MSCA MUST find the plugin address
 /// of the function at `dependencies[dependencyIndex]` during the call to `installPlugin(config)`.
 struct ManifestFunction {
@@ -82,7 +85,7 @@ struct PluginManifest {
     // structs used in the manifest.
     bytes4[] dependencyInterfaceIds;
     // Execution functions defined in this plugin to be installed on the MSCA.
-    bytes4[] executionFunctions;
+    ManifestExecutionFunction[] executionFunctions;
     // Plugin execution functions already installed on the MSCA that this plugin will be able to call.
     bytes4[] permittedExecutionSelectors;
     // Boolean to indicate whether the plugin can call any external address.

src/plugins/TokenReceiverPlugin.sol

@@ -4,14 +4,7 @@ pragma solidity ^0.8.25;
 import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
 import {IERC1155Receiver} from "@openzeppelin/contracts/interfaces/IERC1155Receiver.sol";
 
-import {
-    IPlugin,
-    ManifestFunction,
-    ManifestAssociatedFunctionType,
-    ManifestAssociatedFunction,
-    PluginManifest,
-    PluginMetadata
-} from "../interfaces/IPlugin.sol";
+import {IPlugin, ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../interfaces/IPlugin.sol";
 import {BasePlugin} from "./BasePlugin.sol";
 
 /// @title Token Receiver Plugin
@@ -65,30 +58,13 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC1155Receiver {
     function pluginManifest() external pure override returns (PluginManifest memory) {
         PluginManifest memory manifest;
 
-        manifest.executionFunctions = new bytes4[](3);
-        manifest.executionFunctions[0] = this.onERC721Received.selector;
-        manifest.executionFunctions[1] = this.onERC1155Received.selector;
-        manifest.executionFunctions[2] = this.onERC1155BatchReceived.selector;
-
-        // Only runtime validationFunction is needed since callbacks come from token contracts only
-        ManifestFunction memory alwaysAllowRuntime = ManifestFunction({
-            functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW,
-            functionId: 0, // Unused.
-            dependencyIndex: 0 // Unused.
-        });
-        manifest.validationFunctions = new ManifestAssociatedFunction[](3);
-        manifest.validationFunctions[0] = ManifestAssociatedFunction({
-            executionSelector: this.onERC721Received.selector,
-            associatedFunction: alwaysAllowRuntime
-        });
-        manifest.validationFunctions[1] = ManifestAssociatedFunction({
-            executionSelector: this.onERC1155Received.selector,
-            associatedFunction: alwaysAllowRuntime
-        });
-        manifest.validationFunctions[2] = ManifestAssociatedFunction({
-            executionSelector: this.onERC1155BatchReceived.selector,
-            associatedFunction: alwaysAllowRuntime
-        });
+        manifest.executionFunctions = new ManifestExecutionFunction[](3);
+        manifest.executionFunctions[0] =
+            ManifestExecutionFunction({executionSelector: this.onERC721Received.selector, isPublic: true});
+        manifest.executionFunctions[1] =
+            ManifestExecutionFunction({executionSelector: this.onERC1155Received.selector, isPublic: true});
+        manifest.executionFunctions[2] =
+            ManifestExecutionFunction({executionSelector: this.onERC1155BatchReceived.selector, isPublic: true});
 
         manifest.interfaceIds = new bytes4[](2);
         manifest.interfaceIds[0] = type(IERC721Receiver).interfaceId;

test/account/AccountExecHooks.t.sol

@@ -6,6 +6,7 @@ import {
     ManifestAssociatedFunctionType,
     ManifestAssociatedFunction,
     ManifestExecutionHook,
+    ManifestExecutionFunction,
     ManifestFunction,
     PluginManifest
 } from "../../src/interfaces/IPlugin.sol";
@@ -38,18 +39,7 @@ contract AccountExecHooksTest is AccountTestBase {
     function setUp() public {
         _transferOwnershipToTest();
 
-        m1.executionFunctions.push(_EXEC_SELECTOR);
-
-        m1.validationFunctions.push(
-            ManifestAssociatedFunction({
-                executionSelector: _EXEC_SELECTOR,
-                associatedFunction: ManifestFunction({
-                    functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW,
-                    functionId: 0,
-                    dependencyIndex: 0
-                })
-            })
-        );
+        m1.executionFunctions.push(ManifestExecutionFunction({executionSelector: _EXEC_SELECTOR, isPublic: true}));
     }
 
     function test_preExecHook_install() public {

test/account/ManifestValidity.t.sol

@@ -4,55 +4,17 @@ pragma solidity ^0.8.19;
 import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol";
 import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol";
 
-import {
-    BadValidationMagicValue_PreValidationHook_Plugin,
-    BadHookMagicValue_UserOpValidationFunction_Plugin,
-    BadHookMagicValue_RuntimeValidationFunction_Plugin
-} from "../mocks/plugins/ManifestValidityMocks.sol";
+import {BadHookMagicValue_ValidationFunction_Plugin} from "../mocks/plugins/ManifestValidityMocks.sol";
 import {AccountTestBase} from "../utils/AccountTestBase.sol";
 
 contract ManifestValidityTest is AccountTestBase {
     function setUp() public {
         _transferOwnershipToTest();
     }
 
-    // Tests that the plugin manager rejects a plugin with a pre-runtime validation hook set to "validation always
-    // allow"
-    function test_ManifestValidity_invalid_ValidationAlwaysAllow_PreValidationHook() public {
-        BadValidationMagicValue_PreValidationHook_Plugin plugin =
-            new BadValidationMagicValue_PreValidationHook_Plugin();
-
-        bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest()));
-
-        vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector));
-        account1.installPlugin({
-            plugin: address(plugin),
-            manifestHash: manifestHash,
-            pluginInstallData: "",
-            dependencies: new FunctionReference[](0)
-        });
-    }
-
-    // Tests that the plugin manager rejects a plugin with a user op validationFunction set to "hook always deny"
-    function test_ManifestValidity_invalid_HookAlwaysDeny_UserOpValidation() public {
-        BadHookMagicValue_UserOpValidationFunction_Plugin plugin =
-            new BadHookMagicValue_UserOpValidationFunction_Plugin();
-
-        bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest()));
-
-        vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector));
-        account1.installPlugin({
-            plugin: address(plugin),
-            manifestHash: manifestHash,
-            pluginInstallData: "",
-            dependencies: new FunctionReference[](0)
-        });
-    }
-
-    // Tests that the plugin manager rejects a plugin with a runtime validationFunction set to "hook always deny"
-    function test_ManifestValidity_invalid_HookAlwaysDeny_RuntimeValidationFunction() public {
-        BadHookMagicValue_RuntimeValidationFunction_Plugin plugin =
-            new BadHookMagicValue_RuntimeValidationFunction_Plugin();
+    // Tests that the plugin manager rejects a plugin with a validation function set to "hook always deny"
+    function test_ManifestValidity_invalid_HookAlwaysDeny_Validation() public {
+        BadHookMagicValue_ValidationFunction_Plugin plugin = new BadHookMagicValue_ValidationFunction_Plugin();
 
         bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest()));
 

test/mocks/plugins/BadTransferOwnershipPlugin.sol

@@ -1,13 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.19;
 
-import {
-    ManifestFunction,
-    ManifestAssociatedFunctionType,
-    ManifestAssociatedFunction,
-    PluginManifest,
-    PluginMetadata
-} from "../../../src/interfaces/IPlugin.sol";
+import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol";
 import {BasePlugin} from "../../../src/plugins/BasePlugin.sol";
 import {ISingleOwnerPlugin} from "../../../src/plugins/owner/ISingleOwnerPlugin.sol";
 import {IPluginExecutor} from "../../../src/interfaces/IPluginExecutor.sol";
@@ -38,22 +32,13 @@ contract BadTransferOwnershipPlugin is BasePlugin {
     function pluginManifest() external pure override returns (PluginManifest memory) {
         PluginManifest memory manifest;
 
-        manifest.executionFunctions = new bytes4[](1);
-        manifest.executionFunctions[0] = this.evilTransferOwnership.selector;
+        manifest.executionFunctions = new ManifestExecutionFunction[](1);
+        manifest.executionFunctions[0] =
+            ManifestExecutionFunction({executionSelector: this.evilTransferOwnership.selector, isPublic: true});
 
         manifest.permittedExecutionSelectors = new bytes4[](1);
         manifest.permittedExecutionSelectors[0] = ISingleOwnerPlugin.transferOwnership.selector;
 
-        manifest.validationFunctions = new ManifestAssociatedFunction[](1);
-        manifest.validationFunctions[0] = ManifestAssociatedFunction({
-            executionSelector: this.evilTransferOwnership.selector,
-            associatedFunction: ManifestFunction({
-                functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW,
-                functionId: 0, // Unused.
-                dependencyIndex: 0 // Unused.
-            })
-        });
-
         return manifest;
     }