From cdc9477fd5818b1e9b9889a446d3205104f005b7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 10:07:28 +0100 Subject: [PATCH 01/11] Add a withHookNoRevert modifier and use it when uninstalling the hook module --- .../extensions/draft-AccountERC7579Hooked.sol | 45 ++++++++++-- .../mocks/account/modules/ERC7579Mock.sol | 13 ++++ .../extensions/AccountERC7579.behavior.js | 71 +++++++++++++++++-- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 532fb97cf65..44317152347 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/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,27 @@ 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)) { + try IERC7579Hook(hook_).preCheck(msg.sender, msg.value, msg.data) returns (bytes memory data) { + preCheckSuccess = true; + hookData = data; + } catch { + preCheckSuccess = false; + } + } + _; + if (hook_ != address(0) && preCheckSuccess) { + try IERC7579Hook(hook_).postCheck(hookData) {} catch {} + } + } + /// @inheritdoc AccountERC7579 function accountId() public view virtual override returns (string memory) { // vendorname.accountname.semver @@ -80,15 +102,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); } diff --git a/contracts/mocks/account/modules/ERC7579Mock.sol b/contracts/mocks/account/modules/ERC7579Mock.sol index 0c90013398c..ee6ccefc5b8 100644 --- a/contracts/mocks/account/modules/ERC7579Mock.sol +++ b/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); } } diff --git a/test/account/extensions/AccountERC7579.behavior.js b/test/account/extensions/AccountERC7579.behavior.js index 49953c4c214..2ec4efd000b 100644 --- a/test/account/extensions/AccountERC7579.behavior.js +++ b/test/account/extensions/AccountERC7579.behavior.js @@ -181,7 +181,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 +267,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 +296,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 +316,67 @@ 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); + }); }); }); @@ -515,7 +576,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 +657,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 () { From b390cd0ae66317e85cea4ab43be30b93893d839d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 10:09:30 +0100 Subject: [PATCH 02/11] Apply suggestion from @Amxx --- contracts/account/extensions/draft-AccountERC7579Hooked.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 44317152347..1329cc27461 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -6,7 +6,6 @@ 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). From 8ab30a594f35900eed299f854ccc5ac1bac10f83 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 10:18:06 +0100 Subject: [PATCH 03/11] use LowLevelCall to support de-delegated hook modules --- .../extensions/draft-AccountERC7579Hooked.sol | 14 ++++++++------ .../extensions/AccountERC7579.behavior.js | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 1329cc27461..f5df4573f57 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/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). @@ -47,16 +48,17 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { // slither-disable-next-line reentrancy-no-eth if (hook_ != address(0)) { - try IERC7579Hook(hook_).preCheck(msg.sender, msg.value, msg.data) returns (bytes memory data) { - preCheckSuccess = true; - hookData = data; - } catch { - preCheckSuccess = false; + preCheckSuccess = LowLevelCall.callNoReturn( + hook_, + abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data)) + ); + if (preCheckSuccess) { + hookData = LowLevelCall.returnData(); } } _; if (hook_ != address(0) && preCheckSuccess) { - try IERC7579Hook(hook_).postCheck(hookData) {} catch {} + LowLevelCall.callNoReturn(hook_, abi.encodeCall(IERC7579Hook.postCheck, (hookData))); } } diff --git a/test/account/extensions/AccountERC7579.behavior.js b/test/account/extensions/AccountERC7579.behavior.js index 2ec4efd000b..051e7dead21 100644 --- a/test/account/extensions/AccountERC7579.behavior.js +++ b/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'); @@ -377,6 +378,23 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) { 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); + }); }); }); From dd6a447492c1b1e0d45df1ffd63b7f84c9aa0452 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 10:49:10 +0100 Subject: [PATCH 04/11] add changeset --- .changeset/chatty-dryers-joke.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/chatty-dryers-joke.md diff --git a/.changeset/chatty-dryers-joke.md b/.changeset/chatty-dryers-joke.md new file mode 100644 index 00000000000..f725dd4fec9 --- /dev/null +++ b/.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. From 841387b391f3b9ad42132e1149ef767213c4fd64 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 17:07:31 +0100 Subject: [PATCH 05/11] refactor (test fail) --- .../extensions/draft-AccountERC7579Hooked.sol | 72 ++++++++++--------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index f5df4573f57..b285f66b06c 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -40,28 +40,6 @@ 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 @@ -104,24 +82,48 @@ 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 { + // Inline a variant of the `withHook` modifier that doesn't revert if the hook reverts and the moduleTypeId is `MODULE_TYPE_HOOK`. + + // === Beginning of the precheck === + + 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(); + } else if (moduleTypeId != MODULE_TYPE_HOOK) { + LowLevelCall.bubbleRevert(); + } + } + + // === End of the precheck -- Beginning of the body (`_` part of the modifier) === + if (moduleTypeId == MODULE_TYPE_HOOK) { - // 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); + require(_hook == module, ERC7579Utils.ERC7579UninstalledModule(moduleTypeId, module)); + _hook = address(0); } - } + super._uninstallModule(moduleTypeId, module, deInitData); - function _uninstallHookModule(address module, bytes memory deInitData) private withHookNoRevert { - require(_hook == module, ERC7579Utils.ERC7579UninstalledModule(MODULE_TYPE_HOOK, module)); - _hook = address(0); + // === End of the body (`_` part of the modifier) -- Beginning of the postcheck === - super._uninstallModule(MODULE_TYPE_HOOK, module, deInitData); - } + if (hook_ != address(0) && preCheckSuccess) { + bool postCheckSuccess = LowLevelCall.callNoReturn( + hook_, + abi.encodeCall(IERC7579Hook.postCheck, (hookData)) + ); + if (!postCheckSuccess && moduleTypeId != MODULE_TYPE_HOOK) { + LowLevelCall.bubbleRevert(); + } + } - function _uninstallOtherModule(uint256 moduleTypeId, address module, bytes memory deInitData) private withHook { - super._uninstallModule(moduleTypeId, module, deInitData); + // === End of the postcheck === } /// @dev Hooked version of {AccountERC7579-_execute}. From 9c17ca60c7f5cb2273099b015a54e30f70782f41 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 17:22:59 +0100 Subject: [PATCH 06/11] update --- contracts/account/extensions/draft-AccountERC7579Hooked.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index b285f66b06c..45cf1d16e95 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -97,7 +97,8 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data)) ); if (preCheckSuccess) { - hookData = LowLevelCall.returnData(); + // Note: this can revert, and we won't be able to catch it. If could be leveraged by a malicious hook to force a revert. + hookData = abi.decode(LowLevelCall.returnData(), (bytes)); } else if (moduleTypeId != MODULE_TYPE_HOOK) { LowLevelCall.bubbleRevert(); } From 94884f9ea1c808c89bbfd5aeefe76d7079f7f549 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 17:32:48 +0100 Subject: [PATCH 07/11] manually abi decode to avoid possible revert --- .../extensions/draft-AccountERC7579Hooked.sol | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 45cf1d16e95..05f6344bd4c 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/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 {Bytes} from "../../utils/Bytes.sol"; import {LowLevelCall} from "../../utils/LowLevelCall.sol"; /** @@ -97,8 +98,16 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data)) ); if (preCheckSuccess) { - // Note: this can revert, and we won't be able to catch it. If could be leveraged by a malicious hook to force a revert. - hookData = abi.decode(LowLevelCall.returnData(), (bytes)); + // Note: abi.decode could revert, and we wouldn't be able to catch it. + // If could be leveraged by a malicious hook to force a revert. + // So we have to do the decode manually. + hookData = LowLevelCall.returnData(); + if (hookData.length < 0x20) preCheckSuccess = false; + uint256 offset = uint256(_unsafeReadBytesOffset(hookData, 0)); + if (hookData.length < 0x20 + offset) preCheckSuccess = false; + uint256 length = uint256(_unsafeReadBytesOffset(hookData, offset)); + if (hookData.length < 0x20 + offset + length) preCheckSuccess = false; + Bytes.splice(hookData, 0x20 + offset, 0x20 + offset + length); } else if (moduleTypeId != MODULE_TYPE_HOOK) { LowLevelCall.bubbleRevert(); } @@ -139,4 +148,12 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { function _fallback() internal virtual override withHook returns (bytes memory) { return super._fallback(); } + + /// @dev Copied from Bytes.sol + function _unsafeReadBytesOffset(bytes memory buffer, uint256 offset) private pure returns (bytes32 value) { + // This is not memory safe in the general case, but all calls to this private function are within bounds. + assembly ("memory-safe") { + value := mload(add(add(buffer, 0x20), offset)) + } + } } From f9164bddba1ada029aa6733745050e66981a8d51 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 18:21:15 +0100 Subject: [PATCH 08/11] add helper function --- .../extensions/draft-AccountERC7579Hooked.sol | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 05f6344bd4c..18bba3d71cc 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -98,16 +98,11 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data)) ); if (preCheckSuccess) { + hookData = LowLevelCall.returnData(); // Note: abi.decode could revert, and we wouldn't be able to catch it. // If could be leveraged by a malicious hook to force a revert. // So we have to do the decode manually. - hookData = LowLevelCall.returnData(); - if (hookData.length < 0x20) preCheckSuccess = false; - uint256 offset = uint256(_unsafeReadBytesOffset(hookData, 0)); - if (hookData.length < 0x20 + offset) preCheckSuccess = false; - uint256 length = uint256(_unsafeReadBytesOffset(hookData, offset)); - if (hookData.length < 0x20 + offset + length) preCheckSuccess = false; - Bytes.splice(hookData, 0x20 + offset, 0x20 + offset + length); + preCheckSuccess = _tryInPlaceAbiDecodeBytes(hookData); } else if (moduleTypeId != MODULE_TYPE_HOOK) { LowLevelCall.bubbleRevert(); } @@ -149,6 +144,16 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { return super._fallback(); } + function _tryInPlaceAbiDecodeBytes(bytes memory data) private pure returns (bool success) { + if (data.length < 0x20) return false; + uint256 offset = uint256(_unsafeReadBytesOffset(data, 0)); + if (data.length < 0x20 + offset) return false; + uint256 length = uint256(_unsafeReadBytesOffset(data, offset)); + if (data.length < 0x20 + offset + length) return false; + Bytes.splice(data, 0x20 + offset, 0x20 + offset + length); + return true; + } + /// @dev Copied from Bytes.sol function _unsafeReadBytesOffset(bytes memory buffer, uint256 offset) private pure returns (bytes32 value) { // This is not memory safe in the general case, but all calls to this private function are within bounds. From 47ae554e40452a36bcfc9ca2daa4aa497ce43963 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 18:24:52 +0100 Subject: [PATCH 09/11] update and natspec doc --- .../extensions/draft-AccountERC7579Hooked.sol | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 18bba3d71cc..7372ba22b23 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -98,11 +98,10 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { abi.encodeCall(IERC7579Hook.preCheck, (msg.sender, msg.value, msg.data)) ); if (preCheckSuccess) { - hookData = LowLevelCall.returnData(); // Note: abi.decode could revert, and we wouldn't be able to catch it. // If could be leveraged by a malicious hook to force a revert. // So we have to do the decode manually. - preCheckSuccess = _tryInPlaceAbiDecodeBytes(hookData); + (preCheckSuccess, hookData) = _tryInPlaceAbiDecodeBytes(LowLevelCall.returnData()); } else if (moduleTypeId != MODULE_TYPE_HOOK) { LowLevelCall.bubbleRevert(); } @@ -144,14 +143,18 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { return super._fallback(); } - function _tryInPlaceAbiDecodeBytes(bytes memory data) private pure returns (bool success) { - if (data.length < 0x20) return false; + /** + * @dev Try to abi.decode a bytes array. If successfull, the decoding is done in place, overriding the original + * data. If decoding fails, the original data is left untouched. + */ + function _tryInPlaceAbiDecodeBytes(bytes memory data) private pure returns (bool success, bytes memory passtrough) { + if (data.length < 0x20) return (false, data); uint256 offset = uint256(_unsafeReadBytesOffset(data, 0)); - if (data.length < 0x20 + offset) return false; + if (data.length < 0x20 + offset) return (false, data); uint256 length = uint256(_unsafeReadBytesOffset(data, offset)); - if (data.length < 0x20 + offset + length) return false; + if (data.length < 0x20 + offset + length) return (false, data); Bytes.splice(data, 0x20 + offset, 0x20 + offset + length); - return true; + return (true, data); } /// @dev Copied from Bytes.sol From 0cef8e1f8225b2a3b2fa62e26efe27ce825efb77 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Mar 2026 21:28:07 +0100 Subject: [PATCH 10/11] codespell --- .../extensions/draft-AccountERC7579Hooked.sol | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/contracts/account/extensions/draft-AccountERC7579Hooked.sol b/contracts/account/extensions/draft-AccountERC7579Hooked.sol index 7372ba22b23..98f61363114 100644 --- a/contracts/account/extensions/draft-AccountERC7579Hooked.sol +++ b/contracts/account/extensions/draft-AccountERC7579Hooked.sol @@ -144,17 +144,21 @@ abstract contract AccountERC7579Hooked is AccountERC7579 { } /** - * @dev Try to abi.decode a bytes array. If successfull, the decoding is done in place, overriding the original + * @dev Try to abi.decode a bytes array. If successful, the decoding is done in place, overriding the original * data. If decoding fails, the original data is left untouched. */ - function _tryInPlaceAbiDecodeBytes(bytes memory data) private pure returns (bool success, bytes memory passtrough) { - if (data.length < 0x20) return (false, data); - uint256 offset = uint256(_unsafeReadBytesOffset(data, 0)); - if (data.length < 0x20 + offset) return (false, data); - uint256 length = uint256(_unsafeReadBytesOffset(data, offset)); - if (data.length < 0x20 + offset + length) return (false, data); - Bytes.splice(data, 0x20 + offset, 0x20 + offset + length); - return (true, data); + function _tryInPlaceAbiDecodeBytes( + bytes memory data + ) private pure returns (bool success, bytes memory passthrough) { + unchecked { + if (data.length < 0x20) return (false, data); + uint256 offset = uint256(_unsafeReadBytesOffset(data, 0)); + if (data.length - 0x20 < offset) return (false, data); + uint256 length = uint256(_unsafeReadBytesOffset(data, offset)); + if (data.length - 0x20 - offset < length) return (false, data); + Bytes.splice(data, 0x20 + offset, 0x20 + offset + length); + return (true, data); + } } /// @dev Copied from Bytes.sol From 4998f55170a0c8717582d5ec466716a3cbd33518 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 5 Mar 2026 17:03:14 +0100 Subject: [PATCH 11/11] coverage --- .../extensions/AccountERC7579.behavior.js | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/account/extensions/AccountERC7579.behavior.js b/test/account/extensions/AccountERC7579.behavior.js index 051e7dead21..94002158f96 100644 --- a/test/account/extensions/AccountERC7579.behavior.js +++ b/test/account/extensions/AccountERC7579.behavior.js @@ -297,6 +297,7 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) { withHooks && describe('with hook', function () { beforeEach(async function () { + await this.mock.$_installModule(MODULE_TYPE_EXECUTOR, this.modules[MODULE_TYPE_EXECUTOR], '0x'); await this.mock.$_installModule(MODULE_TYPE_HOOK, this.modules[MODULE_TYPE_HOOK], '0x'); }); @@ -310,7 +311,6 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) { initData, ]); - await this.mock.$_installModule(MODULE_TYPE_EXECUTOR, instance, initData); await expect(this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_EXECUTOR, instance, initData)) .to.emit(this.modules[MODULE_TYPE_HOOK], 'PreCheck') .withArgs(predeploy.entrypoint.v09, 0n, precheckData) @@ -318,6 +318,30 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) { .withArgs(precheckData); }); + it('hook revert during the pre-check prevents uninstalling a non-hook module', async function () { + const instance = this.modules[MODULE_TYPE_EXECUTOR]; + const initData = ethers.hexlify(ethers.randomBytes(256)); + + // Set the hook to revert on preCheck + await this.modules[MODULE_TYPE_HOOK].revertOnPreCheck(true); + + await expect( + this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_EXECUTOR, instance, initData), + ).to.be.revertedWith('preCheck reverts'); + }); + + it('hook revert during the post-check prevents uninstalling a non-hook module', async function () { + const instance = this.modules[MODULE_TYPE_EXECUTOR]; + const initData = ethers.hexlify(ethers.randomBytes(256)); + + // Set the hook to revert on postCheck + await this.modules[MODULE_TYPE_HOOK].revertOnPostCheck(true); + + await expect( + this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_EXECUTOR, instance, initData), + ).to.be.revertedWith('postCheck reverts'); + }); + 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));