Skip to content

Commit

Permalink
Remove uninstall field checking
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamegyed
adamegyed committed Nov 29, 2023
1 parent d9900e5 commit 0e95e25
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 79 deletions.
84 changes: 14 additions & 70 deletions src/account/PluginManagerInternals.sol
View file
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
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

0 comments on commit 0e95e25

Please sign in to comment.