Skip to content

Commit 510b541

Browse files
jaypaikjaypaik
authored
feat!: disallow using dependencies for hooks (#29)
1 parent 7846d38 commit 510b541

File tree

2 files changed

+76
-161
lines changed

2 files changed

+76
-161
lines changed

‎src/account/PluginManagerInternals.sol‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,6 @@ abstract contract PluginManagerInternals is IPluginManager {
288288
_storage.pluginData[plugin].dependencies = dependencies;
289289

290290
// Update components according to the manifest.
291-
// All conflicts should revert.
292291

293292
// Mark whether or not this plugin may spend native token amounts
294293
if (manifest.canSpendNativeToken) {
@@ -380,6 +379,9 @@ abstract contract PluginManagerInternals is IPluginManager {
380379
}
381380
}
382381

382+
// Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them.
383+
FunctionReference[] memory emptyDependencies;
384+
383385
length = manifest.preUserOpValidationHooks.length;
384386
for (uint256 i = 0; i < length;) {
385387
ManifestAssociatedFunction memory mh = manifest.preUserOpValidationHooks[i];
@@ -388,7 +390,7 @@ abstract contract PluginManagerInternals is IPluginManager {
388390
_resolveManifestFunction(
389391
mh.associatedFunction,
390392
plugin,
391-
dependencies,
393+
emptyDependencies,
392394
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
393395
)
394396
);
@@ -406,7 +408,7 @@ abstract contract PluginManagerInternals is IPluginManager {
406408
_resolveManifestFunction(
407409
mh.associatedFunction,
408410
plugin,
409-
dependencies,
411+
emptyDependencies,
410412
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
411413
)
412414
);
@@ -421,10 +423,10 @@ abstract contract PluginManagerInternals is IPluginManager {
421423
_addExecHooks(
422424
mh.executionSelector,
423425
_resolveManifestFunction(
424-
mh.preExecHook, plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
426+
mh.preExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
425427
),
426428
_resolveManifestFunction(
427-
mh.postExecHook, plugin, dependencies, ManifestAssociatedFunctionType.NONE
429+
mh.postExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE
428430
)
429431
);
430432

@@ -488,18 +490,20 @@ abstract contract PluginManagerInternals is IPluginManager {
488490
}
489491

490492
// Remove components according to the manifest, in reverse order (by component type) of their installation.
491-
// If any expected components are missing, revert.
493+
494+
// Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them.
495+
FunctionReference[] memory emptyDependencies;
492496

493497
length = manifest.executionHooks.length;
494498
for (uint256 i = 0; i < length;) {
495499
ManifestExecutionHook memory mh = manifest.executionHooks[i];
496500
_removeExecHooks(
497501
mh.executionSelector,
498502
_resolveManifestFunction(
499-
mh.preExecHook, plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
503+
mh.preExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
500504
),
501505
_resolveManifestFunction(
502-
mh.postExecHook, plugin, dependencies, ManifestAssociatedFunctionType.NONE
506+
mh.postExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE
503507
)
504508
);
505509

@@ -516,7 +520,7 @@ abstract contract PluginManagerInternals is IPluginManager {
516520
_resolveManifestFunction(
517521
mh.associatedFunction,
518522
plugin,
519-
dependencies,
523+
emptyDependencies,
520524
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
521525
)
522526
);
@@ -534,7 +538,7 @@ abstract contract PluginManagerInternals is IPluginManager {
534538
_resolveManifestFunction(
535539
mh.associatedFunction,
536540
plugin,
537-
dependencies,
541+
emptyDependencies,
538542
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
539543
)
540544
);
@@ -698,6 +702,9 @@ abstract contract PluginManagerInternals is IPluginManager {
698702
if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) {
699703
return FunctionReferenceLib.pack(plugin, manifestFunction.functionId);
700704
} else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) {
705+
if (manifestFunction.dependencyIndex >= dependencies.length) {
706+
revert InvalidPluginManifest();
707+
}
701708
return dependencies[manifestFunction.dependencyIndex];
702709
} else if (manifestFunction.functionType == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW)
703710
{

‎test/account/AccountExecHooks.t.sol‎

Lines changed: 59 additions & 151 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
PluginManifest
1414
} from "../../src/interfaces/IPlugin.sol";
1515
import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol";
16+
import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol";
1617
import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol";
1718
import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol";
1819

@@ -196,99 +197,47 @@ contract AccountExecHooksTest is OptimizedTest {
196197
_uninstallPlugin(mockPlugin1);
197198
}
198199

199-
function test_overlappingExecHookPairs_install() public {
200+
function test_overlappingPreExecHooks_install() public {
200201
// Install the first plugin.
201202
_installPlugin1WithHooks(
202203
_EXEC_SELECTOR,
203204
ManifestFunction({
204-
functionType: ManifestAssociatedFunctionType.SELF,
205-
functionId: _PRE_HOOK_FUNCTION_ID_1,
205+
functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY,
206+
functionId: 0,
206207
dependencyIndex: 0
207208
}),
208-
ManifestFunction({
209-
functionType: ManifestAssociatedFunctionType.SELF,
210-
functionId: _POST_HOOK_FUNCTION_ID_2,
211-
dependencyIndex: 0
212-
})
209+
ManifestFunction(ManifestAssociatedFunctionType.NONE, 0, 0)
213210
);
214211

215-
// Install a second plugin that applies the first plugin's hook pair to the same selector.
216-
FunctionReference[] memory dependencies = new FunctionReference[](2);
217-
dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1);
218-
dependencies[1] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2);
219-
_installPlugin2WithHooks(
212+
// Install a second plugin that applies the same pre hook on the same selector.
213+
_installPlugin2WithHooksExpectSuccess(
220214
_EXEC_SELECTOR,
221215
ManifestFunction({
222-
functionType: ManifestAssociatedFunctionType.DEPENDENCY,
216+
functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY,
223217
functionId: 0,
224218
dependencyIndex: 0
225219
}),
226-
ManifestFunction({
227-
functionType: ManifestAssociatedFunctionType.DEPENDENCY,
228-
functionId: 0,
229-
dependencyIndex: 1
230-
}),
231-
dependencies
220+
ManifestFunction(ManifestAssociatedFunctionType.NONE, 0, 0),
221+
new FunctionReference[](0)
232222
);
233223

234224
vm.stopPrank();
235225
}
236226

237-
/// @dev Plugin 1 hook pair: [1, 2]
238-
/// Plugin 2 hook pair: [1, 2]
239-
/// Expected execution: [1, 2]
240-
function test_overlappingExecHookPairs_run() public {
241-
test_overlappingExecHookPairs_install();
242-
243-
// Expect the pre hook to be called just once.
244-
vm.expectCall(
245-
address(mockPlugin1),
246-
abi.encodeWithSelector(
247-
IPlugin.preExecutionHook.selector,
248-
_PRE_HOOK_FUNCTION_ID_1,
249-
address(this), // caller
250-
0, // msg.value in call to account
251-
abi.encodeWithSelector(_EXEC_SELECTOR)
252-
),
253-
1
254-
);
255-
256-
// Expect the post hook to be called just once, with the expected data.
257-
vm.expectCall(
258-
address(mockPlugin1),
259-
abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""),
260-
1
261-
);
262-
227+
function test_overlappingPreExecHooks_run() public {
263228
(bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR));
264-
assertTrue(success);
229+
assertFalse(success);
265230
}
266231

267-
function test_overlappingExecHookPairs_uninstall() public {
268-
test_overlappingExecHookPairs_install();
232+
function test_overlappingPreExecHooks_uninstall() public {
233+
test_overlappingPreExecHooks_install();
269234

270235
// Uninstall the second plugin.
271236
_uninstallPlugin(mockPlugin2);
272237

273-
// Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook.
274-
vm.expectCall(
275-
address(mockPlugin1),
276-
abi.encodeWithSelector(
277-
IPlugin.preExecutionHook.selector,
278-
_PRE_HOOK_FUNCTION_ID_1,
279-
address(this), // caller
280-
0, // msg.value in call to account
281-
abi.encodeWithSelector(_EXEC_SELECTOR)
282-
),
283-
1
284-
);
285-
vm.expectCall(
286-
address(mockPlugin1),
287-
abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""),
288-
1
289-
);
238+
// Expect the pre hook to still exist.
290239
(bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR));
291-
assertTrue(success);
240+
assertFalse(success);
292241

293242
// Uninstall the first plugin.
294243
_uninstallPlugin(mockPlugin1);
@@ -298,7 +247,7 @@ contract AccountExecHooksTest is OptimizedTest {
298247
assertFalse(success);
299248
}
300249

301-
function test_overlappingExecHookPairsOnPost_install() public {
250+
function test_execHookDependencies_failInstall() public {
302251
// Install the first plugin.
303252
_installPlugin1WithHooks(
304253
_EXEC_SELECTOR,
@@ -314,98 +263,29 @@ contract AccountExecHooksTest is OptimizedTest {
314263
})
315264
);
316265

317-
// Install the second plugin.
318-
FunctionReference[] memory dependencies = new FunctionReference[](1);
319-
dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2);
320-
_installPlugin2WithHooks(
266+
// Attempt to install a second plugin that applies the first plugin's hook pair (as dependencies) to the
267+
// same selector. This should revert.
268+
FunctionReference[] memory dependencies = new FunctionReference[](2);
269+
dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1);
270+
dependencies[1] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2);
271+
272+
_installPlugin2WithHooksExpectFail(
321273
_EXEC_SELECTOR,
322274
ManifestFunction({
323-
functionType: ManifestAssociatedFunctionType.SELF,
324-
functionId: _PRE_HOOK_FUNCTION_ID_3,
275+
functionType: ManifestAssociatedFunctionType.DEPENDENCY,
276+
functionId: 0,
325277
dependencyIndex: 0
326278
}),
327279
ManifestFunction({
328280
functionType: ManifestAssociatedFunctionType.DEPENDENCY,
329281
functionId: 0,
330-
dependencyIndex: 0
282+
dependencyIndex: 1
331283
}),
332-
dependencies
333-
);
334-
}
335-
336-
/// @dev Plugin 1 hook pair: [1, 2]
337-
/// Plugin 2 hook pair: [3, 2]
338-
/// Expected execution: [1, 2], [3, 2]
339-
function test_overlappingExecHookPairsOnPost_run() public {
340-
test_overlappingExecHookPairsOnPost_install();
341-
342-
// Expect each pre hook to be called once.
343-
vm.expectCall(
344-
address(mockPlugin1),
345-
abi.encodeWithSelector(
346-
IPlugin.preExecutionHook.selector,
347-
_PRE_HOOK_FUNCTION_ID_1,
348-
address(this), // caller
349-
0, // msg.value in call to account
350-
abi.encodeWithSelector(_EXEC_SELECTOR)
351-
),
352-
1
353-
);
354-
vm.expectCall(
355-
address(mockPlugin2),
356-
abi.encodeWithSelector(
357-
IPlugin.preExecutionHook.selector,
358-
_PRE_HOOK_FUNCTION_ID_3,
359-
address(this), // caller
360-
0, // msg.value in call to account
361-
abi.encodeWithSelector(_EXEC_SELECTOR)
362-
),
363-
1
364-
);
365-
366-
// Expect the post hook to be called twice, with the expected data.
367-
vm.expectCall(
368-
address(mockPlugin1),
369-
abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""),
370-
2
284+
dependencies,
285+
abi.encodePacked(PluginManagerInternals.InvalidPluginManifest.selector)
371286
);
372287

373-
(bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR));
374-
assertTrue(success);
375-
}
376-
377-
function test_overlappingExecHookPairsOnPost_uninstall() public {
378-
test_overlappingExecHookPairsOnPost_install();
379-
380-
// Uninstall the second plugin.
381-
_uninstallPlugin(mockPlugin2);
382-
383-
// Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook.
384-
vm.expectCall(
385-
address(mockPlugin1),
386-
abi.encodeWithSelector(
387-
IPlugin.preExecutionHook.selector,
388-
_PRE_HOOK_FUNCTION_ID_1,
389-
address(this), // caller
390-
0, // msg.value in call to account
391-
abi.encodeWithSelector(_EXEC_SELECTOR)
392-
),
393-
1
394-
);
395-
vm.expectCall(
396-
address(mockPlugin1),
397-
abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""),
398-
1
399-
);
400-
(bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR));
401-
assertTrue(success);
402-
403-
// Uninstall the first plugin.
404-
_uninstallPlugin(mockPlugin1);
405-
406-
// Execution selector should no longer exist.
407-
(success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR));
408-
assertFalse(success);
288+
vm.stopPrank();
409289
}
410290

411291
function _installPlugin1WithHooks(
@@ -430,7 +310,7 @@ contract AccountExecHooksTest is OptimizedTest {
430310
});
431311
}
432312

433-
function _installPlugin2WithHooks(
313+
function _installPlugin2WithHooksExpectSuccess(
434314
bytes4 selector,
435315
ManifestFunction memory preHook,
436316
ManifestFunction memory postHook,
@@ -461,6 +341,34 @@ contract AccountExecHooksTest is OptimizedTest {
461341
});
462342
}
463343

344+
function _installPlugin2WithHooksExpectFail(
345+
bytes4 selector,
346+
ManifestFunction memory preHook,
347+
ManifestFunction memory postHook,
348+
FunctionReference[] memory dependencies,
349+
bytes memory revertData
350+
) internal {
351+
if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) {
352+
m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId);
353+
}
354+
if (postHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) {
355+
m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId);
356+
}
357+
358+
m2.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook));
359+
360+
mockPlugin2 = new MockPlugin(m2);
361+
manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest()));
362+
363+
vm.expectRevert(revertData);
364+
account.installPlugin({
365+
plugin: address(mockPlugin2),
366+
manifestHash: manifestHash2,
367+
pluginInitData: bytes(""),
368+
dependencies: dependencies
369+
});
370+
}
371+
464372
function _uninstallPlugin(MockPlugin plugin) internal {
465373
vm.expectEmit(true, true, true, true);
466374
emit ReceivedCall(abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 0);

0 commit comments

Comments
 (0)