Skip to content

fix: make ownership renouncement irreversible in LibOwner and LibOwnerTwoSteps #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mudgen merged 1 commit into from
Oct 26, 2025
Merged

fix: make ownership renouncement irreversible in LibOwner and LibOwnerTwoSteps #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/access/Owner/LibOwner.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ library LibOwner {

/// @notice Thrown when a non-owner attempts an action restricted to owner.
error OwnerUnauthorizedAccount();
/// @notice Thrown when attempting to transfer ownership from a renounced state.
error OwnerAlreadyRenounced();

bytes32 constant STORAGE_POSITION = keccak256("compose.owner");

Expand Down Expand Up @@ -46,6 +48,9 @@ library LibOwner {
function transferOwnership(address _newOwner) internal {
OwnerStorage storage s = getStorage();
address previousOwner = s.owner;
if (previousOwner == address(0)) {
revert OwnerAlreadyRenounced();
}
s.owner = _newOwner;
emit OwnershipTransferred(previousOwner, _newOwner);
}
Expand Down
8 changes: 7 additions & 1 deletion src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ library LibOwnerTwoSteps {

/// @notice Thrown when a non-owner attempts an action restricted to owner.
error OwnerUnauthorizedAccount();
/// @notice Thrown when attempting to transfer ownership from a renounced state.
error OwnerAlreadyRenounced();

bytes32 constant STORAGE_POSITION = keccak256("compose.owner");

Expand Down Expand Up @@ -51,8 +53,12 @@ library LibOwnerTwoSteps {
/// @param _newOwner The address of the new owner of the contract
function transferOwnership(address _newOwner) internal {
OwnerTwoStepsStorage storage s = getStorage();
address previousOwner = s.owner;
if (previousOwner == address(0)) {
revert OwnerAlreadyRenounced();
}
s.pendingOwner = _newOwner;
emit OwnershipTransferStarted(s.owner, _newOwner);
emit OwnershipTransferStarted(previousOwner, _newOwner);
}

/// @notice Finalizes ownership transfer; must be called by the pending owner.
Expand Down
39 changes: 9 additions & 30 deletions test/access/Owner/LibOwner.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,21 +94,14 @@ contract LibOwnerTest is Test {
assertEq(harness.owner(), INITIAL_OWNER);
}

// TODO: When LibOwner is fixed to make renouncement irreversible:
// 1. Rename this test to: test_RevertWhen_TransferOwnership_FromRenouncedOwner
// 2. Change logic to:
// vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
// harness.transferOwnership(NEW_OWNER);
// 3. Remove the assertions for successful transfer
function test_TransferOwnership_AfterRenounce_AllowsNewOwner() public {
function test_RevertWhen_TransferOwnership_FromRenouncedOwner() public {
// Force renounce
harness.forceRenounce();
assertEq(harness.owner(), ZERO_ADDRESS);

// CURRENT BEHAVIOR (BUG): Library allows transferOwnership after renouncement
// EXPECTED BEHAVIOR: Should revert with OwnerAlreadyRenounced error
// Should revert with OwnerAlreadyRenounced error
vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
harness.transferOwnership(NEW_OWNER);
assertEq(harness.owner(), NEW_OWNER);
}

// ============================================
Expand Down Expand Up @@ -162,22 +155,15 @@ contract LibOwnerTest is Test {
// Edge Cases
// ============================================

// TODO: When LibOwner is fixed to make renouncement irreversible:
// 1. Rename this test to: test_RenounceOwnership_PermanentlyDisablesTransfers
// 2. Change logic to:
// vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
// harness.transferOwnership(ALICE);
// 3. Remove the assertion for successful transfer
function test_RenounceOwnership_AllowsRecovery() public {
function test_RenounceOwnership_PermanentlyDisablesTransfers() public {
// Renounce ownership
vm.prank(INITIAL_OWNER);
harness.transferOwnership(ZERO_ADDRESS);
assertEq(harness.owner(), ZERO_ADDRESS);

// CURRENT BEHAVIOR (BUG): Library allows recovery after renouncement
// EXPECTED BEHAVIOR: Should revert with OwnerAlreadyRenounced error
// Should revert with OwnerAlreadyRenounced error
vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
harness.transferOwnership(ALICE);
assertEq(harness.owner(), ALICE);
}

function test_LibraryDoesNotCheckMsgSender() public {
Expand Down Expand Up @@ -220,24 +206,17 @@ contract LibOwnerTest is Test {
assertEq(harness.owner(), owner3);
}

// TODO: When LibOwner is fixed to make renouncement irreversible:
// 1. Rename this test to: test_Fuzz_RevertWhen_RenouncedOwnerTransfers
// 2. Change logic to:
// vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
// harness.transferOwnership(target);
// 3. Remove the assertion for successful transfer
function test_Fuzz_TransferAfterRenounce_AllowsRecovery(address target) public {
function test_Fuzz_RevertWhen_RenouncedOwnerTransfers(address target) public {
vm.assume(target != address(0));

// Renounce
vm.prank(INITIAL_OWNER);
harness.transferOwnership(ZERO_ADDRESS);
assertEq(harness.owner(), ZERO_ADDRESS);

// CURRENT BEHAVIOR (BUG): Library allows recovery - can transfer to new owner
// EXPECTED BEHAVIOR: Should revert with OwnerAlreadyRenounced error
// Should revert with OwnerAlreadyRenounced error
vm.expectRevert(LibOwner.OwnerAlreadyRenounced.selector);
harness.transferOwnership(target);
assertEq(harness.owner(), target);
}

// ============================================
Expand Down
36 changes: 6 additions & 30 deletions test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,26 +153,14 @@ contract LibOwnerTwoStepsTest is Test {
assertEq(harness.owner(), INITIAL_OWNER); // Owner unchanged until accepted
}

// TODO: When LibOwnerTwoSteps is fixed to make renouncement irreversible:
// 1. Rename this test to: test_RevertWhen_TransferOwnership_FromRenouncedOwner
// 2. Change logic to:
// vm.expectRevert(LibOwnerTwoSteps.OwnerAlreadyRenounced.selector);
// harness.transferOwnership(NEW_OWNER);
// 3. Remove the acceptance and recovery logic
function test_TransferOwnership_AfterRenounce_AllowsRecovery() public {
function test_RevertWhen_TransferOwnership_FromRenouncedOwner() public {
// Force renounce
harness.forceRenounce();
assertEq(harness.owner(), ZERO_ADDRESS);

// CURRENT BEHAVIOR (BUG): Library allows setting pending owner even after renouncement
// EXPECTED BEHAVIOR: Should revert with OwnerAlreadyRenounced error
// Should revert with OwnerAlreadyRenounced error
vm.expectRevert(LibOwnerTwoSteps.OwnerAlreadyRenounced.selector);
harness.transferOwnership(NEW_OWNER);
assertEq(harness.pendingOwner(), NEW_OWNER);

// Can even accept ownership to recover (this shouldn't be possible)
vm.prank(NEW_OWNER);
harness.acceptOwnership();
assertEq(harness.owner(), NEW_OWNER);
}

function test_TransferOwnership_FromPendingOwner_Allowed() public {
Expand Down Expand Up @@ -278,25 +266,13 @@ contract LibOwnerTwoStepsTest is Test {
// Owner remains unchanged
}

// TODO: When LibOwnerTwoSteps is fixed to make renouncement irreversible:
// 1. Rename this test to: test_RenounceOwnership_PreventsNewTransfersAfterForceRenounce
// 2. Change logic to:
// vm.expectRevert(LibOwnerTwoSteps.OwnerAlreadyRenounced.selector);
// harness.transferOwnership(ALICE);
// 3. Remove the acceptance and recovery logic
function test_RenounceOwnership_AllowsRecoveryAfterForceRenounce() public {
function test_RenounceOwnership_PreventsNewTransfersAfterForceRenounce() public {
harness.forceRenounce();
assertEq(harness.owner(), ZERO_ADDRESS);

// CURRENT BEHAVIOR (BUG): Library allows transferOwnership after renouncement
// EXPECTED BEHAVIOR: Should revert with OwnerAlreadyRenounced error
// Should revert with OwnerAlreadyRenounced error
vm.expectRevert(LibOwnerTwoSteps.OwnerAlreadyRenounced.selector);
harness.transferOwnership(ALICE);
assertEq(harness.pendingOwner(), ALICE);

// Can recover ownership (this shouldn't be possible)
vm.prank(ALICE);
harness.acceptOwnership();
assertEq(harness.owner(), ALICE);
}

// ============================================
Expand Down