fix: [quantstamp-16] Disallow using dependencies for hooks

Allowing hooks to be dependencies can lead to hooks being called in unexpected contexts, such as state-changing validation hooks being called outside of validation or one of a pre- and post-exec hook pair being called without the other. To prevent this without changing the interface from the spec, we disallow this case entirely.
alchemyplatform · Jan 22, 2024 · e0df98f · e0df98f
1 parent 0d607ed
commit e0df98f
Show file tree

Hide file tree

Showing 4 changed files with 209 additions and 1,073 deletions.
diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol
@@ -394,7 +394,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
                 _resolveManifestFunction(
                     mh.associatedFunction,
                     plugin,
-                    dependencies,
+                    new FunctionReference[](0),
                     ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 )
             );
@@ -409,7 +409,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
                 _resolveManifestFunction(
                     mh.associatedFunction,
                     plugin,
-                    dependencies,
+                    new FunctionReference[](0),
                     ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 )
             );
@@ -422,10 +422,13 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
             _addExecHooks(
                 mh.executionSelector,
                 _resolveManifestFunction(
-                    mh.preExecHook, plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
+                    mh.preExecHook,
+                    plugin,
+                    new FunctionReference[](0),
+                    ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 ),
                 _resolveManifestFunction(
-                    mh.postExecHook, plugin, dependencies, ManifestAssociatedFunctionType.NONE
+                    mh.postExecHook, plugin, new FunctionReference[](0), ManifestAssociatedFunctionType.NONE
                 )
             );
         }
@@ -502,10 +505,13 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
             _removeExecHooks(
                 mh.executionSelector,
                 _resolveManifestFunction(
-                    mh.preExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
+                    mh.preExecHook,
+                    args.plugin,
+                    new FunctionReference[](0),
+                    ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 ),
                 _resolveManifestFunction(
-                    mh.postExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.NONE
+                    mh.postExecHook, args.plugin, new FunctionReference[](0), ManifestAssociatedFunctionType.NONE
                 )
             );
         }
@@ -520,7 +526,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
                 _resolveManifestFunction(
                     mh.associatedFunction,
                     args.plugin,
-                    dependencies,
+                    new FunctionReference[](0),
                     ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 )
             );
@@ -536,7 +542,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
                 _resolveManifestFunction(
                     mh.associatedFunction,
                     args.plugin,
-                    dependencies,
+                    new FunctionReference[](0),
                     ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
                 )
             );
@@ -629,14 +635,20 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
     function _resolveManifestFunction(
         ManifestFunction memory manifestFunction,
         address plugin,
+        // Can be empty to indicate that type DEPENDENCY is invalid for this function.
         FunctionReference[] memory dependencies,
         // Indicates which magic value, if any, is permissible for the function to resolve.
         ManifestAssociatedFunctionType allowedMagicValue
     ) internal pure returns (FunctionReference) {
         if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) {
             return FunctionReferenceLib.pack(plugin, manifestFunction.functionId);
         } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) {
-            return dependencies[manifestFunction.dependencyIndex];
+            uint256 index = manifestFunction.dependencyIndex;
+            if (index < dependencies.length) {
+                return dependencies[manifestFunction.dependencyIndex];
+            } else {
+                revert InvalidPluginManifest();
+            }
         } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW)
         {
             if (allowedMagicValue == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) {

diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol
@@ -56,7 +56,7 @@ struct ManifestExternalCallPermission {
 struct PluginManifest {
     // List of ERC-165 interfaceIds to add to account to support introspection checks.
     bytes4[] interfaceIds;
-    // If this plugin depends on other plugins' validation functions and/or hooks, the interface IDs of
+    // If this plugin depends on other plugins' validation functions, the interface IDs of
     // those plugins MUST be provided here, with its position in the array matching the `dependencyIndex`
     // members of `ManifestFunction` structs used in the manifest.
     bytes4[] dependencyInterfaceIds;