Fix pending owner late overwrite issue (#1122)

* fix: pending owner late overwrite issue * feat: add tests * tests: remove old version * feat: update CHANGELOG * Update packages/access/src/tests/test_ownable.cairo Co-authored-by: immrsd <[email protected]> * Update packages/access/src/tests/test_ownable_twostep.cairo Co-authored-by: immrsd <[email protected]> * feat: apply review updates * feat: update CHANGELOG --------- Co-authored-by: immrsd <[email protected]>
OpenZeppelin · Aug 30, 2024 · ef87d78 · ef87d78
1 parent 5ae6b40
commit ef87d78
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 70 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- ERC721Enumerable component (#983)
 - ERC2981 (NFT Royalty Standard) component (#1091)
 - `merkle_tree` package with utilities to verify proofs and multi proofs (#1101)
 
@@ -22,9 +23,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed (Breaking)
 
-- Changed ABI suffix to Trait in dual case account and eth account modules (#1096).
+- Changed ABI suffix to Trait in dual case account and eth account modules (#1096)
   - `DualCaseAccountABI` renamed to `DualCaseAccountTrait`
   - `DualCaseEthAccountABI` renamed to `DualCaseEthAccountTrait`
+- Removed `_accept_ownership` from `OwnableComponent::InternalImpl`
+
+### Fixed
+
+- `OwnableTwoStep` allowing a pending owner to accept ownership after the original owner has renounced ownership (#1119)
 
 ## 0.15.1 (2024-08-13)
 
@@ -37,7 +43,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- ERC721Enumerable component (#983)
 - TimelockController component (#996)
 - HashCall implementation (#996)
 - Separated package for each submodule (#1065)

diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc
@@ -89,7 +89,6 @@ This module includes the internal `assert_only_owner` to restrict a function to
 * xref:OwnableComponent-assert_only_owner[`++assert_only_owner(self)++`]
 * xref:OwnableComponent-_transfer_ownership[`++_transfer_ownership(self, new_owner)++`]
 * xref:OwnableComponent-_propose_owner[`++_propose_owner(self, new_owner)++`]
-* xref:OwnableComponent-_accept_ownership[`++_accept_ownership(self)++`]
 --
 
 [.contract-index]
@@ -239,17 +238,6 @@ Internal function without access restriction.
 
 Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event.
 
-[.contract-item]
-[[OwnableComponent-_accept_ownership]]
-==== `[.contract-item-name]#++_accept_ownership++#++(ref self: ContractState)++` [.item-kind]#internal#
-
-Transfers ownership to the pending owner. Resets pending owner to zero address.
-Calls xref:OwnableComponent-_transfer_ownership[_transfer_ownership].
-
-Internal function without access restriction.
-
-Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event.
-
 [#OwnableComponent-Events]
 ==== Events
 

diff --git a/packages/access/src/ownable/ownable.cairo b/packages/access/src/ownable/ownable.cairo
@@ -113,14 +113,26 @@ pub mod OwnableComponent {
 
         /// Finishes the two-step ownership transfer process by accepting the ownership.
         /// Can only be called by the pending owner.
+        ///
+        /// Requirements:
+        ///
+        /// - The caller is the pending owner.
+        ///
+        /// Emits an `OwnershipTransferred` event.
         fn accept_ownership(ref self: ComponentState<TContractState>) {
             let caller = get_caller_address();
             let pending_owner = self.Ownable_pending_owner.read();
             assert(caller == pending_owner, Errors::NOT_PENDING_OWNER);
-            self._accept_ownership();
+            self._transfer_ownership(pending_owner);
         }
 
         /// Starts the two-step ownership transfer process by setting the pending owner.
+        ///
+        /// Requirements:
+        ///
+        /// - The caller is the contract owner.
+        ///
+        /// Emits an `OwnershipTransferStarted` event.
         fn transfer_ownership(
             ref self: ComponentState<TContractState>, new_owner: ContractAddress
         ) {
@@ -130,6 +142,12 @@ pub mod OwnableComponent {
 
         /// Leaves the contract without owner. It will not be possible to call `assert_only_owner`
         /// functions anymore. Can only be called by the current owner.
+        ///
+        /// Requirements:
+        ///
+        /// - The caller is the contract owner.
+        ///
+        /// Emits an `OwnershipTransferred` event.
         fn renounce_ownership(ref self: ComponentState<TContractState>) {
             Ownable::renounce_ownership(ref self);
         }
@@ -265,14 +283,17 @@ pub mod OwnableComponent {
             assert(caller == owner, Errors::NOT_OWNER);
         }
 
-        /// Transfers ownership of the contract to a new address.
+        /// Transfers ownership of the contract to a new address and resets
+        /// the pending owner to the zero address.
         ///
         /// Internal function without access restriction.
         ///
         /// Emits an `OwnershipTransferred` event.
         fn _transfer_ownership(
             ref self: ComponentState<TContractState>, new_owner: ContractAddress
         ) {
+            self.Ownable_pending_owner.write(Zero::zero());
+
             let previous_owner: ContractAddress = self.Ownable_owner.read();
             self.Ownable_owner.write(new_owner);
             self
@@ -296,16 +317,5 @@ pub mod OwnableComponent {
                     }
                 );
         }
-
-        /// Transfers ownership to the pending owner.
-        ///
-        /// Internal function without access restriction.
-        ///
-        /// Emits an `OwnershipTransferred` event.
-        fn _accept_ownership(ref self: ComponentState<TContractState>) {
-            let pending_owner = self.Ownable_pending_owner.read();
-            self.Ownable_pending_owner.write(Zero::zero());
-            self._transfer_ownership(pending_owner);
-        }
     }
 }
diff --git a/packages/access/src/tests/test_ownable.cairo b/packages/access/src/tests/test_ownable.cairo
@@ -5,7 +5,7 @@ use crate::ownable::interface::{IOwnable, IOwnableCamelOnly};
 use crate::tests::mocks::ownable_mocks::DualCaseOwnableMock;
 
 use openzeppelin_test_common::ownable::OwnableSpyHelpers;
-use openzeppelin_testing::constants::{ZERO, OTHER, OWNER};
+use openzeppelin_testing::constants::{ZERO, OTHER, OWNER, RECIPIENT};
 use snforge_std::{spy_events, test_address, start_cheat_caller_address};
 
 //
@@ -86,6 +86,20 @@ fn test__transfer_ownership() {
     assert_eq!(current_owner, OTHER());
 }
 
+#[test]
+fn test__transfer_ownership_resets_pending_owner() {
+    let mut state = setup();
+
+    state.Ownable_pending_owner.write(OTHER());
+    let current_pending_owner = state.Ownable_pending_owner.read();
+    assert_eq!(current_pending_owner, OTHER());
+
+    state._transfer_ownership(RECIPIENT());
+
+    let current_pending_owner = state.Ownable_pending_owner.read();
+    assert!(current_pending_owner.is_zero());
+}
+
 //
 // transfer_ownership & transferOwnership
 //

diff --git a/packages/access/src/tests/test_ownable_twostep.cairo b/packages/access/src/tests/test_ownable_twostep.cairo
@@ -44,23 +44,6 @@ fn test_initializer_owner_pending_owner() {
     assert!(state.Ownable_pending_owner.read().is_zero());
 }
 
-//
-// _accept_ownership
-//
-
-#[test]
-fn test__accept_ownership() {
-    let mut state = setup();
-    let mut spy = spy_events();
-    state.Ownable_pending_owner.write(OTHER());
-
-    state._accept_ownership();
-
-    spy.assert_only_event_ownership_transferred(test_address(), OWNER(), OTHER());
-    assert_eq!(state.owner(), OTHER());
-    assert!(state.pending_owner().is_zero());
-}
-
 //
 // _propose_owner
 //
@@ -244,6 +227,22 @@ fn test_renounce_ownership() {
     assert!(state.owner().is_zero());
 }
 
+#[test]
+fn test_renounce_ownership_resets_pending_owner() {
+    let mut state = setup();
+    let contract_address = test_address();
+    start_cheat_caller_address(contract_address, OWNER());
+
+    state.Ownable_pending_owner.write(OTHER());
+    let current_pending_owner = state.Ownable_pending_owner.read();
+    assert_eq!(current_pending_owner, OTHER());
+
+    state.renounce_ownership();
+
+    let current_pending_owner = state.Ownable_pending_owner.read();
+    assert!(current_pending_owner.is_zero());
+}
+
 #[test]
 #[should_panic(expected: ('Caller is the zero address',))]
 fn test_renounce_ownership_from_zero_address() {
@@ -307,31 +306,6 @@ fn test_full_two_step_transfer() {
     assert!(state.pending_owner().is_zero());
 }
 
-#[test]
-fn test_pending_accept_after_owner_renounce() {
-    let mut state = setup();
-    let mut spy = spy_events();
-    let contract_address = test_address();
-    start_cheat_caller_address(contract_address, OWNER());
-    state.transfer_ownership(OTHER());
-
-    spy.assert_event_ownership_transfer_started(contract_address, OWNER(), OTHER());
-    assert_eq!(state.owner(), OWNER());
-    assert_eq!(state.pending_owner(), OTHER());
-
-    state.renounce_ownership();
-
-    spy.assert_only_event_ownership_transferred(contract_address, OWNER(), ZERO());
-    assert!(state.owner().is_zero());
-
-    start_cheat_caller_address(contract_address, OTHER());
-    state.accept_ownership();
-
-    spy.assert_only_event_ownership_transferred(contract_address, ZERO(), OTHER());
-    assert_eq!(state.owner(), OTHER());
-    assert!(state.pending_owner().is_zero());
-}
-
 //
 // Helpers
 //