diff --git a/contracts/allocation_logic/src/lib.rs b/contracts/allocation_logic/src/lib.rs index 32378b4..38ee967 100644 --- a/contracts/allocation_logic/src/lib.rs +++ b/contracts/allocation_logic/src/lib.rs @@ -142,6 +142,7 @@ pub enum DataKey { Strategy(String), CommitmentCore, Admin, + PendingAdmin, Initialized, ReentrancyGuard, PoolRegistry, // Vec of all pool IDs @@ -740,12 +741,46 @@ impl AllocationStrategiesContract { read_version(&env) } - /// Update admin (admin-only). - pub fn set_admin(env: Env, caller: Address, new_admin: Address) -> Result<(), Error> { + /// Return the pending admin, if a handoff has been proposed. + pub fn get_pending_admin(env: Env) -> Option
{ + env.storage().instance().get(&DataKey::PendingAdmin) + } + + /// Propose a new admin (admin-only). + /// + /// The proposed admin must call [`Self::accept_admin`] before control transfers. + pub fn propose_admin(env: Env, caller: Address, new_admin: Address) -> Result<(), Error> { caller.require_auth(); Self::require_initialized(&env)?; Self::require_admin(&env, &caller)?; - env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().instance().set(&DataKey::PendingAdmin, &new_admin); + Ok(()) + } + + /// Accept a pending admin handoff. + /// + /// Only the pending admin can finalize the transfer. + pub fn accept_admin(env: Env, caller: Address) -> Result<(), Error> { + caller.require_auth(); + Self::require_initialized(&env)?; + let pending: Address = env + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .ok_or(Error::Unauthorized)?; + if caller != pending { + return Err(Error::Unauthorized); + } + env.storage().instance().set(&DataKey::Admin, &caller); + env.storage().instance().remove(&DataKey::PendingAdmin); + Ok(()) + } + + /// Deprecated compatibility wrapper for proposing a new admin. + /// + /// Does not transfer control until `new_admin` calls [`Self::accept_admin`]. + pub fn set_admin(env: Env, caller: Address, new_admin: Address) -> Result<(), Error> { + Self::propose_admin(env, caller, new_admin)?; Ok(()) } diff --git a/contracts/allocation_logic/src/tests.rs b/contracts/allocation_logic/src/tests.rs index 515b211..ffd74b8 100644 --- a/contracts/allocation_logic/src/tests.rs +++ b/contracts/allocation_logic/src/tests.rs @@ -147,6 +147,42 @@ fn test_migrate_rejects_non_admin() { assert_eq!(client.get_version(), CURRENT_VERSION); } +#[test] +fn test_admin_handoff_requires_pending_admin_acceptance() { + let env = Env::default(); + env.mock_all_auths(); + let (admin, _core_id, client) = create_contract(&env); + let pending = Address::generate(&env); + let wrong = Address::generate(&env); + + assert_eq!(client.try_propose_admin(&wrong, &pending), Err(Ok(Error::Unauthorized))); + assert_eq!(client.try_set_admin(&admin, &pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(pending.clone())); + assert_eq!(client.try_register_pool(&pending, &99, &RiskLevel::Low, &400, &1_000), Err(Ok(Error::Unauthorized))); + assert_eq!(client.try_accept_admin(&wrong), Err(Ok(Error::Unauthorized))); + assert_eq!(client.try_accept_admin(&pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), None); + assert_eq!(client.try_register_pool(&admin, &99, &RiskLevel::Low, &400, &1_000), Err(Ok(Error::Unauthorized))); + assert_eq!(client.try_register_pool(&pending, &99, &RiskLevel::Low, &400, &1_000), Ok(Ok(()))); +} + +#[test] +fn test_admin_handoff_reproposal_overwrites_pending_admin() { + let env = Env::default(); + env.mock_all_auths(); + let (admin, _core_id, client) = create_contract(&env); + let first_pending = Address::generate(&env); + let second_pending = Address::generate(&env); + + assert_eq!(client.try_propose_admin(&admin, &first_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(first_pending.clone())); + assert_eq!(client.try_propose_admin(&admin, &second_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(second_pending.clone())); + assert_eq!(client.try_accept_admin(&first_pending), Err(Ok(Error::Unauthorized))); + assert_eq!(client.try_accept_admin(&second_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), None); +} + // ============================================================================ // COMPREHENSIVE REBALANCE TESTS - Issue #236 // Focus: Owner Match, Strategy Persistence, Summary Correctness diff --git a/contracts/attestation_engine/src/lib.rs b/contracts/attestation_engine/src/lib.rs index 1f1bacf..2d711a4 100644 --- a/contracts/attestation_engine/src/lib.rs +++ b/contracts/attestation_engine/src/lib.rs @@ -61,6 +61,8 @@ pub enum AttestationError { pub enum DataKey { /// Admin address Admin, + /// Proposed admin waiting to accept the handoff. + PendingAdmin, /// Core contract address CoreContract, /// Verifier whitelist (Address -> bool) @@ -499,10 +501,47 @@ impl AttestationEngineContract { read_version(&e) } - /// Update admin (admin-only). - pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), AttestationError> { + /// Return the pending admin, if a handoff has been proposed. + pub fn get_pending_admin(e: Env) -> Option
{ + e.storage().instance().get(&DataKey::PendingAdmin) + } + + /// Propose a new admin (admin-only). + /// + /// The proposed admin must call [`Self::accept_admin`] before control transfers. + pub fn propose_admin( + e: Env, + caller: Address, + new_admin: Address, + ) -> Result<(), AttestationError> { require_admin(&e, &caller)?; - e.storage().instance().set(&DataKey::Admin, &new_admin); + e.storage().instance().set(&DataKey::PendingAdmin, &new_admin); + Ok(()) + } + + /// Accept a pending admin handoff. + /// + /// Only the pending admin can finalize the transfer. + pub fn accept_admin(e: Env, caller: Address) -> Result<(), AttestationError> { + caller.require_auth(); + let pending: Address = e + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .ok_or(AttestationError::Unauthorized)?; + if caller != pending { + return Err(AttestationError::Unauthorized); + } + e.storage().instance().set(&DataKey::Admin, &caller); + e.storage().instance().remove(&DataKey::PendingAdmin); + Ok(()) + } + + /// Deprecated compatibility wrapper for proposing a new admin. + /// + /// Does not transfer control until `new_admin` calls [`Self::accept_admin`]. + pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), AttestationError> { + Self::propose_admin(e, caller, new_admin)?; Ok(()) } diff --git a/contracts/attestation_engine/src/tests.rs b/contracts/attestation_engine/src/tests.rs index bd27b78..b9f940b 100644 --- a/contracts/attestation_engine/src/tests.rs +++ b/contracts/attestation_engine/src/tests.rs @@ -228,6 +228,69 @@ fn test_migrate_rejects_non_admin() { assert_eq!(client.get_version(), 0); } +#[test] +fn test_admin_handoff_requires_pending_admin_acceptance() { + let e = Env::default(); + e.mock_all_auths(); + let contract_id = e.register_contract(None, AttestationEngineContract); + let client = AttestationEngineContractClient::new(&e, &contract_id); + let admin = Address::generate(&e); + let pending = Address::generate(&e); + let wrong = Address::generate(&e); + let core = Address::generate(&e); + + client.initialize(&admin, &core); + + assert_eq!( + client.try_propose_admin(&wrong, &pending), + Err(Ok(AttestationError::Unauthorized)) + ); + assert_eq!(client.try_set_admin(&admin, &pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), admin); + assert_eq!(client.get_pending_admin(), Some(pending.clone())); + assert_eq!( + client.try_add_verifier(&pending, &pending), + Err(Ok(AttestationError::Unauthorized)) + ); + assert_eq!( + client.try_accept_admin(&wrong), + Err(Ok(AttestationError::Unauthorized)) + ); + assert_eq!(client.try_accept_admin(&pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), pending.clone()); + assert_eq!(client.get_pending_admin(), None); + assert_eq!( + client.try_add_verifier(&admin, &admin), + Err(Ok(AttestationError::Unauthorized)) + ); + assert_eq!(client.try_add_verifier(&pending, &pending), Ok(Ok(()))); +} + +#[test] +fn test_admin_handoff_reproposal_overwrites_pending_admin() { + let e = Env::default(); + e.mock_all_auths(); + let contract_id = e.register_contract(None, AttestationEngineContract); + let client = AttestationEngineContractClient::new(&e, &contract_id); + let admin = Address::generate(&e); + let first_pending = Address::generate(&e); + let second_pending = Address::generate(&e); + let core = Address::generate(&e); + + client.initialize(&admin, &core); + + assert_eq!(client.try_propose_admin(&admin, &first_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(first_pending.clone())); + assert_eq!(client.try_propose_admin(&admin, &second_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(second_pending.clone())); + assert_eq!( + client.try_accept_admin(&first_pending), + Err(Ok(AttestationError::Unauthorized)) + ); + assert_eq!(client.try_accept_admin(&second_pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), second_pending); +} + #[test] fn test_get_health_metrics_cross_reads_commitment_core_state() { let e = Env::default(); diff --git a/contracts/commitment_nft/src/lib.rs b/contracts/commitment_nft/src/lib.rs index cdafc97..cf7a300 100644 --- a/contracts/commitment_nft/src/lib.rs +++ b/contracts/commitment_nft/src/lib.rs @@ -178,6 +178,8 @@ pub struct TransferParams { pub enum DataKey { /// Admin address (singleton) Admin, + /// Proposed admin waiting to accept the handoff. + PendingAdmin, /// Counter for generating unique token IDs / Total supply TokenCounter, /// NFT data storage (token_id -> CommitmentNFT) @@ -575,15 +577,48 @@ impl CommitmentNFTContract { read_version(&e) } - /// Update admin (admin-only). - pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), ContractError> { + /// Return the pending admin, if a handoff has been proposed. + pub fn get_pending_admin(e: Env) -> Option
{ + e.storage().instance().get(&DataKey::PendingAdmin) + } + + /// Propose a new admin (admin-only). + /// + /// The proposed admin must call [`Self::accept_admin`] before control transfers. + pub fn propose_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), ContractError> { require_admin(&e, &caller)?; if is_zero_address(&e, &new_admin) { return Err(ContractError::InvalidAddress); } - e.storage().instance().set(&DataKey::Admin, &new_admin); + e.storage().instance().set(&DataKey::PendingAdmin, &new_admin); + Ok(()) + } + + /// Accept a pending admin handoff. + /// + /// Only the pending admin can finalize the transfer. + pub fn accept_admin(e: Env, caller: Address) -> Result<(), ContractError> { + caller.require_auth(); + let pending: Address = e + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .ok_or(ContractError::NotAuthorized)?; + if caller != pending { + return Err(ContractError::NotAuthorized); + } + e.storage().instance().set(&DataKey::Admin, &caller); + e.storage().instance().remove(&DataKey::PendingAdmin); + Ok(()) + } + + /// Deprecated compatibility wrapper for proposing a new admin. + /// + /// Does not transfer control until `new_admin` calls [`Self::accept_admin`]. + pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), ContractError> { + Self::propose_admin(e, caller, new_admin)?; Ok(()) } diff --git a/contracts/commitment_nft/src/smoke_tests.rs b/contracts/commitment_nft/src/smoke_tests.rs index db52bdd..f35e2e4 100644 --- a/contracts/commitment_nft/src/smoke_tests.rs +++ b/contracts/commitment_nft/src/smoke_tests.rs @@ -24,6 +24,57 @@ fn test_initialize_sets_admin_and_zero_supply() { assert_eq!(client.total_supply(), 0); } +#[test] +fn test_admin_handoff_requires_pending_admin_acceptance() { + let e = Env::default(); + let (admin, client) = setup_contract(&e); + let pending = Address::generate(&e); + let wrong = Address::generate(&e); + + assert_eq!( + client.try_propose_admin(&wrong, &pending), + Err(Ok(ContractError::NotAuthorized)) + ); + assert_eq!(client.try_set_admin(&admin, &pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), admin); + assert_eq!(client.get_pending_admin(), Some(pending.clone())); + assert_eq!( + client.try_add_authorized_contract(&pending, &pending), + Err(Ok(ContractError::NotAuthorized)) + ); + assert_eq!( + client.try_accept_admin(&wrong), + Err(Ok(ContractError::NotAuthorized)) + ); + assert_eq!(client.try_accept_admin(&pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), pending.clone()); + assert_eq!(client.get_pending_admin(), None); + assert_eq!( + client.try_add_authorized_contract(&admin, &admin), + Err(Ok(ContractError::NotAuthorized)) + ); + assert_eq!(client.try_add_authorized_contract(&pending, &pending), Ok(Ok(()))); +} + +#[test] +fn test_admin_handoff_reproposal_overwrites_pending_admin() { + let e = Env::default(); + let (admin, client) = setup_contract(&e); + let first_pending = Address::generate(&e); + let second_pending = Address::generate(&e); + + assert_eq!(client.try_propose_admin(&admin, &first_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(first_pending.clone())); + assert_eq!(client.try_propose_admin(&admin, &second_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(second_pending.clone())); + assert_eq!( + client.try_accept_admin(&first_pending), + Err(Ok(ContractError::NotAuthorized)) + ); + assert_eq!(client.try_accept_admin(&second_pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), second_pending); +} + #[test] fn test_mint_and_settle_as_core_updates_supply_and_activity() { let e = Env::default(); diff --git a/contracts/price_oracle/src/lib.rs b/contracts/price_oracle/src/lib.rs index 5392266..78036b5 100644 --- a/contracts/price_oracle/src/lib.rs +++ b/contracts/price_oracle/src/lib.rs @@ -64,6 +64,7 @@ pub struct OracleConfig { #[contracttype] pub enum DataKey { Admin, + PendingAdmin, /// Default max age (seconds) for price validity (legacy) MaxStalenessSeconds, /// Whitelist: set of Address that can call set_price @@ -340,21 +341,50 @@ impl PriceOracleContract { read_admin(&e) } + /// @notice Get the pending admin, if a handoff has been proposed. + pub fn get_pending_admin(e: Env) -> Option
{ + e.storage().instance().get(&DataKey::PendingAdmin) + } + /// Get current on-chain version (0 if legacy/uninitialized). pub fn get_version(e: Env) -> u32 { read_version(&e) } - /// @notice Update admin address (admin-only). - /// @dev Transfers control over whitelist management and configuration. + /// @notice Propose a new admin address (admin-only). + /// @dev The proposed admin must call `accept_admin` before control transfers. /// @param e Contract environment. /// @param caller Must be the current admin. /// @param new_admin Address to set as new admin. /// @return Ok(()) on success, Err(Unauthorized) if not admin. /// @security Only the admin can transfer admin authority. - pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), OracleError> { + pub fn propose_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), OracleError> { require_admin_result(&e, &caller)?; - e.storage().instance().set(&DataKey::Admin, &new_admin); + e.storage().instance().set(&DataKey::PendingAdmin, &new_admin); + Ok(()) + } + + /// @notice Accept a pending admin handoff. + /// @dev Only the pending admin can finalize the transfer. + pub fn accept_admin(e: Env, caller: Address) -> Result<(), OracleError> { + caller.require_auth(); + let pending: Address = e + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .ok_or(OracleError::Unauthorized)?; + if caller != pending { + return Err(OracleError::Unauthorized); + } + e.storage().instance().set(&DataKey::Admin, &caller); + e.storage().instance().remove(&DataKey::PendingAdmin); + Ok(()) + } + + /// @notice Deprecated compatibility wrapper for proposing a new admin. + /// @dev Does not transfer control until `accept_admin` is called by `new_admin`. + pub fn set_admin(e: Env, caller: Address, new_admin: Address) -> Result<(), OracleError> { + Self::propose_admin(e, caller, new_admin)?; Ok(()) } diff --git a/contracts/price_oracle/src/tests.rs b/contracts/price_oracle/src/tests.rs index 8cb32b8..d894dfe 100644 --- a/contracts/price_oracle/src/tests.rs +++ b/contracts/price_oracle/src/tests.rs @@ -52,10 +52,15 @@ fn test_admin_transfer_and_oracle_control() { assert_eq!(client.try_add_oracle(&admin1, &oracle), Ok(Ok(()))); assert!(client.is_oracle_whitelisted(&oracle)); - // Transfer admin + // Transfer admin requires propose + accept assert_eq!(client.try_set_admin(&admin2, &admin2), Err(Ok(OracleError::Unauthorized))); assert_eq!(client.try_set_admin(&admin1, &admin2), Ok(Ok(()))); + assert_eq!(client.get_admin(), admin1); + assert_eq!(client.get_pending_admin(), Some(admin2.clone())); + assert_eq!(client.try_accept_admin(&admin1), Err(Ok(OracleError::Unauthorized))); + assert_eq!(client.try_accept_admin(&admin2), Ok(Ok(()))); assert_eq!(client.get_admin(), admin2); + assert_eq!(client.get_pending_admin(), None); // Now only admin2 can remove assert_eq!(client.try_remove_oracle(&admin1, &oracle), Err(Ok(OracleError::Unauthorized))); @@ -63,6 +68,30 @@ fn test_admin_transfer_and_oracle_control() { assert!(!client.is_oracle_whitelisted(&oracle)); } +#[test] +fn test_admin_handoff_reproposal_overwrites_pending_admin() { + let e = Env::default(); + e.mock_all_auths(); + let admin = Address::generate(&e); + let first_pending = Address::generate(&e); + let second_pending = Address::generate(&e); + let contract_id = e.register_contract(None, PriceOracleContract); + let client = PriceOracleContractClient::new(&e, &contract_id); + + e.as_contract(&contract_id, || { + PriceOracleContract::initialize(e.clone(), admin.clone()).unwrap(); + }); + + assert_eq!(client.try_propose_admin(&first_pending, &first_pending), Err(Ok(OracleError::Unauthorized))); + assert_eq!(client.try_propose_admin(&admin, &first_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(first_pending.clone())); + assert_eq!(client.try_propose_admin(&admin, &second_pending), Ok(Ok(()))); + assert_eq!(client.get_pending_admin(), Some(second_pending.clone())); + assert_eq!(client.try_accept_admin(&first_pending), Err(Ok(OracleError::Unauthorized))); + assert_eq!(client.try_accept_admin(&second_pending), Ok(Ok(()))); + assert_eq!(client.get_admin(), second_pending); +} + use super::*; use soroban_sdk::testutils::{Address as _, Ledger}; diff --git a/docs/SECURITY_CONSIDERATIONS.md b/docs/SECURITY_CONSIDERATIONS.md index 2fdea3d..749c80c 100644 --- a/docs/SECURITY_CONSIDERATIONS.md +++ b/docs/SECURITY_CONSIDERATIONS.md @@ -4,6 +4,7 @@ - Admin-only functions in allocation_logic and attestation_engine require `require_auth` and compare caller to stored admin. - commitment_nft `set_core_contract` enforces admin auth, and `settle` / `mark_inactive` now require authorization from the configured core contract; `mint` still relies on a caller-supplied address and should remain in audit scope. +- Admin rotation in `commitment_nft`, `attestation_engine`, `allocation_logic`, and `price_oracle` uses a two-step handoff: the current admin proposes `PendingAdmin`, then the pending admin must authenticate and accept before control changes. Legacy `set_admin` entrypoints are compatibility wrappers for proposing, not finalizing, a transfer. - commitment_core state-changing functions (`create_commitment`, `settle`, `early_exit`, `allocate`, `update_value`) do not call `require_auth` and accept caller-provided addresses. - Attestation recording requires caller authorization (`is_authorized_verifier`) and `require_auth`. diff --git a/docs/TIMELOCK_RUNBOOK.md b/docs/TIMELOCK_RUNBOOK.md index a6bb14d..a5b98e4 100644 --- a/docs/TIMELOCK_RUNBOOK.md +++ b/docs/TIMELOCK_RUNBOOK.md @@ -71,6 +71,9 @@ Use for signer rotation, multisig replacement, or governance transfer. Operational guidance: - verify the new admin can authenticate on Soroban before queueing +- for contracts with two-step admin handoff, queue or execute the proposal first, then require the pending admin to call `accept_admin` from the destination key or multisig before treating the rotation as complete +- after acceptance, confirm `get_admin()` returns the new address and `get_pending_admin()` is empty +- if the destination address is wrong or the recipient cannot authenticate, have the current admin re-run the proposal with the corrected address; a new proposal overwrites the pending admin - use a longer delay if the change also alters operational processes or key custody ### `Upgrade`