Add AccountERC7579Guarded extension - #6647
Draft
james-toussaint wants to merge 5 commits into
Draft
Conversation
PR OpenZeppelin#6390 special-cased hook-module uninstallation so that a bugged or malicious hook could not block its own removal by reverting in preCheck/postCheck. This reverts that behavior, giving the hook module full control over its lifecycle: uninstalling a hook module now goes through the standard withHook flow, so the hook can prevent its own uninstallation by reverting. OpenZeppelin#6390 was never released, so its pending changeset is dropped and no new changeset is required.
PR OpenZeppelin#6142 (released in 5.6.0) swallowed reverts from a module's onUninstall hook so a buggy or malicious module could not block its own removal. This reverts that behavior: onUninstall is called without catching reverts, so a module can now prevent its own uninstallation by reverting. A forced uninstallation that bypasses the onUninstall hook can still be performed through a delegate call via execute; document this in the _uninstallModule NatSpec.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: f861252 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the ERC-7579 account framework by introducing a guarded “escape hatch” for forced module removal, while also reverting earlier behavior that prevented modules (including hook modules) from blocking their own uninstallation.
Changes:
- Make
AccountERC7579propagateonUninstallreverts and factor storage-only removal into a new_removeModuleprimitive. - Simplify
AccountERC7579Hookeduninstallation logic (restore plainwithHookbehavior) and override_removeModuleto support hook-module removal without hook wrapping. - Add
AccountERC7579Guardedwith a guardian-gatedforceUninstallthat bypasses hooks andonUninstall.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/account/extensions/AccountERC7579.behavior.js |
Updates behavior tests to expect uninstall to revert when onUninstall reverts; adjusts hook-uninstall expectations accordingly. |
contracts/account/extensions/draft-AccountERC7579.sol |
Introduces _removeModule and changes _uninstallModule to call onUninstall without swallowing reverts. |
contracts/account/extensions/draft-AccountERC7579Hooked.sol |
Removes the special-cased “ignore hook failures during hook uninstall” logic; adds a hook-aware _removeModule override. |
contracts/account/extensions/draft-AccountERC7579Guarded.sol |
New extension exposing a guardian-gated forceUninstall escape hatch built on _removeModule. |
.changeset/lazy-hooks-return.md |
Patch changeset documenting the uninstall revert behavior change in AccountERC7579. |
.changeset/guarded-accounts-recover.md |
Minor changeset documenting the new AccountERC7579Guarded extension. |
.changeset/chatty-dryers-joke.md |
Removes an obsolete changeset related to the previously reverted hooked-uninstall behavior. |
Comments suppressed due to low confidence (1)
contracts/account/extensions/draft-AccountERC7579Guarded.sol:85
_setGuardiancurrently allows overwriting an existing guardian and setting it toaddress(0), which contradicts the contract-level rationale (guardian set once, delay cannot be bypassed) and can brickforceUninstall. Enforce the set-once + nonzero invariant.
function _setGuardian(address newGuardian) internal virtual {
_guardian = newGuardian;
emit UninstallGuardianSet(newGuardian);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,91 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Contracts (account/extensions/draft-AccountERC7579Guarded.sol) | |||
Comment on lines
+50
to
+52
| /// @dev The caller is not the configured guardian. | ||
| error AccountUnauthorizedGuardian(address caller); | ||
|
|
| * | ||
| * Recovering from a bricked hook then goes through the guardian's own delay: | ||
| * ```solidity | ||
| * bytes memory call = abi.encodeCall(AccountERC7579Guarded.forceUninstall, (MODULE_TYPE_HOOK, hook, "")); |
Comment on lines
+64
to
+71
| function forceUninstall( | ||
| uint256 moduleTypeId, | ||
| address module, | ||
| bytes calldata deInitData | ||
| ) public virtual onlyGuardian { | ||
| _removeModule(moduleTypeId, module, deInitData); | ||
| emit ModuleUninstalled(moduleTypeId, module); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6628.
PR Checklist
npx changeset add)