From ef87d7847980e0cf83f4b7f3ff23e6590fb643ec Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Fri, 30 Aug 2024 11:51:17 +0200 Subject: [PATCH] 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 <103599616+immrsd@users.noreply.github.com> * Update packages/access/src/tests/test_ownable_twostep.cairo Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com> * feat: apply review updates * feat: update CHANGELOG --------- Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com> --- CHANGELOG.md | 9 ++- docs/modules/ROOT/pages/api/access.adoc | 12 ---- packages/access/src/ownable/ownable.cairo | 36 +++++++----- packages/access/src/tests/test_ownable.cairo | 16 ++++- .../src/tests/test_ownable_twostep.cairo | 58 +++++-------------- 5 files changed, 61 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5afb36f5e..425b28d28 100644 --- 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 index 693047cdc..b75e6d6cc 100644 --- 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 index 4d1f4fbe1..8ba3c228e 100644 --- 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) { 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, 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) { Ownable::renounce_ownership(ref self); } @@ -265,7 +283,8 @@ 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. /// @@ -273,6 +292,8 @@ pub mod OwnableComponent { fn _transfer_ownership( ref self: ComponentState, 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) { - 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 index 83078ffc6..8cd610ada 100644 --- 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 index e6177eea5..85f3baf4f 100644 --- 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 //