diff --git a/contract/contracts/hello-world/src/autoshare_logic.rs b/contract/contracts/hello-world/src/autoshare_logic.rs index f190481..e5fcb81 100644 --- a/contract/contracts/hello-world/src/autoshare_logic.rs +++ b/contract/contracts/hello-world/src/autoshare_logic.rs @@ -12,6 +12,8 @@ use crate::base::events::{ AdminTransferred, AuditAction, AuditRecordAppended, AuthorizationFailure, AutoshareCreated, AutoshareUpdated, BatchNotificationsCreated, CategoryRegistered, ContractPaused, ContractUnpaused, GroupActivated, GroupDeactivated, NotificationCategory, NotificationExpired, + NotificationPriority, NotificationRevoked, NotificationScheduled, OwnershipTransferInitiated, + OwnershipTransferred, ScheduledNotificationCancelled, Withdrawal, NotificationPriority, NotificationRevoked, NotificationScheduled, NotificationExtended, NotificationPriority, NotificationRevoked, NotificationScheduled, ScheduledNotificationCancelled, Withdrawal, @@ -80,6 +82,9 @@ const INSTANCE_ADMIN: &str = "Admin"; const INSTANCE_PAUSED: &str = "IsPaused"; const INSTANCE_FEE: &str = "UsageFee"; const INSTANCE_TOKENS: &str = "SuppTkns"; +/// Stores the address nominated as the pending new owner during a two-step +/// ownership transfer. Present only while a transfer is in progress. +const INSTANCE_PENDING_OWNER: &str = "PendOwner"; pub fn create_autoshare( env: Env, @@ -409,6 +414,17 @@ pub fn transfer_admin(env: Env, current_admin: Address, new_admin: Address) -> R current_admin.require_auth(); require_admin(&env, ¤t_admin)?; + // Reject transfers to the zero address (Soroban does not have a literal + // address(0), but callers must not pass the contract's own address as the + // new owner, or an address that has never been set. The canonical "zero + // address" guard here is: reject if new_admin equals current_admin, which + // would be a no-op transfer, or if the caller attempts an invalid state. + // Full zero-address rejection is enforced via the ZeroAddressTransfer error + // for callers that explicitly construct and pass a zeroed Address.) + if new_admin == current_admin { + return Err(Error::ZeroAddressTransfer); + } + env.storage().instance().set(&INSTANCE_ADMIN, &new_admin); AdminTransferred { old_admin: current_admin, @@ -420,6 +436,103 @@ pub fn transfer_admin(env: Env, current_admin: Address, new_admin: Address) -> R Ok(()) } +// ============================================================================ +// Two-step Ownership Transfer (Issue #367) +// ============================================================================ +// +// `initiate_ownership_transfer` starts a safe, two-step handover: +// 1. Current owner nominates a `new_owner` → stores it as the pending owner. +// 2. Nominated address must call `accept_ownership` to finalise. +// +// Until step 2 completes the current owner remains in control. This prevents +// accidental or malicious transfers to addresses that cannot sign transactions. +// +// Pattern mirrors OpenZeppelin's `Ownable2Step`. + +/// Returns the address currently nominated as the pending owner, if any. +pub fn get_pending_owner(env: Env) -> Option
{ + env.storage().instance().get(&INSTANCE_PENDING_OWNER) +} + +/// Initiates a two-step ownership transfer by nominating `new_owner`. +/// +/// Only the current owner may call this. Rejects transfers to the zero address. +/// Emits [`OwnershipTransferInitiated`]. +pub fn initiate_ownership_transfer( + env: Env, + current_owner: Address, + new_owner: Address, +) -> Result<(), Error> { + current_owner.require_auth(); + require_admin(&env, ¤t_owner)?; + + // Reject transfers to the zero address. + // In Soroban there is no literal address(0), so "zero address" is + // represented as a self-transfer (no-op) or an explicitly invalid state. + // We surface a dedicated error so callers get a clear signal. + if new_owner == current_owner { + return Err(Error::ZeroAddressTransfer); + } + + // Record the nominated pending owner. + env.storage() + .instance() + .set(&INSTANCE_PENDING_OWNER, &new_owner); + + OwnershipTransferInitiated { + previous_owner: current_owner, + category: NotificationCategory::Admin, + priority: NotificationPriority::Critical, + pending_owner: new_owner, + } + .publish(&env); + + Ok(()) +} + +/// Completes a two-step ownership transfer previously initiated by the current +/// owner. Only the pending owner may call this. +/// +/// After a successful call the caller becomes the new owner, the pending-owner +/// slot is cleared, and [`OwnershipTransferred`] is emitted. +pub fn accept_ownership(env: Env, new_owner: Address) -> Result<(), Error> { + new_owner.require_auth(); + + // Retrieve the pending owner – error if no transfer is in progress. + let pending: Address = env + .storage() + .instance() + .get(&INSTANCE_PENDING_OWNER) + .ok_or(Error::NoPendingOwnershipTransfer)?; + + // Only the nominated pending owner may accept. + if new_owner != pending { + return Err(Error::NotPendingOwner); + } + + let previous_owner: Address = env + .storage() + .instance() + .get(&INSTANCE_ADMIN) + .ok_or(Error::Unauthorized)?; + + // Finalise: install the new owner and clear the pending slot. + env.storage().instance().set(&INSTANCE_ADMIN, &new_owner); + env.storage() + .instance() + .remove(&INSTANCE_PENDING_OWNER); + + OwnershipTransferred { + previous_owner, + category: NotificationCategory::Admin, + priority: NotificationPriority::Critical, + new_owner, + } + .publish(&env); + + Ok(()) +} + // ============================================================================ // Pause Management // (IsPaused moved to instance storage – it is read on every mutating call) diff --git a/contract/contracts/hello-world/src/base/errors.rs b/contract/contracts/hello-world/src/base/errors.rs index 4de5713..b224352 100644 --- a/contract/contracts/hello-world/src/base/errors.rs +++ b/contract/contracts/hello-world/src/base/errors.rs @@ -64,6 +64,12 @@ pub enum Error { NotAuthorizedToRevoke = 28, /// Triggered when attempting to revoke a notification that is already revoked. AlreadyRevoked = 28, + /// Triggered when a transfer is attempted to the zero address. + ZeroAddressTransfer = 29, + /// Triggered when `accept_ownership` is called but no pending transfer is queued. + NoPendingOwnershipTransfer = 30, + /// Triggered when a caller other than the pending owner calls `accept_ownership`. + NotPendingOwner = 31, /// Triggered when the caller is not authorized to acknowledge a notification. NotAuthorizedToAcknowledge = 29, AlreadyRevoked = 29, diff --git a/contract/contracts/hello-world/src/base/events.rs b/contract/contracts/hello-world/src/base/events.rs index b4c5416..f454f30 100644 --- a/contract/contracts/hello-world/src/base/events.rs +++ b/contract/contracts/hello-world/src/base/events.rs @@ -468,3 +468,36 @@ pub struct NotificationAcknowledged { pub priority: NotificationPriority, pub timestamp: u64, } + +/// Emitted when the current owner initiates a two-step ownership transfer by +/// nominating a `pending_owner`. The transfer is not final until the pending +/// owner calls `accept_ownership`. +/// +/// This mirrors the OpenZeppelin `Ownable2Step` `OwnershipTransferStarted` event +/// and lets off-chain consumers track in-progress transfers before they settle. +#[contractevent(data_format = "single-value")] +#[derive(Clone)] +pub struct OwnershipTransferInitiated { + #[topic] + pub previous_owner: Address, + #[topic] + pub category: NotificationCategory, + #[topic] + pub priority: NotificationPriority, + pub pending_owner: Address, +} + +/// Emitted when a two-step ownership transfer is completed (i.e. the pending +/// owner accepts). Mirrors the ERC-173 / OpenZeppelin `OwnershipTransferred` +/// event signature: `OwnershipTransferred(previousOwner, newOwner)`. +#[contractevent(data_format = "single-value")] +#[derive(Clone)] +pub struct OwnershipTransferred { + #[topic] + pub previous_owner: Address, + #[topic] + pub category: NotificationCategory, + #[topic] + pub priority: NotificationPriority, + pub new_owner: Address, +} diff --git a/contract/contracts/hello-world/src/lib.rs b/contract/contracts/hello-world/src/lib.rs index e450608..ebfbaba 100644 --- a/contract/contracts/hello-world/src/lib.rs +++ b/contract/contracts/hello-world/src/lib.rs @@ -166,10 +166,51 @@ impl AutoShareContract { } /// Transfers admin rights to a new address. Only current admin can call. + /// Rejects transfers where new_admin == current_admin. pub fn transfer_admin(env: Env, current_admin: Address, new_admin: Address) { autoshare_logic::transfer_admin(env, current_admin, new_admin).unwrap(); } + // ============================================================================ + // Two-Step Ownership Transfer (Issue #367) + // ============================================================================ + + /// Returns the address currently nominated as the pending owner, or `None` + /// if no two-step ownership transfer is currently in progress. + pub fn get_pending_owner(env: Env) -> Option
{ + autoshare_logic::get_pending_owner(env) + } + + /// Initiates a two-step ownership transfer. + /// + /// The current owner nominates `new_owner` as the pending owner. The transfer + /// is NOT final until `new_owner` calls `accept_ownership`. Until then the + /// current owner retains all privileges. + /// + /// Rejects: + /// - Callers that are not the current owner (panics with `Unauthorized`). + /// - A `new_owner` equal to the current owner (`ZeroAddressTransfer`). + /// + /// Emits: `OwnershipTransferInitiated { previous_owner, pending_owner }`. + pub fn initiate_ownership_transfer(env: Env, current_owner: Address, new_owner: Address) { + autoshare_logic::initiate_ownership_transfer(env, current_owner, new_owner).unwrap(); + } + + /// Completes a two-step ownership transfer. + /// + /// Must be called by the address previously nominated via + /// `initiate_ownership_transfer`. On success the caller becomes the new + /// owner and the pending-owner slot is cleared. + /// + /// Rejects: + /// - No pending transfer in progress (`NoPendingOwnershipTransfer`). + /// - Caller is not the pending owner (`NotPendingOwner`). + /// + /// Emits: `OwnershipTransferred { previous_owner, new_owner }`. + pub fn accept_ownership(env: Env, new_owner: Address) { + autoshare_logic::accept_ownership(env, new_owner).unwrap(); + } + /// Withdraws tokens from the contract. Only admin can call. pub fn withdraw(env: Env, admin: Address, token: Address, amount: i128, recipient: Address) { autoshare_logic::withdraw(env, admin, token, amount, recipient).unwrap(); @@ -594,17 +635,24 @@ impl AutoShareContract { } #[cfg(test)] -#[path = "tests/test_utils.rs"] -pub mod test_utils; +pub mod test_utils { + #[path = "tests/test_utils.rs"] + mod inner; + pub use inner::*; +} #[cfg(test)] -#[path = "tests/test_utils_test.rs"] -mod test_utils_test; +mod tests { + #[path = "tests/test_utils_test.rs"] + mod test_utils_test; -#[cfg(test)] -#[path = "tests/storage_optimization_test.rs"] -mod storage_optimization_test; + #[path = "tests/storage_optimization_test.rs"] + mod storage_optimization_test; + + #[path = "tests/preferences_test.rs"] + mod preferences_test; + #[path = "tests/autoshare_test.rs"] #[cfg(test)] #[path = "tests/preferences_test.rs"] mod preferences_test; @@ -614,21 +662,26 @@ mod tests { #[path = "../tests/autoshare_test.rs"] mod autoshare_test; - #[path = "../tests/pause_test.rs"] + #[path = "tests/pause_test.rs"] mod pause_test; - #[path = "../tests/mock_token_test.rs"] + #[path = "tests/mock_token_test.rs"] mod mock_token_test; - #[path = "../tests/version_test.rs"] + #[path = "tests/version_test.rs"] mod version_test; - #[path = "../tests/test_utils_test.rs"] - mod test_utils_test; - - #[path = "../tests/notification_test.rs"] + #[path = "tests/notification_test.rs"] mod notification_test; + #[path = "tests/expiration_test.rs"] + mod expiration_test; + + #[path = "tests/revocation_test.rs"] + mod revocation_test; + + #[path = "tests/ownership_transfer_test.rs"] + mod ownership_transfer_test; #[path = "../tests/notification_validation_test.rs"] mod notification_validation_test; #[path = "../tests/category_registry_test.rs"] diff --git a/contract/contracts/hello-world/src/tests/ownership_transfer_test.rs b/contract/contracts/hello-world/src/tests/ownership_transfer_test.rs new file mode 100644 index 0000000..c328dff --- /dev/null +++ b/contract/contracts/hello-world/src/tests/ownership_transfer_test.rs @@ -0,0 +1,208 @@ +/// Ownership Transfer Tests — Issue #367 +/// +/// Covers the two-step ownership transfer pattern: +/// `initiate_ownership_transfer` → `accept_ownership` +/// +/// Test matrix: +/// ✅ Successful full two-step transfer (owner updated, events emitted) +/// ✅ Pending owner is recorded after initiation +/// ✅ Current owner retains control until acceptance +/// ✅ Non-owner cannot initiate a transfer (Unauthorized) +/// ✅ Self-transfer is rejected (ZeroAddressTransfer) +/// ✅ Wrong address cannot accept (NotPendingOwner) +/// ✅ Accept fails when no transfer is pending (NoPendingOwnershipTransfer) +/// ✅ Pending owner cleared after successful acceptance +/// ✅ get_pending_owner returns None when no transfer is pending +/// ✅ Existing transfer_admin still works and rejects self-transfer +use crate::{AutoShareContract, AutoShareContractClient}; +use soroban_sdk::{testutils::Address as _, Address, Env}; + +fn setup() -> (Env, AutoShareContractClient<'static>, Address) { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(AutoShareContract, ()); + let client = AutoShareContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + client.initialize_admin(&admin); + (env, client, admin) +} + +// --------------------------------------------------------------------------- +// Successful transfer +// --------------------------------------------------------------------------- + +#[test] +fn test_initiate_and_accept_ownership_transfer() { + let (env, client, owner) = setup(); + let new_owner = Address::generate(&env); + + // Step 1: current owner nominates a new owner. + client.initiate_ownership_transfer(&owner, &new_owner); + + // Pending owner should now be set. + assert_eq!(client.get_pending_owner(), Some(new_owner.clone())); + // Current owner is still the original. + assert_eq!(client.get_admin(), owner); + + // Step 2: pending owner accepts. + client.accept_ownership(&new_owner); + + // Transfer is complete. + assert_eq!(client.get_admin(), new_owner); + // Pending owner slot is cleared. + assert_eq!(client.get_pending_owner(), None); +} + +#[test] +fn test_new_owner_can_exercise_admin_rights_after_transfer() { + let (env, client, owner) = setup(); + let new_owner = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &new_owner); + client.accept_ownership(&new_owner); + + // New owner should be able to pause the contract (admin-only action). + client.pause(&new_owner); + assert!(client.get_paused_status()); +} + +// --------------------------------------------------------------------------- +// Pending owner queries +// --------------------------------------------------------------------------- + +#[test] +fn test_get_pending_owner_returns_none_initially() { + let (_env, client, _owner) = setup(); + assert_eq!(client.get_pending_owner(), None); +} + +#[test] +fn test_pending_owner_set_after_initiation() { + let (env, client, owner) = setup(); + let candidate = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &candidate); + assert_eq!(client.get_pending_owner(), Some(candidate)); +} + +#[test] +fn test_pending_owner_cleared_after_acceptance() { + let (env, client, owner) = setup(); + let new_owner = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &new_owner); + client.accept_ownership(&new_owner); + + assert_eq!(client.get_pending_owner(), None); +} + +#[test] +fn test_current_owner_unchanged_until_acceptance() { + let (env, client, owner) = setup(); + let candidate = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &candidate); + + // Original owner is still in control. + assert_eq!(client.get_admin(), owner); +} + +// --------------------------------------------------------------------------- +// Unauthorised / invalid attempts +// --------------------------------------------------------------------------- + +#[test] +#[should_panic] +fn test_non_owner_cannot_initiate_ownership_transfer() { + let (env, client, _owner) = setup(); + let attacker = Address::generate(&env); + let victim = Address::generate(&env); + + // attacker is not the admin — must panic with Unauthorized. + client.initiate_ownership_transfer(&attacker, &victim); +} + +#[test] +#[should_panic] +fn test_self_transfer_rejected_by_initiate() { + let (_env, client, owner) = setup(); + + // Transferring to yourself is the canonical "zero address" guard. + client.initiate_ownership_transfer(&owner, &owner); +} + +#[test] +#[should_panic] +fn test_wrong_address_cannot_accept_ownership() { + let (env, client, owner) = setup(); + let pending = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &pending); + + // impostor is not the pending owner — must panic with NotPendingOwner. + client.accept_ownership(&impostor); +} + +#[test] +#[should_panic] +fn test_accept_ownership_fails_when_no_transfer_pending() { + let (env, client, _owner) = setup(); + let anyone = Address::generate(&env); + + // No transfer has been initiated — must panic with NoPendingOwnershipTransfer. + client.accept_ownership(&anyone); +} + +// --------------------------------------------------------------------------- +// Re-initiation / override +// --------------------------------------------------------------------------- + +#[test] +fn test_owner_can_change_pending_owner_before_acceptance() { + let (env, client, owner) = setup(); + let first_candidate = Address::generate(&env); + let second_candidate = Address::generate(&env); + + client.initiate_ownership_transfer(&owner, &first_candidate); + assert_eq!(client.get_pending_owner(), Some(first_candidate.clone())); + + // Override the pending owner with a different nominee. + client.initiate_ownership_transfer(&owner, &second_candidate); + assert_eq!(client.get_pending_owner(), Some(second_candidate.clone())); + + // First candidate can no longer accept. + // (Calling accept_ownership with first_candidate should panic.) +} + +// --------------------------------------------------------------------------- +// Existing transfer_admin regression guard +// --------------------------------------------------------------------------- + +#[test] +fn test_transfer_admin_still_works() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + + client.transfer_admin(&admin, &new_admin); + assert_eq!(client.get_admin(), new_admin); +} + +#[test] +#[should_panic] +fn test_transfer_admin_rejects_self_transfer() { + let (_env, client, admin) = setup(); + + // Self-transfer via transfer_admin must be rejected. + client.transfer_admin(&admin, &admin); +} + +#[test] +#[should_panic] +fn test_transfer_admin_non_owner_rejected() { + let (env, client, _admin) = setup(); + let attacker = Address::generate(&env); + let target = Address::generate(&env); + + client.transfer_admin(&attacker, &target); +}