Fix pending owner late overwrite issue

CHANGELOG.md

@@ -23,6 +23,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - `DualCaseAccountABI` renamed to `DualCaseAccountTrait`
   - `DualCaseEthAccountABI` renamed to `DualCaseEthAccountTrait`
 
+### Fixed
+
+- `OwnableTwoStep` allowing a pending owner to accept ownership after the original owner has renounced ownership (#1119)
+
 ## 0.15.1 (2024-08-13)
 
 ### Changed

packages/access/src/ownable/ownable.cairo

@@ -273,6 +273,8 @@ pub mod OwnableComponent {
         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

packages/access/src/tests/test_ownable.cairo

@@ -5,7 +5,7 @@ use openzeppelin_access::ownable::interface::{IOwnable, IOwnableCamelOnly};
 use openzeppelin_access::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_owner = state.Ownable_pending_owner.read();
+    assert!(current_owner.is_zero());
+}
+
 //
 // transfer_ownership & transferOwnership
 //

packages/access/src/tests/test_ownable_twostep.cairo

@@ -244,6 +244,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_owner = state.Ownable_pending_owner.read();
+    assert!(current_owner.is_zero());
+}
+
 #[test]
 #[should_panic(expected: ('Caller is the zero address',))]
 fn test_renounce_ownership_from_zero_address() {
@@ -307,31 +323,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
 //