Skip to content

Remove extraneous install/uninstall field checking #15

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

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Remove extraneous install/uninstall field checking #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 14 additions & 70 deletions src/account/PluginManagerInternals.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,8 @@ abstract contract PluginManagerInternals is IPluginManager {

error ArrayLengthMismatch();
error ExecuteFromPluginAlreadySet(bytes4 selector, address plugin);
error PermittedExecutionSelectorNotInstalled(bytes4 selector, address plugin);
error ExecuteFromPluginNotSet(bytes4 selector, address plugin);
error ExecutionFunctionAlreadySet(bytes4 selector);
error ExecutionFunctionNotSet(bytes4 selector);
error ExecutionHookAlreadySet(bytes4 selector, FunctionReference hook);
error ExecutionHookNotSet(bytes4 selector, FunctionReference hook);
error InvalidPostExecHook(bytes4 selector, FunctionReference hook);
error InvalidPostPermittedCallHook(bytes4 selector, FunctionReference hook);
error InvalidDependenciesProvided();
error InvalidPluginManifest();
error MissingPluginDependency(address dependency);
Expand All @@ -47,18 +41,13 @@ abstract contract PluginManagerInternals is IPluginManager {
error PluginAlreadyInstalled(address plugin);
error PluginDependencyViolation(address plugin);
error PermittedCallHookAlreadySet(bytes4 selector, address plugin, FunctionReference hook);
error PermittedCallHookNotSet(bytes4 selector, address plugin, FunctionReference hook);
error PluginInstallCallbackFailed(address plugin, bytes revertReason);
error PluginInterfaceNotSupported(address plugin);
error PluginNotInstalled(address plugin);
error PreRuntimeValidationHookAlreadySet(bytes4 selector, FunctionReference hook);
error PreRuntimeValidationHookNotSet(bytes4 selector, FunctionReference hook);
error PreUserOpValidationHookAlreadySet(bytes4 selector, FunctionReference hook);
error PreUserOpValidationHookNotSet(bytes4 selector, FunctionReference hook);
error RuntimeValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction);
error RuntimeValidationFunctionNotSet(bytes4 selector, FunctionReference validationFunction);
error UserOpValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction);
error UserOpValidationFunctionNotSet(bytes4 selector, FunctionReference validationFunction);
error PluginApplyHookCallbackFailed(address providingPlugin, bytes revertReason);
error PluginUnapplyHookCallbackFailed(address providingPlugin, bytes revertReason);

Expand Down Expand Up @@ -91,10 +80,6 @@ abstract contract PluginManagerInternals is IPluginManager {
function _removeExecutionFunction(bytes4 selector) internal {
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

if (_selectorData.plugin == address(0)) {
revert ExecutionFunctionNotSet(selector);
}

_selectorData.plugin = address(0);
}

Expand All @@ -117,14 +102,6 @@ abstract contract PluginManagerInternals is IPluginManager {
{
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

if (_selectorData.userOpValidation != validationFunction) {
// Revert if there's a different validationFunction set than the one the manifest intendes to remove.
// This
// indicates something wrong with the manifest and should not be allowed. In these cases, the original
// manifest should be passed for uninstall.
revert UserOpValidationFunctionNotSet(selector, validationFunction);
}

_selectorData.userOpValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE;
}

Expand All @@ -147,14 +124,6 @@ abstract contract PluginManagerInternals is IPluginManager {
{
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

if (_selectorData.runtimeValidation != validationFunction) {
// Revert if there's a different validationFunction set than the one the manifest intendes to remove.
// This
// indicates something wrong with the manifest and should not be allowed. In these cases, the original
// manifest should be passed for uninstall.
revert RuntimeValidationFunctionNotSet(selector, validationFunction);
}

_selectorData.runtimeValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE;
}

Expand All @@ -181,15 +150,9 @@ abstract contract PluginManagerInternals is IPluginManager {
{
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

// Removal also clears the flags.
if (!_selectorData.preExecHooks.remove(_toSetValue(preExecHook))) {
revert ExecutionHookNotSet(selector, preExecHook);
}
// May ignore return value, as the manifest hash is validated to ensure that the hook exists.
_selectorData.preExecHooks.remove(_toSetValue(preExecHook));

// Remove the associated post-exec hook, if it is set to the expected value.
if (postExecHook != _selectorData.associatedPostExecHooks[preExecHook]) {
revert InvalidPostExecHook(selector, postExecHook);
}
// If the post exec hook is set, clear it.
if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) {
_selectorData.associatedPostExecHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE;
Expand All @@ -201,23 +164,15 @@ abstract contract PluginManagerInternals is IPluginManager {
{
bytes24 key = getPermittedCallKey(plugin, selector);

if (accountStorage.selectorData[selector].plugin == address(0)) {
revert PermittedExecutionSelectorNotInstalled(selector, plugin);
}

if (accountStorage.permittedCalls[key].callPermitted) {
revert ExecuteFromPluginAlreadySet(selector, plugin);
}
// 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.
accountStorage.permittedCalls[key].callPermitted = true;
}

function _disableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage)
internal
{
bytes24 key = getPermittedCallKey(plugin, selector);
if (!accountStorage.permittedCalls[key].callPermitted) {
revert ExecuteFromPluginNotSet(selector, plugin);
}
accountStorage.permittedCalls[key].callPermitted = false;
}

Expand Down Expand Up @@ -250,14 +205,9 @@ abstract contract PluginManagerInternals is IPluginManager {
bytes24 permittedCallKey = getPermittedCallKey(plugin, selector);
PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey];

if (!_permittedCalldata.prePermittedCallHooks.remove(_toSetValue(preExecHook))) {
revert PermittedCallHookNotSet(selector, plugin, preExecHook);
}
// May ignore return value, as the manifest hash is validated to ensure that the hook exists.
_permittedCalldata.prePermittedCallHooks.remove(_toSetValue(preExecHook));

// Remove the associated post-exec hook, if it is set to the expected value.
if (postExecHook != _permittedCalldata.associatedPostPermittedCallHooks[preExecHook]) {
revert InvalidPostPermittedCallHook(selector, postExecHook);
}
// If the post permitted call exec hook is set, clear it.
if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) {
_permittedCalldata.associatedPostPermittedCallHooks[preExecHook] =
Expand All @@ -282,13 +232,10 @@ abstract contract PluginManagerInternals is IPluginManager {
internal
notNullFunction(preUserOpValidationHook)
{
if (
!getAccountStorage().selectorData[selector].preUserOpValidationHooks.remove(
_toSetValue(preUserOpValidationHook)
)
) {
revert PreUserOpValidationHookNotSet(selector, preUserOpValidationHook);
}
// May ignore return value, as the manifest hash is validated to ensure that the hook exists.
getAccountStorage().selectorData[selector].preUserOpValidationHooks.remove(
_toSetValue(preUserOpValidationHook)
);
}

function _addPreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook)
Expand All @@ -308,13 +255,10 @@ abstract contract PluginManagerInternals is IPluginManager {
internal
notNullFunction(preRuntimeValidationHook)
{
if (
!getAccountStorage().selectorData[selector].preRuntimeValidationHooks.remove(
_toSetValue(preRuntimeValidationHook)
)
) {
revert PreRuntimeValidationHookNotSet(selector, preRuntimeValidationHook);
}
// May ignore return value, as the manifest hash is validated to ensure that the hook exists.
getAccountStorage().selectorData[selector].preRuntimeValidationHooks.remove(
_toSetValue(preRuntimeValidationHook)
);
}

function _installPlugin(
Expand Down
11 changes: 2 additions & 9 deletions test/account/UpgradeableModularAccount.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ contract UpgradeableModularAccountTest is Test {
assertEq(plugins[1], address(tokenReceiverPlugin));
}

function test_installPlugin_ExecuteFromPlugin_BadPermittedExecSelector() public {
function test_installPlugin_ExecuteFromPlugin_PermittedExecSelectorNotInstalled() public {
vm.startPrank(owner2);

PluginManifest memory m;
Expand All @@ -306,13 +306,6 @@ contract UpgradeableModularAccountTest is Test {
MockPlugin mockPluginWithBadPermittedExec = new MockPlugin(m);
bytes32 manifestHash = keccak256(abi.encode(mockPluginWithBadPermittedExec.pluginManifest()));

vm.expectRevert(
abi.encodeWithSelector(
PluginManagerInternals.PermittedExecutionSelectorNotInstalled.selector,
IPlugin.onInstall.selector,
address(mockPluginWithBadPermittedExec)
)
);
IPluginManager(account2).installPlugin({
plugin: address(mockPluginWithBadPermittedExec),
manifestHash: manifestHash,
Expand Down Expand Up @@ -590,7 +583,7 @@ contract UpgradeableModularAccountTest is Test {
}

function test_injectHooksUninstall() external {
(, MockPlugin newPlugin, bytes32 manifestHash) = _installWithInjectHooks();
(, MockPlugin newPlugin,) = _installWithInjectHooks();

vm.expectEmit(true, true, true, true);
emit PluginUninstalled(address(newPlugin), true);
Expand Down