Inline withHook in AccountERC7579Hooked's uninstallModule to avoid reverting

.changeset/chatty-dryers-joke.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`AccountERC7579Hooked`: Do not revert if hook checks fail during the hook module uninstallation.

contracts/account/extensions/draft-AccountERC7579Hooked.sol

@@ -6,6 +6,7 @@ pragma solidity ^0.8.26;
 import {IERC7579Hook, MODULE_TYPE_HOOK} from "../../interfaces/draft-IERC7579.sol";
 import {ERC7579Utils, Mode} from "../../account/utils/draft-ERC7579Utils.sol";
 import {AccountERC7579} from "./draft-AccountERC7579.sol";
+import {LowLevelCall} from "../../utils/LowLevelCall.sol";
 
 /**
  * @dev Extension of {AccountERC7579} with support for a single hook module (type 4).
@@ -39,6 +40,28 @@ abstract contract AccountERC7579Hooked is AccountERC7579 {
         if (hook_ != address(0)) IERC7579Hook(hook_).postCheck(hookData);
     }
 
+    /// @dev Variant of `withHook` modifier that doesn't revert if the hook reverts. This is useful for uninstalling malicious or bugged hooks.
+    modifier withHookNoRevert() {
+        address hook_ = hook();
+        bytes memory hookData;
+        bool preCheckSuccess;
+
+        // slither-disable-next-line reentrancy-no-eth
+        if (hook_ != address(0)) {
+            preCheckSuccess = LowLevelCall.callNoReturn(
+                hook_,
+                abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data))
+            );
+            if (preCheckSuccess) {
+                hookData = LowLevelCall.returnData();
+            }
+        }
+        _;
+        if (hook_ != address(0) && preCheckSuccess) {
+            LowLevelCall.callNoReturn(hook_, abi.encodeCall(IERC7579Hook.postCheck, (hookData)));
+        }
+    }
+
     /// @inheritdoc AccountERC7579
     function accountId() public view virtual override returns (string memory) {
         // vendorname.accountname.semver
@@ -80,15 +103,24 @@ abstract contract AccountERC7579Hooked is AccountERC7579 {
     }
 
     /// @dev Uninstalls a module with support for hook modules. See {AccountERC7579-_uninstallModule}
-    function _uninstallModule(
-        uint256 moduleTypeId,
-        address module,
-        bytes memory deInitData
-    ) internal virtual override withHook {
+    function _uninstallModule(uint256 moduleTypeId, address module, bytes memory deInitData) internal virtual override {
         if (moduleTypeId == MODULE_TYPE_HOOK) {
-            require(_hook == module, ERC7579Utils.ERC7579UninstalledModule(moduleTypeId, module));
-            _hook = address(0);
+            // includes the `withHookNoRevert` modifier to ensure that the hook can be uninstalled even if it is malicious or bugged and reverts on preCheck or postCheck.
+            _uninstallHookModule(module, deInitData);
+        } else {
+            // calls super with the `withHook` modifier.
+            _uninstallOtherModule(moduleTypeId, module, deInitData);
         }
+    }
+
+    function _uninstallHookModule(address module, bytes memory deInitData) private withHookNoRevert {
+        require(_hook == module, ERC7579Utils.ERC7579UninstalledModule(MODULE_TYPE_HOOK, module));
+        _hook = address(0);
+
+        super._uninstallModule(MODULE_TYPE_HOOK, module, deInitData);
+    }
+
+    function _uninstallOtherModule(uint256 moduleTypeId, address module, bytes memory deInitData) private withHook {
         super._uninstallModule(moduleTypeId, module, deInitData);
     }
 

contracts/mocks/account/modules/ERC7579Mock.sol

@@ -48,16 +48,29 @@ abstract contract ERC7579HookMock is ERC7579ModuleMock(MODULE_TYPE_HOOK), IERC75
     event PreCheck(address sender, uint256 value, bytes data);
     event PostCheck(bytes hookData);
 
+    bool private _shouldRevertOnPreCheck = false;
+    bool private _shouldRevertOnPostCheck = false;
+
+    function revertOnPreCheck(bool shouldRevert) external {
+        _shouldRevertOnPreCheck = shouldRevert;
+    }
+
+    function revertOnPostCheck(bool shouldRevert) external {
+        _shouldRevertOnPostCheck = shouldRevert;
+    }
+
     function preCheck(
         address msgSender,
         uint256 value,
         bytes calldata msgData
     ) external returns (bytes memory hookData) {
+        require(!_shouldRevertOnPreCheck, "preCheck reverts");
         emit PreCheck(msgSender, value, msgData);
         return msgData;
     }
 
     function postCheck(bytes calldata hookData) external {
+        require(!_shouldRevertOnPostCheck, "postCheck reverts");
         emit PostCheck(hookData);
     }
 }

test/account/extensions/AccountERC7579.behavior.js

@@ -1,5 +1,6 @@
 const { ethers, predeploy } = require('hardhat');
 const { expect } = require('chai');
+const { setCode } = require('@nomicfoundation/hardhat-network-helpers');
 const { impersonate } = require('../../helpers/account');
 const { selector } = require('../../helpers/methods');
 const { zip } = require('../../helpers/iterate');
@@ -181,7 +182,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
       withHooks &&
         describe('with hook', function () {
           beforeEach(async function () {
-            await this.mockFromEntrypoint.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
+            await this.mock.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
           });
 
           it('should call the hook of the installed module when performing an module install', async function () {
@@ -267,7 +268,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
         const anotherInstance = await ethers.deployContract('$ERC7579ModuleMock', [MODULE_TYPE_FALLBACK]);
         const initData = '0x12345678abcdef';
 
-        await this.mockFromEntrypoint.$_installModule(MODULE_TYPE_FALLBACK, instance, initData);
+        await this.mock.$_installModule(MODULE_TYPE_FALLBACK, instance, initData);
         await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_FALLBACK, anotherInstance, initData))
           .to.be.revertedWithCustomError(this.mock, 'ERC7579UninstalledModule')
           .withArgs(MODULE_TYPE_FALLBACK, anotherInstance);
@@ -296,7 +297,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
       withHooks &&
         describe('with hook', function () {
           beforeEach(async function () {
-            await this.mockFromEntrypoint.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
+            await this.mock.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
           });
 
           it('should call the hook of the installed module when performing a module uninstall', async function () {
@@ -316,6 +317,84 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
               .to.emit(this.modules[MODULE_TYPE_HOOK], 'PostCheck')
               .withArgs(precheckData);
           });
+
+          it('can uninstall a hook module that reverts during its pre-check', async function () {
+            const instance = this.modules[MODULE_TYPE_HOOK];
+            const initData = ethers.hexlify(ethers.randomBytes(256));
+
+            // Set the hook to revert on preCheck
+            await instance.revertOnPreCheck(true);
+
+            // Should uninstall
+            await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_HOOK, instance, initData))
+              .to.emit(this.mock, 'ModuleUninstalled')
+              .withArgs(MODULE_TYPE_HOOK, instance)
+              .to.not.emit(instance, 'PreCheck')
+              .to.not.emit(instance, 'PostCheck');
+
+            await expect(this.mock.isModuleInstalled(MODULE_TYPE_HOOK, instance, initData)).to.eventually.equal(false);
+          });
+
+          it('can uninstall a hook module that reverts during its post-check', async function () {
+            const instance = this.modules[MODULE_TYPE_HOOK];
+            const initData = ethers.hexlify(ethers.randomBytes(256));
+
+            // Set the hook to revert on postCheck
+            await instance.revertOnPostCheck(true);
+
+            // Should uninstall
+            await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_HOOK, instance, initData))
+              .to.emit(this.mock, 'ModuleUninstalled')
+              .withArgs(MODULE_TYPE_HOOK, instance)
+              .to.emit(instance, 'PreCheck')
+              .withArgs(
+                predeploy.entrypoint.v09,
+                0n,
+                this.mock.interface.encodeFunctionData('uninstallModule', [
+                  MODULE_TYPE_HOOK,
+                  instance.target,
+                  initData,
+                ]),
+              )
+              .to.not.emit(instance, 'PostCheck');
+
+            await expect(this.mock.isModuleInstalled(MODULE_TYPE_HOOK, instance, initData)).to.eventually.equal(false);
+          });
+
+          it('can uninstall a hook module that reverts during both pre-check and post-check', async function () {
+            const instance = this.modules[MODULE_TYPE_HOOK];
+            const initData = ethers.hexlify(ethers.randomBytes(256));
+
+            // Set the hook to revert on preCheck and postCheck
+            await instance.revertOnPreCheck(true);
+            await instance.revertOnPostCheck(true);
+
+            // Should uninstall
+            await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_HOOK, instance, initData))
+              .to.emit(this.mock, 'ModuleUninstalled')
+              .withArgs(MODULE_TYPE_HOOK, instance)
+              .to.not.emit(instance, 'PreCheck')
+              .to.not.emit(instance, 'PostCheck');
+
+            await expect(this.mock.isModuleInstalled(MODULE_TYPE_HOOK, instance, initData)).to.eventually.equal(false);
+          });
+
+          it('can uninstall a hook module that has no code (removed delegation)', async function () {
+            const instance = this.modules[MODULE_TYPE_HOOK];
+            const initData = ethers.hexlify(ethers.randomBytes(256));
+
+            // Delete the code of the module to simulate a removed delegation
+            await setCode(instance.target, '0x');
+
+            // Should uninstall
+            await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_HOOK, instance, initData))
+              .to.emit(this.mock, 'ModuleUninstalled')
+              .withArgs(MODULE_TYPE_HOOK, instance)
+              .to.not.emit(instance, 'PreCheck')
+              .to.not.emit(instance, 'PostCheck');
+
+            await expect(this.mock.isModuleInstalled(MODULE_TYPE_HOOK, instance, initData)).to.eventually.equal(false);
+          });
         });
     });
 
@@ -515,7 +594,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
           withHooks &&
             describe('with hook', function () {
               beforeEach(async function () {
-                await this.mockFromEntrypoint.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
+                await this.mock.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
               });
 
               it(`should call the hook of the installed module when executing ${execFn}`, async function () {
@@ -596,7 +675,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
         withHooks &&
           describe('with hook', function () {
             beforeEach(async function () {
-              await this.mockFromEntrypoint.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
+              await this.mock.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x');
             });
 
             it('should call the hook of the installed module when performing a callback', async function () {