fix: make ownership renouncement irreversible in LibOwner and LibOwnerTwoSteps#127
Merged
Merged
Conversation
…rTwoSteps ## Problem Previously, the libraries allowed `transferOwnership()` to be called even after the owner was set to `address(0)` through renouncement. This meant ownership renouncement was not truly permanent and could be reversed, violating the expected behavior. ## Solution Added `OwnerAlreadyRenounced` error and validation in both libraries: - `LibOwner.transferOwnership()` now reverts if `previousOwner == address(0)` - `LibOwnerTwoSteps.transferOwnership()` now reverts if `previousOwner == address(0)` ## Changes - Added `error OwnerAlreadyRenounced()` to both LibOwner and LibOwnerTwoSteps - Added renouncement checks in transferOwnership() functions - Updated 5 tests to expect reverts when attempting transfers from renounced owner ## Testing All 122 tests in the access control module pass: - OwnerFacet: 19 tests ✓ - LibOwner: 25 tests ✓ - LibOwnerTwoSteps: 35 tests ✓ - OwnerTwoStepsFacet: 43 tests ✓ ## Impact This is a breaking change for any code that relied on the ability to transfer ownership after renouncement. However, this was incorrect behavior that needed to be fixed for security. Fixes Perfect-Abstractions#116
Contributor
Coverage Report
Last updated: Sun, 26 Oct 2025 20:42:22 GMT for commit |
Contributor
|
Yes, thanks! |
JackieXu
pushed a commit
to JackieXu/Compose
that referenced
this pull request
Nov 6, 2025
fix: make ownership renouncement irreversible in LibOwner and LibOwnerTwoSteps
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
2 participants
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.
Problem
Previously, the libraries allowed
transferOwnership()to be called even after the owner was set toaddress(0)through renouncement. This meant ownership renouncement was not truly permanent and could be reversed, violating the expected behavior.Solution
Added
OwnerAlreadyRenouncederror and validation in both libraries:LibOwner.transferOwnership()now reverts ifpreviousOwner == address(0)LibOwnerTwoSteps.transferOwnership()now reverts ifpreviousOwner == address(0)Changes
error OwnerAlreadyRenounced()to both LibOwner and LibOwnerTwoStepsTesting
All 122 tests in the access control module pass:
Impact
This is a breaking change for any code that relied on the ability to transfer ownership after renouncement. However, this was incorrect behavior that needed to be fixed for security.
Fixes #116
Fixes #46