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

[dphilipson]

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.

[jaypaik]

Looks like a couple legit lint errors. Also left a comment on _resolveManifestFunction.

[adam-alchemy]

I think this is related to the spearbit issue https://github.com/spearbit-audits/alchemy-nov-review/issues/23. In the reference implementation PR, there's an added check in _resolveManifestFunction here: https://github.com/erc6900/reference-implementation/blob/c804a93279e3270cc78d3b6bfc0e7d8d95524fe1/src/account/PluginManagerInternals.sol#L705-L707.

Do you want to also address that here too, or spin it out into a separate PR?

[dphilipson]

@adam Yup, I got that fix when following Jay's suggestion for the arguments to _resolveManifestFunction.

[jaypaik]

Looks good, some comments.

[jaypaik]

Nit:

Suggested change

	uint256 index = manifestFunction.dependencyIndex;
	if (index < dependencies.length) {
	return dependencies[manifestFunction.dependencyIndex];
	} else {
	revert InvalidPluginManifest();
	}
	uint256 index = manifestFunction.dependencyIndex;
	if (index < dependencies.length) {
	return dependencies[index];
	}
	revert InvalidPluginManifest();

[dphilipson]

This suggestion is inconsistent with the existing code, such as the two magic value checks just below it. Also as a personal preference I prefer the current style, but I'm happy to change this and the rest of the function if this is the recommended style for this codebase.

[jaypaik]

Yup makes sense. I think we've been changing things to the new style for less nesting in various places. Happy to pick this up as a part of #97 which is doing similar things.