Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove permitted call hooks from MSCA #65

Closed
wants to merge 3 commits into from
+122 −2,046
Closed

feat: remove permitted call hooks from MSCA #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e70f1c
feat: remove permitted call hooks from MSCA
fangting-alchemy Jan 16, 2024
33fe39b
refactor: use bool callPermitted for struct PermittedCallData
fangting-alchemy Jan 16, 2024
5892732
fix lint
fangting-alchemy Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/account/AccountExecutor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ abstract contract AccountExecutor {
/// - Pre Runtime Validation Hook
/// - Runtime Validation
/// - Pre Execution Hook
/// - Pre Permitted Call Hook
/// And if it fails, reverts with the appropriate custom error.
function _executeRuntimePluginFunction(bytes memory buffer, address plugin, bytes4 errorSelector) internal {
if (!_executeRaw(plugin, buffer)) {
Expand Down
16 changes: 2 additions & 14 deletions src/account/AccountLoupe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,6 @@ abstract contract AccountLoupe is IAccountLoupe, AccountStorageV1 {
execHooks = _getHooks(_getAccountStorage().selectorData[selector].executionHooks);
}

/// @inheritdoc IAccountLoupe
function getPermittedCallHooks(address callingPlugin, bytes4 selector)
external
view
returns (ExecutionHooks[] memory execHooks)
{
PermittedCallData storage permittedCallData =
_getAccountStorage().permittedCalls[_getPermittedCallKey(callingPlugin, selector)];

execHooks = _getHooks(permittedCallData.permittedCallHooks);
}

/// @inheritdoc IAccountLoupe
function getPreValidationHooks(bytes4 selector)
external
Expand All @@ -73,8 +61,8 @@ abstract contract AccountLoupe is IAccountLoupe, AccountStorageV1 {
pluginAddresses = CastLib.toAddressArray(_getAccountStorage().plugins.getAll());
}

/// @dev Collects hook data from stored hooks (either execution hooks or permitted call hooks) and prepares it
/// for returning as the `ExecutionHooks` type defined by `IAccountLoupe`.
/// @dev Collects hook data from stored execution hooks and prepares it for returning as the `ExecutionHooks`
/// type defined by `IAccountLoupe`.
function _getHooks(HookGroup storage storedHooks) internal view returns (ExecutionHooks[] memory execHooks) {
FunctionReference[] memory preExecHooks = CastLib.toFunctionReferenceArray(storedHooks.preHooks.getAll());
FunctionReference[] memory postOnlyExecHooks =
Expand Down
235 changes: 7 additions & 228 deletions src/account/PluginManagerInternals.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
error NativeFunctionNotAllowed(bytes4 selector);
error NullFunctionReference();
error PluginAlreadyInstalled(address plugin);
error PluginApplyHookCallbackFailed(address providingPlugin, bytes revertReason);
error PluginDependencyViolation(address plugin);
error PluginHookUnapplyCallbackFailed(address providingPlugin, bytes revertReason);
error PluginInstallCallbackFailed(address plugin, bytes revertReason);
error PluginInterfaceNotSupported(address plugin);
error PluginNotInstalled(address plugin);
Expand Down Expand Up @@ -155,55 +153,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
}
}

function _enableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage storage_) internal {
PermittedCallData storage permittedCallData =
storage_.permittedCalls[_getPermittedCallKey(plugin, selector)];

// 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.
permittedCallData.callPermitted = true;
}

function _addPermittedCallHooks(
bytes4 selector,
address plugin,
FunctionReference preExecHook,
FunctionReference postExecHook
) internal {
PermittedCallData storage permittedCallData =
_getAccountStorage().permittedCalls[_getPermittedCallKey(plugin, selector)];

_addHooks(permittedCallData.permittedCallHooks, selector, preExecHook, postExecHook);

if (preExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) {
permittedCallData.hasPrePermittedCallHooks = true;
} else if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) {
// Only set this flag if the pre hook is empty and the post hook is non-empty.
permittedCallData.hasPostOnlyPermittedCallHooks = true;
}
}

function _removePermittedCallHooks(
bytes4 selector,
address plugin,
FunctionReference preExecHook,
FunctionReference postExecHook
) internal {
PermittedCallData storage permittedCallData =
_getAccountStorage().permittedCalls[_getPermittedCallKey(plugin, selector)];

(bool shouldClearHasPreHooks, bool shouldClearHasPostOnlyHooks) =
_removeHooks(permittedCallData.permittedCallHooks, preExecHook, postExecHook);

if (shouldClearHasPreHooks) {
permittedCallData.hasPrePermittedCallHooks = false;
}

if (shouldClearHasPostOnlyHooks) {
permittedCallData.hasPostOnlyPermittedCallHooks = false;
}
}

function _addHooks(
HookGroup storage hooks,
bytes4 selector,
Expand Down Expand Up @@ -328,8 +277,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
address plugin,
bytes32 manifestHash,
bytes memory pluginInitData,
FunctionReference[] memory dependencies,
InjectedHook[] memory injectedHooks
FunctionReference[] memory dependencies
) internal {
AccountStorage storage storage_ = _getAccountStorage();

Expand Down Expand Up @@ -392,7 +340,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
// Add installed plugin and selectors this plugin can call
length = manifest.permittedExecutionSelectors.length;
for (uint256 i = 0; i < length;) {
_enableExecFromPlugin(manifest.permittedExecutionSelectors[i], plugin, storage_);
storage_.callPermitted[_getPermittedCallKey(plugin, manifest.permittedExecutionSelectors[i])] = true;

unchecked {
++i;
Expand Down Expand Up @@ -432,46 +380,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
}
}

// Add injected hooks
length = injectedHooks.length;
// Manually set injected hooks array length
StoredInjectedHook[] storage injectedHooksArray = storage_.pluginData[plugin].injectedHooks;
assembly ("memory-safe") {
sstore(injectedHooksArray.slot, length)
}
for (uint256 i = 0; i < length;) {
InjectedHook memory hook = injectedHooks[i];

// Check that the dependency is installed. This also blocks self-dependencies.
if (storage_.pluginData[hook.providingPlugin].manifestHash == bytes32(0)) {
revert MissingPluginDependency(hook.providingPlugin);
}

injectedHooksArray[i] = StoredInjectedHook({
providingPlugin: hook.providingPlugin,
selector: hook.selector,
preExecHookFunctionId: hook.injectedHooksInfo.preExecHookFunctionId,
isPostHookUsed: hook.injectedHooksInfo.isPostHookUsed,
postExecHookFunctionId: hook.injectedHooksInfo.postExecHookFunctionId
});

// Increment the dependent count for the plugin providing the hook.
storage_.pluginData[hook.providingPlugin].dependentCount += 1;

_addPermittedCallHooks(
hook.selector,
plugin,
FunctionReferenceLib.pack(hook.providingPlugin, hook.injectedHooksInfo.preExecHookFunctionId),
hook.injectedHooksInfo.isPostHookUsed
? FunctionReferenceLib.pack(hook.providingPlugin, hook.injectedHooksInfo.postExecHookFunctionId)
: FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE
);

unchecked {
++i;
}
}

// Add user operation validation functions
length = manifest.userOpValidationFunctions.length;
for (uint256 i = 0; i < length;) {
Expand Down Expand Up @@ -563,31 +471,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
}
}

// Add pre and post permitted call hooks
length = manifest.permittedCallHooks.length;
for (uint256 i = 0; i < length;) {
_addPermittedCallHooks(
manifest.permittedCallHooks[i].executionSelector,
plugin,
_resolveManifestFunction(
manifest.permittedCallHooks[i].preExecHook,
plugin,
dependencies,
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
),
_resolveManifestFunction(
manifest.permittedCallHooks[i].postExecHook,
plugin,
dependencies,
ManifestAssociatedFunctionType.NONE
)
);

unchecked {
++i;
}
}

// Add new interface ids the plugin enabled for the account
length = manifest.interfaceIds.length;
for (uint256 i = 0; i < length;) {
Expand All @@ -610,42 +493,17 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
storage_.pluginData[plugin].canSpendNativeToken = true;
}

// Call injected hooks' onHookApply after all setup, this is before calling plugin onInstall
length = injectedHooks.length;
for (uint256 i = 0; i < length;) {
InjectedHook memory hook = injectedHooks[i];

/* solhint-disable no-empty-blocks */
try IPlugin(hook.providingPlugin).onHookApply(
plugin, hook.injectedHooksInfo, injectedHooks[i].hookApplyData
) {} catch (bytes memory revertReason) {
revert PluginApplyHookCallbackFailed(hook.providingPlugin, revertReason);
}
/* solhint-enable no-empty-blocks */

// zero out hookApplyData to reduce log cost
injectedHooks[i].hookApplyData = new bytes(0);

unchecked {
++i;
}
}

// Initialize the plugin storage for the account.
// solhint-disable-next-line no-empty-blocks
try IPlugin(plugin).onInstall(pluginInitData) {}
catch (bytes memory revertReason) {
revert PluginInstallCallbackFailed(plugin, revertReason);
}

emit PluginInstalled(plugin, manifestHash, dependencies, injectedHooks);
emit PluginInstalled(plugin, manifestHash, dependencies);
}

function _uninstallPlugin(
UninstallPluginArgs memory args,
bytes calldata uninstallData,
bytes[] calldata hookUnapplyData
) internal {
function _uninstallPlugin(UninstallPluginArgs memory args, bytes calldata uninstallData) internal {
AccountStorage storage storage_ = _getAccountStorage();

// Check if the plugin exists.
Expand Down Expand Up @@ -685,31 +543,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {

// Remove components according to the manifest, in reverse order (by component type) of their installation.

// Remove pre and post permitted call hooks
length = args.manifest.permittedCallHooks.length;
for (uint256 i = 0; i < length;) {
_removePermittedCallHooks(
args.manifest.permittedCallHooks[i].executionSelector,
args.plugin,
_resolveManifestFunction(
args.manifest.permittedCallHooks[i].preExecHook,
args.plugin,
dependencies,
ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY
),
_resolveManifestFunction(
args.manifest.permittedCallHooks[i].postExecHook,
args.plugin,
dependencies,
ManifestAssociatedFunctionType.NONE
)
);

unchecked {
++i;
}
}

// Remove pre and post execution function hooks
length = args.manifest.executionHooks.length;
for (uint256 i = 0; i < length;) {
Expand Down Expand Up @@ -827,33 +660,11 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
}
}

// Remove injected hooks
length = pluginData.injectedHooks.length;
for (uint256 i = 0; i < length;) {
StoredInjectedHook memory hook = pluginData.injectedHooks[i];

// Decrement the dependent count for the plugin providing the hook.
storage_.pluginData[hook.providingPlugin].dependentCount -= 1;

_removePermittedCallHooks(
hook.selector,
args.plugin,
FunctionReferenceLib.pack(hook.providingPlugin, hook.preExecHookFunctionId),
hook.isPostHookUsed
? FunctionReferenceLib.pack(hook.providingPlugin, hook.postExecHookFunctionId)
: FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE
);

unchecked {
++i;
}
}

// Remove permitted account execution function call permissions
length = args.manifest.permittedExecutionSelectors.length;
for (uint256 i = 0; i < length;) {
storage_.permittedCalls[_getPermittedCallKey(args.plugin, args.manifest.permittedExecutionSelectors[i])]
.callPermitted = false;
storage_.callPermitted[_getPermittedCallKey(args.plugin, args.manifest.permittedExecutionSelectors[i])]
= false;

unchecked {
++i;
Expand All @@ -880,40 +691,8 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 {
}
}

// Call onHookUnapply on all injected hooks
bool callbacksSucceeded = true;
length = pluginData.injectedHooks.length;
bool hasUnapplyHookData = hookUnapplyData.length != 0;
if (hasUnapplyHookData && hookUnapplyData.length != length) {
revert ArrayLengthMismatch();
}
for (uint256 i = 0; i < length;) {
StoredInjectedHook memory hook = pluginData.injectedHooks[i];

/* solhint-disable no-empty-blocks */
try IPlugin(hook.providingPlugin).onHookUnapply{gas: args.callbackGasLimit}(
args.plugin,
InjectedHooksInfo({
preExecHookFunctionId: hook.preExecHookFunctionId,
isPostHookUsed: hook.isPostHookUsed,
postExecHookFunctionId: hook.postExecHookFunctionId
}),
hasUnapplyHookData ? hookUnapplyData[i] : bytes("")
) {} catch (bytes memory revertReason) {
if (!args.forceUninstall) {
revert PluginHookUnapplyCallbackFailed(hook.providingPlugin, revertReason);
}
callbacksSucceeded = false;
emit PluginIgnoredHookUnapplyCallbackFailure(args.plugin, hook.providingPlugin);
}
/* solhint-enable no-empty-blocks */

unchecked {
++i;
}
}

// Clear the plugin storage for the account.
bool callbacksSucceeded = true;
// solhint-disable-next-line no-empty-blocks
try IPlugin(args.plugin).onUninstall{gas: args.callbackGasLimit}(uninstallData) {}
catch (bytes memory revertReason) {
Expand Down
Loading