feat: [v0.8-develop, experimental]: move runtime validation always allow to execution function definition [4/N]

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,
@@ -57,8 +61,7 @@ struct AccountStorage {
     mapping(address => PluginData) pluginData;
     // Execution functions and their associated functions
     mapping(bytes4 => SelectorData) selectorData;
-    // bytes24 key = address(calling plugin) || bytes4(selector of execution function)
-    mapping(bytes24 => bool) callPermitted;
+    mapping(address caller => mapping(bytes4 selector => bool)) callPermitted;
     // key = address(calling plugin) || target address
     mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls;
     // For ERC165 introspection
@@ -77,10 +80,6 @@ function getAccountStorage() pure returns (AccountStorage storage _storage) {
     }
 }
 
-function getPermittedCallKey(address addr, bytes4 selector) pure returns (bytes24) {
-    return bytes24(bytes20(addr)) | (bytes24(selector) >> 160);
-}
-
 using EnumerableSet for EnumerableSet.Bytes32Set;
 
 function toSetValue(FunctionReference functionReference) pure returns (bytes32) {

src/account/PluginManagerInternals.sol

@@ -21,7 +21,6 @@ import {
     getAccountStorage,
     SelectorData,
     toSetValue,
-    getPermittedCallKey,
     PermittedExternalCallData
 } from "./AccountStorage.sol";
 
@@ -60,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)
@@ -212,15 +216,17 @@ 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
         length = manifest.permittedExecutionSelectors.length;
         for (uint256 i = 0; i < length; ++i) {
             // If there are duplicates, this will just enable the flag again. This is not a problem, since the
             // boolean will be set to false twice during uninstall, which is fine.
-            _storage.callPermitted[getPermittedCallKey(plugin, manifest.permittedExecutionSelectors[i])] = true;
+            _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true;
         }
 
         // Add the permitted external calls to the account.
@@ -254,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
                 )
             );
         }
@@ -379,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
                 )
             );
         }
@@ -417,12 +417,13 @@ abstract contract PluginManagerInternals is IPluginManager {
 
         length = manifest.permittedExecutionSelectors.length;
         for (uint256 i = 0; i < length; ++i) {
-            _storage.callPermitted[getPermittedCallKey(plugin, manifest.permittedExecutionSelectors[i])] = false;
+            _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = false;
         }
 
         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;
@@ -466,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

@@ -23,7 +23,6 @@ import {AccountLoupe} from "./AccountLoupe.sol";
 import {
     AccountStorage,
     getAccountStorage,
-    getPermittedCallKey,
     SelectorData,
     toSetValue,
     toFunctionReference,
@@ -187,11 +186,9 @@ contract UpgradeableModularAccount is
         bytes4 selector = bytes4(data[:4]);
         address callingPlugin = msg.sender;
 
-        bytes24 execFromPluginKey = getPermittedCallKey(callingPlugin, selector);
-
         AccountStorage storage _storage = getAccountStorage();
 
-        if (!_storage.callPermitted[execFromPluginKey]) {
+        if (!_storage.callPermitted[callingPlugin][selector]) {
             revert ExecFromPluginNotPermitted(callingPlugin, selector);
         }
 
@@ -375,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);
         }
 
@@ -446,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 validation, 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 {