From 4af8827adce27eb164d37dbd32716261f5e0565c Mon Sep 17 00:00:00 2001 From: Adam Gall Date: Sun, 26 Oct 2025 16:41:32 -0400 Subject: [PATCH] fix: make ownership renouncement irreversible in LibOwner and LibOwnerTwoSteps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 #116 --- src/access/Owner/LibOwner.sol | 5 +++ src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol | 8 +++- test/access/Owner/LibOwner.t.sol | 39 +++++-------------- .../OwnerTwoSteps/LibOwnerTwoSteps.t.sol | 36 +++-------------- 4 files changed, 27 insertions(+), 61 deletions(-) diff --git a/src/access/Owner/LibOwner.sol b/src/access/Owner/LibOwner.sol index 332a291e..17eba061 100644 --- a/src/access/Owner/LibOwner.sol +++ b/src/access/Owner/LibOwner.sol @@ -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"); @@ -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); } diff --git a/src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol b/src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol index 8cf47608..3d4015b3 100644 --- a/src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol +++ b/src/access/OwnerTwoSteps/LibOwnerTwoSteps.sol @@ -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"); @@ -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. diff --git a/test/access/Owner/LibOwner.t.sol b/test/access/Owner/LibOwner.t.sol index 1416928d..68e2ca14 100644 --- a/test/access/Owner/LibOwner.t.sol +++ b/test/access/Owner/LibOwner.t.sol @@ -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); } // ============================================ @@ -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 { @@ -220,13 +206,7 @@ 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 @@ -234,10 +214,9 @@ contract LibOwnerTest is Test { 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); } // ============================================ diff --git a/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol b/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol index a063bcf5..f3e720c6 100644 --- a/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol +++ b/test/access/OwnerTwoSteps/LibOwnerTwoSteps.t.sol @@ -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 { @@ -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); } // ============================================