From a7ee616870eb536a75a12b3e8c210985e4683c42 Mon Sep 17 00:00:00 2001 From: fangting-alchemy <119372438+fangting-alchemy@users.noreply.github.com> Date: Thu, 21 Dec 2023 15:13:29 -0800 Subject: [PATCH] fix: [quantstamp-20] clear plugin data early in uninstallPlugin (#25) and clean github lint action --- .github/workflows/test.yml | 24 +++++++++------------ src/account/PluginManagerInternals.sol | 30 +++++++++++++------------- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a45ada406..68325d4de 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,19 +12,15 @@ jobs: name: Run Linters runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 - with: - version: nightly-5be158ba6dc7c798a6f032026fe60fc01686b33b - - name: "Check out the repo" uses: "actions/checkout@v3" with: submodules: "recursive" - - name: "Install Foundry" - uses: "foundry-rs/foundry-toolchain@v1" + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + with: + version: nightly-5be158ba6dc7c798a6f032026fe60fc01686b33b - name: "Install Pnpm" uses: "pnpm/action-setup@v2" @@ -37,16 +33,14 @@ jobs: cache: "pnpm" node-version: "lts/*" - - name: "Install the Node.js dependencies" + - name: "Install Node.js dependencies" run: "pnpm install" - - run: forge install - - run: forge fmt --check - name: "Lint the contracts" run: "pnpm lint" - + # check-inspect: # name: Verify Inspections # runs-on: ubuntu-latest @@ -70,7 +64,8 @@ jobs: name: Run Forge Tests runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - name: "Check out the repo" + uses: actions/checkout@v3 with: submodules: recursive @@ -92,7 +87,8 @@ jobs: name: Run Forge Tests [lite build] runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - name: "Check out the repo" + uses: actions/checkout@v3 with: submodules: recursive diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index fc6104e1f..594211602 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -651,18 +651,20 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { revert PluginNotInstalled(args.plugin); } + PluginData memory pluginData = storage_.pluginData[args.plugin]; + // Check manifest hash. - if (!_isValidPluginManifest(args.manifest, storage_.pluginData[args.plugin].manifestHash)) { + if (!_isValidPluginManifest(args.manifest, pluginData.manifestHash)) { revert InvalidPluginManifest(); } // Ensure that there are no dependent plugins. - if (storage_.pluginData[args.plugin].dependentCount != 0) { + if (pluginData.dependentCount != 0) { revert PluginDependencyViolation(args.plugin); } // Remove this plugin as a dependent from its dependencies. - FunctionReference[] memory dependencies = storage_.pluginData[args.plugin].dependencies; + FunctionReference[] memory dependencies = pluginData.dependencies; uint256 length = dependencies.length; for (uint256 i = 0; i < length;) { FunctionReference dependency = dependencies[i]; @@ -676,6 +678,9 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } } + // Remove the plugin metadata from the account. + delete storage_.pluginData[args.plugin]; + // Remove components according to the manifest, in reverse order (by component type) of their installation. // Remove pre and post permitted call hooks @@ -786,11 +791,9 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } } - // Remove permitted external call permissions - if (args.manifest.permitAnyExternalAddress) { - // Only clear if it was set during install time - storage_.pluginData[args.plugin].anyExternalAddressPermitted = false; - } else { + // Remove permitted external call permissions, anyExternalAddressPermitted is cleared when pluginData being + // deleted + if (!args.manifest.permitAnyExternalAddress) { // Only clear the specific permitted external calls if "permit any" flag was not set. length = args.manifest.permittedExternalCalls.length; for (uint256 i = 0; i < length;) { @@ -823,9 +826,9 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } // Remove injected hooks - length = storage_.pluginData[args.plugin].injectedHooks.length; + length = pluginData.injectedHooks.length; for (uint256 i = 0; i < length;) { - StoredInjectedHook memory hook = storage_.pluginData[args.plugin].injectedHooks[i]; + StoredInjectedHook memory hook = pluginData.injectedHooks[i]; // Decrement the dependent count for the plugin providing the hook. storage_.pluginData[hook.providingPlugin].dependentCount -= 1; @@ -877,13 +880,13 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { // Call onHookUnapply on all injected hooks bool callbacksSucceeded = true; - length = storage_.pluginData[args.plugin].injectedHooks.length; + 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 = storage_.pluginData[args.plugin].injectedHooks[i]; + StoredInjectedHook memory hook = pluginData.injectedHooks[i]; /* solhint-disable no-empty-blocks */ try IPlugin(hook.providingPlugin).onHookUnapply{gas: args.callbackGasLimit}( @@ -908,9 +911,6 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { } } - // Remove the plugin metadata from the account. - delete storage_.pluginData[args.plugin]; - // Clear the plugin storage for the account. // solhint-disable-next-line no-empty-blocks try IPlugin(args.plugin).onUninstall{gas: args.callbackGasLimit}(uninstallData) {}