From b258c3d9875447e92f10bf7b872e2dbad9299d19 Mon Sep 17 00:00:00 2001 From: malaysiaonelove Date: Thu, 23 Jul 2026 02:06:18 +0000 Subject: [PATCH] feat: two-step admin/role transfer across all six contracts (closes #20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the propose/accept/cancel flow for every admin key and wiring address exposed by the Orivex-Contracts platform. ## Shared module - contracts/common/src/two_step.rs: new PendingTransfer struct and three shared events (TransferProposed, TransferAccepted, TransferCancelled). Soft timelock semantics per #20 acceptance criteria. ## Per-contract additions | Contract | Roles covered | | -------------- | ------------------------------------------ | | reward-pool | Admin | | badge-nft | Admin | | stake-vault | Admin | | course-registry| Admin, RewardPoolAddress, BadgeNftAddress | | quest-engine | Admin, RewardPool, StakeVault | | governance | Admin, BadgeContractAddress | For each role we add: - propose_new_(current_admin, proposed): admin-only stage 1 - accept_(acceptor): only the proposed address may call - cancel_(caller): caller == proposed address OR caller == current admin (typo recovery) Single-step setters (set_reward_pool_address, set_badge_nft_address, quest-engine set_*_address, etc.) are kept for the initial bootstrap. Subsequent rotations go through the two-step flow. ## Dependencies Added contracts-common to reward-pool, badge-nft, stake-vault, quest-engine, governance (course-registry already had it). ## Tests Per role, ~10 tests covering happy-path propose→accept, event emission, unauthorized-propose, wrong-acceptor panic, no-pending panics, typo recovery (admin cancel AND self cancel), random-cancel panic. ## Docs Top-level contracts/README.md plus per-contract READMEs document the new auth flow. Closes #20 --- contracts/README.md | 15 ++ contracts/badge-nft/Cargo.toml | 2 + contracts/badge-nft/README.md | 19 ++ contracts/badge-nft/src/lib.rs | 97 +++++++++ contracts/badge-nft/src/test.rs | 126 +++++++++++ contracts/badge-nft/src/types.rs | 2 + contracts/common/src/lib.rs | 1 + contracts/common/src/two_step.rs | 72 +++++++ contracts/course-registry/README.md | 24 +++ contracts/course-registry/src/lib.rs | 278 +++++++++++++++++++++++++ contracts/course-registry/src/test.rs | 225 ++++++++++++++++++++ contracts/course-registry/src/types.rs | 7 + contracts/governance/Cargo.toml | 2 + contracts/governance/README.md | 17 ++ contracts/governance/src/lib.rs | 185 ++++++++++++++++ contracts/governance/src/test.rs | 146 +++++++++++++ contracts/governance/src/types.rs | 5 + contracts/quest-engine/Cargo.toml | 2 + contracts/quest-engine/README.md | 20 ++ contracts/quest-engine/src/lib.rs | 276 ++++++++++++++++++++++++ contracts/quest-engine/src/test.rs | 180 ++++++++++++++++ contracts/quest-engine/src/types.rs | 7 + contracts/reward-pool/Cargo.toml | 2 + contracts/reward-pool/README.md | 22 ++ contracts/reward-pool/src/lib.rs | 102 +++++++++ contracts/reward-pool/src/test.rs | 180 ++++++++++++++++ contracts/reward-pool/src/types.rs | 4 + contracts/stake-vault/Cargo.toml | 2 + contracts/stake-vault/README.md | 16 ++ contracts/stake-vault/src/lib.rs | 99 +++++++++ contracts/stake-vault/src/test.rs | 130 ++++++++++++ contracts/stake-vault/src/types.rs | 2 + 32 files changed, 2267 insertions(+) create mode 100644 contracts/common/src/two_step.rs diff --git a/contracts/README.md b/contracts/README.md index f15955d..67e5d85 100644 --- a/contracts/README.md +++ b/contracts/README.md @@ -15,3 +15,18 @@ This repository uses the recommended structure for a Soroban project: ├── Cargo.toml └── README.md ``` + +## Two-Step Access Control (Issue #20) + +Every contract exposes a **two-step** flow for rotating each admin and +wiring address: + +1. `propose_new_(current_admin, proposed)` — admin-only. +2. `accept_(acceptor)` — only the proposed address. +3. `cancel_(caller)` — current admin OR proposed address. + +The shared types (`PendingTransfer`) and events +(`TransferProposed` / `TransferAccepted` / `TransferCancelled`) live in +`contracts/common::two_step`. The timelock is **soft**: the proposed +address may accept immediately. Off-chain monitors are expected to alert +on `TransferProposed` events so communities can react before acceptance. diff --git a/contracts/badge-nft/Cargo.toml b/contracts/badge-nft/Cargo.toml index 0ff5b38..aac05c1 100644 --- a/contracts/badge-nft/Cargo.toml +++ b/contracts/badge-nft/Cargo.toml @@ -13,9 +13,11 @@ doctest = false [dependencies] soroban-sdk = { workspace = true } +contracts-common = { path = "../common", default-features = false } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } +contracts-common = { path = "../common" } [features] default = ["contract"] diff --git a/contracts/badge-nft/README.md b/contracts/badge-nft/README.md index f0cfe8b..31c13da 100644 --- a/contracts/badge-nft/README.md +++ b/contracts/badge-nft/README.md @@ -11,3 +11,22 @@ Soulbound badge issuance, retrieval, and admin revocation. - `get_badge_count(learner)` — returns the count. - `has_badge(learner, course_id)` — boolean lookup. - `upgrade_contract(admin, new_wasm_hash)` — admin-only WASM upgrade. + +## Two-Step Admin Transfer (Issue #20) + +The registry role (`Admin`) is rotated through `propose → accept` so a +typo or compromised-key incident can be cancelled without permanently +locking mint authority to the wrong address. + +- `propose_new_admin(current_admin, proposed)` — admin-only. Stores a + `PendingTransfer` under `DataKey::PendingAdmin` and emits + `TransferProposed`. +- `accept_admin_ownership(acceptor)` — only the proposed address may + call. Overwrites `DataKey::Admin`, clears the pending record, emits + `TransferAccepted`. +- `cancel_admin_transfer(caller)` — callable by the current admin OR + the (typo'd) proposed address. Clears the pending record, emits + `TransferCancelled`. + +The timelock is **soft**; see `contracts/common::two_step` for details. + diff --git a/contracts/badge-nft/src/lib.rs b/contracts/badge-nft/src/lib.rs index 9729294..f0c925e 100644 --- a/contracts/badge-nft/src/lib.rs +++ b/contracts/badge-nft/src/lib.rs @@ -28,6 +28,11 @@ pub trait BadgeNFTInterface { fn get_badges(env: Env, learner: Address) -> Vec; fn get_badge_count(env: Env, learner: Address) -> u32; fn has_badge(env: Env, learner: Address, course_id: u32) -> bool; + fn upgrade_contract(env: Env, admin: Address, new_wasm_hash: soroban_sdk::BytesN<32>); + // ── Two-step admin transfer (Issue #20) ───────────────────── + fn propose_new_admin(env: Env, current_admin: Address, proposed: Address); + fn accept_admin_ownership(env: Env, acceptor: Address); + fn cancel_admin_transfer(env: Env, caller: Address); } #[contractevent] @@ -59,6 +64,9 @@ pub struct ContractUpgraded { // to avoid duplicate symbol errors at link time. #[cfg(feature = "contract")] mod contract_impl { + use contracts_common::two_step::{ + PendingTransfer, TransferAccepted, TransferCancelled, TransferProposed, + }; use soroban_sdk::{contract, contractimpl, Address, BytesN, Env, Vec}; use crate::types::{Badge, DataKey}; @@ -273,6 +281,95 @@ mod contract_impl { } .publish(&env); } + + // ── Two-step admin transfer (Issue #20) ────────────────── + + /// Stage 1 — propose a new admin. Only the current admin may call. + pub fn propose_new_admin( + env: Env, + current_admin: Address, + proposed: Address, + ) { + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the authorized registry" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + /// Stage 2 — accept the admin role. Only the proposed address may call. + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + acceptor.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + /// Cancel a pending admin transfer. Callable by the proposed + /// address or the current admin. + pub fn cancel_admin_transfer(env: Env, caller: Address) { + caller.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } } } diff --git a/contracts/badge-nft/src/test.rs b/contracts/badge-nft/src/test.rs index c6f72c9..e43b931 100644 --- a/contracts/badge-nft/src/test.rs +++ b/contracts/badge-nft/src/test.rs @@ -448,3 +448,129 @@ fn test_has_badge_multiple_badges() { assert!(!client.has_badge(&learner, &4)); assert!(client.has_badge(&learner, &5)); } + +// ── Two-Step Admin Transfer Tests (Issue #20) ──────────────────────────── + +#[test] +fn test_propose_new_admin_emits_event() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &proposed); + + let events = env.events().all(); + assert_eq!(events.len(), 1, "TransferProposed event emitted"); + + let last = events.last().unwrap(); + let expected_topics: Vec = + (Symbol::new(&env, "transfer_proposed"), registry.clone(), proposed.clone()).into_val(&env); + assert_eq!(last.1, expected_topics); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the authorized registry")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let impostor = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let new_admin = Address::generate(&env); + let learner = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &new_admin); + client.accept_admin_ownership(&new_admin); + + // New admin can mint (the only admin-gated op). + client.mint_badge(&new_admin, &learner, &1); + assert!(client.has_badge(&learner, &1)); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &proposed); + client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(®istry); + client.accept_admin_ownership(&impostor); +} + +#[test] +fn test_cancel_admin_transfer_typo_recovery() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &typo); + + // Original admin catches the typo and cancels. + client.cancel_admin_transfer(®istry); + + // Registry authority unchanged — still able to mint. + let learner = Address::generate(&env); + client.mint_badge(®istry, &learner, &1); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_self_recovery() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &typo); + // Typo'd address can self-cancel. + client.cancel_admin_transfer(&typo); + + let learner = Address::generate(&env); + client.mint_badge(®istry, &learner, &1); +} + +#[test] +#[should_panic(expected = "Unauthorized: only proposer or current admin can cancel")] +fn test_cancel_admin_transfer_by_random_panics() { + let (env, client) = setup(); + let registry = Address::generate(&env); + let proposed = Address::generate(&env); + let random = Address::generate(&env); + + client.initialize(®istry); + client.propose_new_admin(®istry, &proposed); + client.cancel_admin_transfer(&random); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_cancel_admin_transfer_no_pending_panics() { + let (env, client) = setup(); + let registry = Address::generate(&env); + client.initialize(®istry); + client.cancel_admin_transfer(®istry); +} diff --git a/contracts/badge-nft/src/types.rs b/contracts/badge-nft/src/types.rs index 484c526..51af4ad 100644 --- a/contracts/badge-nft/src/types.rs +++ b/contracts/badge-nft/src/types.rs @@ -12,4 +12,6 @@ pub struct Badge { pub enum DataKey { Admin, UserBadges(Address), + /// Pending two-step admin transfer (Issue #20). + PendingAdmin, } diff --git a/contracts/common/src/lib.rs b/contracts/common/src/lib.rs index da4db71..61b0d97 100644 --- a/contracts/common/src/lib.rs +++ b/contracts/common/src/lib.rs @@ -10,6 +10,7 @@ pub mod auth; pub mod constants; pub mod errors; +pub mod two_step; pub mod types; // Re-export soroban-sdk for convenience diff --git a/contracts/common/src/two_step.rs b/contracts/common/src/two_step.rs new file mode 100644 index 0000000..f9923df --- /dev/null +++ b/contracts/common/src/two_step.rs @@ -0,0 +1,72 @@ +#![no_std] + +//! # Two-Step Transfer Helper (Issue #20) +//! +//! Shared types and events for two-step admin / role transfers across all +//! Orivex contracts. Each role in each contract follows the same triplet: +//! +//! 1. `propose_new_(env, current_admin, proposed)` — current admin (or +//! the role's current holder for self-keyed propose methods) starts the +//! transfer. A `PendingTransfer` is written to persistent storage under +//! a contract-defined key and `TransferProposed` is emitted. +//! 2. `accept_(env, acceptor)` — only the proposed address may accept. +//! The live storage slot is overwritten and the pending record is +//! cleared; `TransferAccepted` is emitted. +//! 3. `cancel_(env, caller)` — only the proposed address or the +//! current admin may cancel. The pending record is cleared and +//! `TransferCancelled` is emitted. +//! +//! ## Timelock +//! +//! This crate ships a **soft timelock** (see Issue #20 acceptance criteria): +//! acceptance and cancellation are both immediate. Off-chain monitors are +//! expected to alert on `TransferProposed` so communities can react before +//! the proposed address calls `accept_*`. A hard-timelock upgrade is a +//! straightforward follow-up that adds a `delay_seconds` field to +//! `PendingTransfer` and a check in `accept__*` callers. + +use soroban_sdk::{contractevent, contracttype, Address}; + +/// A pending two-step transfer proposal. +/// +/// Stored in the calling contract's persistent storage under a key of the +/// contract's choice (typically `DataKey::Pending`). The contract +/// reads it back during the corresponding `accept_` to authorize the +/// final write and clears the record on success. +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PendingTransfer { + /// Address proposed for the role. + pub proposed: Address, + /// Ledger timestamp when the proposal was made. + pub proposed_at: u64, +} + +/// Emitted when the current role holder proposes a new address. Topics: +/// the current value and the proposed value. Data: timestamp. +#[contractevent] +pub struct TransferProposed { + #[topic] + pub current: Address, + #[topic] + pub proposed: Address, + pub proposed_at: u64, +} + +/// Emitted when the proposed address accepts and the live value updates. +/// Topic: the new value (the address that just became live). +#[contractevent] +pub struct TransferAccepted { + #[topic] + pub new_value: Address, +} + +/// Emitted when a pending transfer is cancelled before acceptance. +/// Topics: the canceller and the address that was proposed. +#[contractevent] +pub struct TransferCancelled { + #[topic] + pub cancelled_by: Address, + #[topic] + pub was_proposed: Address, +} diff --git a/contracts/course-registry/README.md b/contracts/course-registry/README.md index 8594eda..d698a74 100644 --- a/contracts/course-registry/README.md +++ b/contracts/course-registry/README.md @@ -143,3 +143,27 @@ cargo test -p course-registry --lib - Add course deletion with proper cleanup - Support for multiple admin roles - Course versioning for content updates + +## Two-Step Admin & Wiring Transfer (Issue #20) + +`Admin`, `RewardPoolAddress`, and `BadgeNftAddress` are all rotated +through `propose → accept`. The single-step setters +(`set_reward_pool_address`, `set_badge_nft_address`) remain in place +for the initial wiring at deploy time; later rotations must go through +the two-step flow. + +| Role | Propose | Accept | Cancel | +| ---- | ------- | ------ | ------ | +| Admin | `propose_new_admin(current_admin, proposed)` | `accept_admin_ownership(acceptor)` | `cancel_admin_transfer(caller)` | +| RewardPoolAddress | `propose_new_reward_pool_address(current_admin, proposed)` | `accept_reward_pool_address(acceptor)` | `cancel_reward_pool_transfer(caller)` | +| BadgeNftAddress | `propose_new_badge_nft_address(current_admin, proposed)` | `accept_badge_nft_address(acceptor)` | `cancel_badge_nft_transfer(caller)` | + +The cancel step is callable by the *current* admin **or** the +(possibly typo'd) proposed address, so a fat-finger incident is +recoverable without the original admin even needing to re-issue. + +Timelock mode is **soft**: the proposed address may accept +immediately. Off-chain monitors should subscribe to events +`TransferProposed { current, proposed, proposed_at }`, +`TransferAccepted { new_value }`, and `TransferCancelled { cancelled_by, was_proposed }` +(in `contracts/common::two_step`). diff --git a/contracts/course-registry/src/lib.rs b/contracts/course-registry/src/lib.rs index 353f59f..02ad14b 100644 --- a/contracts/course-registry/src/lib.rs +++ b/contracts/course-registry/src/lib.rs @@ -19,6 +19,11 @@ pub const BASE_REWARD_AMOUNT: i128 = 10_0000000; // badges, and RewardPool payout triggering. use soroban_sdk::{contract, contractevent, contractimpl, Address, BytesN, Env}; +/// Re-exported two-step transfer events (Issue #20). +pub use contracts_common::two_step::{ + TransferAccepted, TransferCancelled, TransferProposed, +}; + pub mod types; use types::{Course, DataKey}; @@ -89,6 +94,279 @@ pub struct ContractUpgraded { #[contractimpl] impl CourseRegistry { + // ── Two-step admin transfer (Issue #20) ────────────────────── + + /// Stage 1 — propose a new admin. Only the current admin may call. + pub fn propose_new_admin(env: Env, current_admin: Address, proposed: Address) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the protocol admin" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + pub fn cancel_admin_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } + + // ── Two-step RewardPoolAddress transfer (Issue #20) ────── + // The single-step `set_reward_pool_address` remains for the + // initial bootstrap; updates after deployment require the + // two-step flow below. + + pub fn propose_new_reward_pool_address( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the protocol admin" + ); + + let current: Address = env + .storage() + .instance() + .get(&DataKey::RewardPoolAddress) + .expect("RewardPool address not configured"); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingRewardPool, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_reward_pool_address(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingRewardPool) + .expect("No pending RewardPool transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed RewardPool" + ); + + let new_value = pending.proposed.clone(); + env.storage() + .instance() + .set(&DataKey::RewardPoolAddress, &new_value); + env.storage().persistent().remove(&DataKey::PendingRewardPool); + + TransferAccepted { new_value }.publish(&env); + } + + pub fn cancel_reward_pool_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingRewardPool) + .expect("No pending RewardPool transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingRewardPool); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } + + // ── Two-step BadgeNftAddress transfer (Issue #20) ───────── + // The single-step `set_badge_nft_address` remains for the + // initial bootstrap; updates after deployment require the + // two-step flow below. + + pub fn propose_new_badge_nft_address( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the protocol admin" + ); + + let current: Address = env + .storage() + .instance() + .get(&DataKey::BadgeNftAddress) + .expect("BadgeNFT address not configured"); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingBadgeNft, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_badge_nft_address(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingBadgeNft) + .expect("No pending BadgeNFT transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed BadgeNFT" + ); + + let new_value = pending.proposed.clone(); + env.storage() + .instance() + .set(&DataKey::BadgeNftAddress, &new_value); + env.storage().persistent().remove(&DataKey::PendingBadgeNft); + + TransferAccepted { new_value }.publish(&env); + } + + pub fn cancel_badge_nft_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingBadgeNft) + .expect("No pending BadgeNFT transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Contract not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingBadgeNft); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } /// Sets the official Protocol Admin. Must be called once upon deployment. /// Sets the single Protocol Admin in instance storage at deploy time. /// Idempotent guards prevent re-initialization: the function panics if diff --git a/contracts/course-registry/src/test.rs b/contracts/course-registry/src/test.rs index 68df887..96d3b71 100644 --- a/contracts/course-registry/src/test.rs +++ b/contracts/course-registry/src/test.rs @@ -1112,3 +1112,228 @@ fn test_reward_distributed_only_on_final_module() { client.complete_module(&admin, &learner, &course_id); assert_eq!(token_sac.balance(&learner), 10_0000000); } + +// ── Two-Step Admin Transfer (Issue #20) ──────────────────────────────────── + +#[test] +fn test_propose_new_admin_emits_event() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&admin, &proposed); + + let events = env.events().all(); + assert_eq!(events.len(), 1, "TransferProposed event"); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the protocol admin")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let impostor = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let new_admin = Address::generate(&env); + let instructor = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&admin, &new_admin); + client.accept_admin_ownership(&new_admin); + + // New admin can now create courses. + let id = client.create_course(&new_admin, &instructor, &3, &dummy_hash(&env)); + assert_eq!(id, 1); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&admin, &proposed); + client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + client.initialize(&admin); + let impostor = Address::generate(&env); + client.accept_admin_ownership(&impostor); +} + +#[test] +fn test_cancel_admin_transfer_by_admin_recovers_from_typo() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&admin); + + // Original admin unchanged. + let instructor = Address::generate(&env); + let id = client.create_course(&admin, &instructor, &3, &dummy_hash(&env)); + assert_eq!(id, 1); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_self_recovery() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(&admin); + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&typo); + + let instructor = Address::generate(&env); + let id = client.create_course(&admin, &instructor, &3, &dummy_hash(&env)); + assert_eq!(id, 1); +} + +// ── Two-Step RewardPoolAddress Transfer (Issue #20) ──────────────────────── + +#[test] +fn test_propose_accept_reward_pool_address_happy_path() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let (reward_pool_client, _token_sac, _) = setup_reward_pool(&env, &admin); + let new_pool = Address::generate(&env); + + client.initialize(&admin); + client.set_reward_pool_address(&admin, &reward_pool_client.address); + client.propose_new_reward_pool_address(&admin, &new_pool); + client.accept_reward_pool_address(&new_pool); + + // Subsequent completions should now route to the new pool. We verify + // by seeding a pending reward against the old pool address and ensuring + // the live slot now points at `new_pool`. + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&DataKey::RewardPoolAddress) + .unwrap(); + assert_eq!(stored, new_pool); + }); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed RewardPool")] +fn test_accept_reward_pool_address_wrong_acceptor_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let (reward_pool_client, _, _) = setup_reward_pool(&env, &admin); + let proposed_pool = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(&admin); + client.set_reward_pool_address(&admin, &reward_pool_client.address); + client.propose_new_reward_pool_address(&admin, &proposed_pool); + client.accept_reward_pool_address(&impostor); +} + +#[test] +fn test_cancel_reward_pool_transfer_by_admin_recovers_typo() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let (reward_pool_client, _, _) = setup_reward_pool(&env, &admin); + let typo = Address::generate(&env); + + client.initialize(&admin); + client.set_reward_pool_address(&admin, &reward_pool_client.address); + client.propose_new_reward_pool_address(&admin, &typo); + client.cancel_reward_pool_transfer(&admin); + + // Live address unchanged. + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&DataKey::RewardPoolAddress) + .unwrap(); + assert_eq!(stored, reward_pool_client.address); + }); +} + +// ── Two-Step BadgeNftAddress Transfer (Issue #20) ────────────────────────── + +#[test] +fn test_propose_accept_badge_nft_address_happy_path() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let badge_client = setup_badge_nft(&env, &client.address); + let new_badge = setup_badge_nft(&env, &client.address); + let _ = new_badge.address; // deploy a second badge + + client.initialize(&admin); + client.set_badge_nft_address(&admin, &badge_client.address); + client.propose_new_badge_nft_address(&admin, &new_badge.address); + client.accept_badge_nft_address(&new_badge.address); + + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&DataKey::BadgeNftAddress) + .unwrap(); + assert_eq!(stored, new_badge.address); + }); + let _ = badge_client; // silence unused +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed BadgeNFT")] +fn test_accept_badge_nft_address_wrong_acceptor_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let badge_client = setup_badge_nft(&env, &client.address); + let proposed = setup_badge_nft(&env, &client.address); + let impostor = Address::generate(&env); + + client.initialize(&admin); + client.set_badge_nft_address(&admin, &badge_client.address); + client.propose_new_badge_nft_address(&admin, &proposed.address); + client.accept_badge_nft_address(&impostor); +} + +#[test] +fn test_cancel_badge_nft_transfer_by_admin_recovers_typo() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let badge_client = setup_badge_nft(&env, &client.address); + let typo = Address::generate(&env); + + client.initialize(&admin); + client.set_badge_nft_address(&admin, &badge_client.address); + client.propose_new_badge_nft_address(&admin, &typo); + client.cancel_badge_nft_transfer(&admin); + + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&DataKey::BadgeNftAddress) + .unwrap(); + assert_eq!(stored, badge_client.address); + }); +} diff --git a/contracts/course-registry/src/types.rs b/contracts/course-registry/src/types.rs index 55bdbfe..018d4a1 100644 --- a/contracts/course-registry/src/types.rs +++ b/contracts/course-registry/src/types.rs @@ -22,4 +22,11 @@ pub enum DataKey { /// whose reward payout failed. The learner can call /// `claim_completion_reward` to retry. PendingReward(Address, u32), + // ── Two-step transfer slots (Issue #20) ─────────────────── + /// Pending Admin transfer. + PendingAdmin, + /// Pending RewardPoolAddress transfer. + PendingRewardPool, + /// Pending BadgeNftAddress transfer. + PendingBadgeNft, } diff --git a/contracts/governance/Cargo.toml b/contracts/governance/Cargo.toml index d8bf913..f0561c3 100644 --- a/contracts/governance/Cargo.toml +++ b/contracts/governance/Cargo.toml @@ -18,7 +18,9 @@ testutils = ["soroban-sdk/testutils", "badge-nft/testutils"] [dependencies] soroban-sdk = { workspace = true } +contracts-common = { path = "../common", default-features = false } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } badge-nft = { path = "../badge-nft" } +contracts-common = { path = "../common" } diff --git a/contracts/governance/README.md b/contracts/governance/README.md index eeec33f..3a0fa52 100644 --- a/contracts/governance/README.md +++ b/contracts/governance/README.md @@ -10,3 +10,20 @@ Badge-weighted proposal lifecycle. - `execute_proposal(proposal_id)` — marks a passed proposal as executed. - `cancel_proposal(caller, proposal_id)` — proposer or admin only. - `upgrade_contract(admin, new_wasm_hash)` — admin-only WASM upgrade. + +## Two-Step Transfer (Issue #20) + +Both `Admin` and the `BadgeContractAddress` reference are rotated +through `propose → accept`. The `BadgeContractAddress` two-step is the +only path to rotate the badge contract after init. + +- `propose_new_admin(current_admin, proposed)` / `accept_admin_ownership(acceptor)` + / `cancel_admin_transfer(caller)`. +- `propose_new_badge_contract_address(current_admin, proposed)` / + `accept_badge_contract_address(acceptor)` / + `cancel_badge_contract_transfer(caller)`. + +All three steps for each role emit the shared events +`TransferProposed` / `TransferAccepted` / `TransferCancelled` from +`contracts/common::two_step`, with a soft timelock (immediate accept). + diff --git a/contracts/governance/src/lib.rs b/contracts/governance/src/lib.rs index a0199b0..265923e 100644 --- a/contracts/governance/src/lib.rs +++ b/contracts/governance/src/lib.rs @@ -18,6 +18,11 @@ use soroban_sdk::{ BytesN, Env, Symbol, Vec, }; +/// Re-exported two-step transfer events (Issue #20). +pub use contracts_common::two_step::{ + TransferAccepted, TransferCancelled, TransferProposed, +}; + pub mod types; pub use types::{DataKey, Proposal}; @@ -62,6 +67,186 @@ pub struct ContractUpgraded { #[contractimpl] impl Governance { + // ── Two-step admin transfer (Issue #20) ────────────────────── + + pub fn propose_new_admin(env: Env, current_admin: Address, proposed: Address) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + pub fn cancel_admin_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } + + // ── Two-step BadgeContractAddress transfer (Issue #20) ──────────── + // The BadgeContractAddress is set at `initialize` time. Once + // configured, two-step methods are the only way to rotate the + // badge contract reference. + + pub fn propose_new_badge_contract_address( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let current: Address = env + .storage() + .instance() + .get(&BADGE_NFT_KEY) + .expect("Contract not initialized"); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingBadgeContract, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_badge_contract_address(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingBadgeContract) + .expect("No pending BadgeContract transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed BadgeContract" + ); + + let new_value = pending.proposed.clone(); + env.storage() + .instance() + .set(&BADGE_NFT_KEY, &new_value); + env.storage() + .persistent() + .remove(&DataKey::PendingBadgeContract); + + TransferAccepted { new_value }.publish(&env); + } + + pub fn cancel_badge_contract_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingBadgeContract) + .expect("No pending BadgeContract transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage() + .persistent() + .remove(&DataKey::PendingBadgeContract); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } /// Initializes the governance contract with the admin and BadgeNFT contract address. /// Must be called once upon deployment. /// Bootstrap with admin and the BadgeNFT contract address used for diff --git a/contracts/governance/src/test.rs b/contracts/governance/src/test.rs index 5fc5f62..70e7338 100644 --- a/contracts/governance/src/test.rs +++ b/contracts/governance/src/test.rs @@ -418,3 +418,149 @@ fn test_upgrade_contract_not_initialized_panics() { let new_wasm_hash = BytesN::from_array(&env, &[0xabu8; 32]); governance_client.upgrade_contract(&attacker, &new_wasm_hash); } + +// ── Two-Step Admin Transfer (Issue #20) ──────────────────────────────────── + +#[test] +fn test_propose_new_admin_emits_event() { + let (env, governance_client, badge_client, admin) = setup(); + let proposed = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&admin, &proposed); + + let events = env.events().all(); + assert!(!events.is_empty(), "TransferProposed event emitted"); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the admin")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, governance_client, badge_client, admin) = setup(); + let impostor = Address::generate(&env); + let proposed = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, governance_client, badge_client, admin) = setup(); + let new_admin = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&admin, &new_admin); + governance_client.accept_admin_ownership(&new_admin); + + // New admin can call admin-only `cancel_proposal` (proposer or admin). + let proposer = Address::generate(&env); + seed_proposal(&env, &governance_client, 1, &proposer); + governance_client.cancel_proposal(&new_admin, &1); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, governance_client, badge_client, admin) = setup(); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&admin, &proposed); + governance_client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, governance_client, badge_client, admin) = setup(); + let impostor = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.accept_admin_ownership(&impostor); +} + +#[test] +fn test_cancel_admin_transfer_typo_recovery() { + let (env, governance_client, badge_client, admin) = setup(); + let typo = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&admin, &typo); + governance_client.cancel_admin_transfer(&admin); + + // Live admin authority unchanged. + let proposer = Address::generate(&env); + seed_proposal(&env, &governance_client, 1, &proposer); + governance_client.cancel_proposal(&admin, &1); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_self_recovery() { + let (env, governance_client, badge_client, admin) = setup(); + let typo = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_admin(&admin, &typo); + governance_client.cancel_admin_transfer(&typo); + + let proposer = Address::generate(&env); + seed_proposal(&env, &governance_client, 1, &proposer); + governance_client.cancel_proposal(&admin, &1); +} + +// ── Two-Step BadgeContractAddress Transfer (Issue #20) ───────────────────── + +#[test] +fn test_propose_accept_badge_contract_address_happy_path() { + let (env, governance_client, _badge_client, admin) = setup(); + let new_badge = Address::generate(&env); + + // Note: `initialize` set the badge contract to `_badge_client.address`. + governance_client.initialize(&admin, &_badge_client.address); + governance_client.propose_new_badge_contract_address(&admin, &new_badge); + governance_client.accept_badge_contract_address(&new_badge); + + env.as_contract(&governance_client.address, || { + use soroban_sdk::Symbol; + let stored: Address = env + .storage() + .instance() + .get(&Symbol::new(&env, "badge")) + .unwrap(); + assert_eq!(stored, new_badge); + }); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed BadgeContract")] +fn test_accept_badge_contract_address_wrong_acceptor_panics() { + let (env, governance_client, badge_client, admin) = setup(); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_badge_contract_address(&admin, &proposed); + governance_client.accept_badge_contract_address(&impostor); +} + +#[test] +fn test_cancel_badge_contract_transfer_by_admin_recovers_typo() { + let (env, governance_client, badge_client, admin) = setup(); + let typo = Address::generate(&env); + + governance_client.initialize(&admin, &badge_client.address); + governance_client.propose_new_badge_contract_address(&admin, &typo); + governance_client.cancel_badge_contract_transfer(&admin); + + env.as_contract(&governance_client.address, || { + use soroban_sdk::Symbol; + let stored: Address = env + .storage() + .instance() + .get(&Symbol::new(&env, "badge")) + .unwrap(); + assert_eq!(stored, badge_client.address); + }); +} diff --git a/contracts/governance/src/types.rs b/contracts/governance/src/types.rs index b2536e7..f5b8743 100644 --- a/contracts/governance/src/types.rs +++ b/contracts/governance/src/types.rs @@ -18,4 +18,9 @@ pub enum DataKey { Proposal(u32), UserVote(Address, u32), Admin, + // ── Two-step transfer slots (Issue #20) ──────────────── + /// Pending Admin transfer. + PendingAdmin, + /// Pending BadgeContractAddress transfer. + PendingBadgeContract, } diff --git a/contracts/quest-engine/Cargo.toml b/contracts/quest-engine/Cargo.toml index 14c8536..68d9092 100644 --- a/contracts/quest-engine/Cargo.toml +++ b/contracts/quest-engine/Cargo.toml @@ -13,6 +13,8 @@ testutils = ["soroban-sdk/testutils"] [dependencies] soroban-sdk = { workspace = true } +contracts-common = { path = "../common", default-features = false } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } +contracts-common = { path = "../common" } diff --git a/contracts/quest-engine/README.md b/contracts/quest-engine/README.md index c3db3c8..ac7a426 100644 --- a/contracts/quest-engine/README.md +++ b/contracts/quest-engine/README.md @@ -15,3 +15,23 @@ Build quests (employer-funded, peer-reviewed) and Explore quests (admin-verified - `verify_explore_quest(admin, learner, quest_id)` — admin-only pool payout. - `get_quest(quest_id)` / `get_submission(learner, quest_id)` — view accessors. - `upgrade_contract(admin, new_wasm_hash)` — admin-only WASM upgrade. +- `set_reward_pool_address(admin, new_address)` / `set_stake_vault_address(admin, new_address)` — admin-only single-step wiring setters (kept for the initial bootstrap). + +## Two-Step Admin & Wiring Transfer (Issue #20) + +`Admin`, `RewardPool`, and `StakeVault` are all rotated through +`propose → accept`. The single-step setters remain for the initial +wiring at deploy time; later rotations must go through the two-step +flow: + +| Role | Propose | Accept | Cancel | +| ---- | ------- | ------ | ------ | +| Admin | `propose_new_admin(current_admin, proposed)` | `accept_admin_ownership(acceptor)` | `cancel_admin_transfer(caller)` | +| RewardPool | `propose_new_reward_pool_address(current_admin, proposed)` | `accept_reward_pool_address(acceptor)` | `cancel_reward_pool_transfer(caller)` | +| StakeVault | `propose_new_stake_vault_address(current_admin, proposed)` | `accept_stake_vault_address(acceptor)` | `cancel_stake_vault_transfer(caller)` | + +Cancellable by the current admin OR by the (typo'd) proposed address. +Soft timelock: accept is immediate. Events are the shared +`TransferProposed` / `TransferAccepted` / `TransferCancelled` from +`contracts/common::two_step`. + diff --git a/contracts/quest-engine/src/lib.rs b/contracts/quest-engine/src/lib.rs index ef5e69c..a6c9396 100644 --- a/contracts/quest-engine/src/lib.rs +++ b/contracts/quest-engine/src/lib.rs @@ -125,6 +125,11 @@ pub struct StakeVaultUpdated { pub new_address: Address, } +/// Re-exported two-step transfer events (Issue #20). +pub use contracts_common::two_step::{ + TransferAccepted, TransferCancelled, TransferProposed, +}; + #[contract] pub struct QuestEngineContract; @@ -146,6 +151,277 @@ pub fn compute_learner_payout(reward: i128, multiplier_bps: u32) -> (i128, i128, #[contractimpl] impl QuestEngineContract { + // ── Two-step admin transfer (Issue #20) ────────────────────── + + /// Stage 1 — propose a new admin. Only the current admin may call. + pub fn propose_new_admin(env: Env, current_admin: Address, proposed: Address) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + pub fn cancel_admin_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } + + // ── Two-step RewardPool address transfer (Issue #20) ─────── + // The single-step `set_reward_pool_address` is preserved for the + // initial bootstrap; later changes require the two-step flow. + + pub fn propose_new_reward_pool_address( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let current: Address = env + .storage() + .instance() + .get(&DataKey::RewardPool) + .expect("RewardPool address not configured"); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingRewardPool, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_reward_pool_address(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingRewardPool) + .expect("No pending RewardPool transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed RewardPool" + ); + + let new_value = pending.proposed.clone(); + env.storage() + .instance() + .set(&DataKey::RewardPool, &new_value); + env.storage().persistent().remove(&DataKey::PendingRewardPool); + + TransferAccepted { new_value }.publish(&env); + } + + pub fn cancel_reward_pool_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingRewardPool) + .expect("No pending RewardPool transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingRewardPool); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } + + // ── Two-step StakeVault address transfer (Issue #20) ─────── + // The single-step `set_stake_vault_address` is preserved for the + // initial bootstrap; later changes require the two-step flow. + + pub fn propose_new_stake_vault_address( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let current: Address = env + .storage() + .instance() + .get(&DataKey::StakeVault) + .expect("StakeVault address not configured"); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingStakeVault, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current, + proposed, + proposed_at, + } + .publish(&env); + } + + pub fn accept_stake_vault_address(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingStakeVault) + .expect("No pending StakeVault transfer"); + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed StakeVault" + ); + + let new_value = pending.proposed.clone(); + env.storage() + .instance() + .set(&DataKey::StakeVault, &new_value); + env.storage().persistent().remove(&DataKey::PendingStakeVault); + + TransferAccepted { new_value }.publish(&env); + } + + pub fn cancel_stake_vault_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingStakeVault) + .expect("No pending StakeVault transfer"); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingStakeVault); + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } /// Initializes the QuestEngine contract with the token address and admin. pub fn initialize( env: Env, diff --git a/contracts/quest-engine/src/test.rs b/contracts/quest-engine/src/test.rs index 6f0e230..66dfd89 100644 --- a/contracts/quest-engine/src/test.rs +++ b/contracts/quest-engine/src/test.rs @@ -1202,3 +1202,183 @@ fn test_review_submission_multiplier_120_large_reward() { assert_eq!(token_balance(&env, &token_id, &learner), base_amount); assert_eq!(token_balance(&env, &token_id, &reward_pool), fee); } + +// ── Two-Step Admin Transfer (Issue #20) ──────────────────────────────────── + +#[test] +fn test_propose_new_admin_emits_event() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let proposed = Address::generate(&env); + + client.propose_new_admin(&admin, &proposed); + + let events = env.events().all(); + assert!(!events.is_empty(), "TransferProposed event emitted"); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the admin")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, client, _token, _rp, _admin, _sv) = setup(); + let impostor = Address::generate(&env); + let proposed = Address::generate(&env); + + client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let new_admin = Address::generate(&env); + + client.propose_new_admin(&admin, &new_admin); + client.accept_admin_ownership(&new_admin); + + // New admin can create Explore quests (admin-only). + let quest_id = client.create_explore_quest(&new_admin, &100, &BytesN::from_array(&env, &[1u8; 32])); + assert_eq!(quest_id, 1); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.propose_new_admin(&admin, &proposed); + client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, client, _token, _rp, _admin, _sv) = setup(); + let impostor = Address::generate(&env); + + client.accept_admin_ownership(&impostor); +} + +#[test] +fn test_cancel_admin_transfer_typo_recovery() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let typo = Address::generate(&env); + + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&admin); + + // Admin authority unchanged. + let quest_id = client.create_explore_quest(&admin, &100, &BytesN::from_array(&env, &[2u8; 32])); + assert_eq!(quest_id, 1); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_self_recovery() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let typo = Address::generate(&env); + + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&typo); + + let quest_id = client.create_explore_quest(&admin, &100, &BytesN::from_array(&env, &[3u8; 32])); + assert_eq!(quest_id, 1); +} + +// ── Two-Step RewardPool Address Transfer (Issue #20) ────────────────────── + +#[test] +fn test_propose_accept_reward_pool_address_happy_path() { + let (env, client, _token, _old_rp, admin, _sv) = setup(); + let new_pool = Address::generate(&env); + + client.propose_new_reward_pool_address(&admin, &new_pool); + client.accept_reward_pool_address(&new_pool); + + // Verify by reading storage via as_contract. + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&crate::types::DataKey::RewardPool) + .unwrap(); + assert_eq!(stored, new_pool); + }); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed RewardPool")] +fn test_accept_reward_pool_address_wrong_acceptor_panics() { + let (env, client, _token, _old_rp, admin, _sv) = setup(); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.propose_new_reward_pool_address(&admin, &proposed); + client.accept_reward_pool_address(&impostor); +} + +#[test] +fn test_cancel_reward_pool_transfer_by_admin_recovers_typo() { + let (env, client, _token, old_rp, admin, _sv) = setup(); + let typo = Address::generate(&env); + + client.propose_new_reward_pool_address(&admin, &typo); + client.cancel_reward_pool_transfer(&admin); + + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&crate::types::DataKey::RewardPool) + .unwrap(); + assert_eq!(stored, old_rp); + }); +} + +// ── Two-Step StakeVault Address Transfer (Issue #20) ────────────────────── + +#[test] +fn test_propose_accept_stake_vault_address_happy_path() { + let (env, client, _token, _rp, admin, _old_sv) = setup(); + let new_sv = Address::generate(&env); + + client.propose_new_stake_vault_address(&admin, &new_sv); + client.accept_stake_vault_address(&new_sv); + + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&crate::types::DataKey::StakeVault) + .unwrap(); + assert_eq!(stored, new_sv); + }); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed StakeVault")] +fn test_accept_stake_vault_address_wrong_acceptor_panics() { + let (env, client, _token, _rp, admin, _sv) = setup(); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.propose_new_stake_vault_address(&admin, &proposed); + client.accept_stake_vault_address(&impostor); +} + +#[test] +fn test_cancel_stake_vault_transfer_by_admin_recovers_typo() { + let (env, client, _token, _rp, admin, old_sv) = setup(); + let typo = Address::generate(&env); + + client.propose_new_stake_vault_address(&admin, &typo); + client.cancel_stake_vault_transfer(&admin); + + env.as_contract(&client.address, || { + let stored: Address = env + .storage() + .instance() + .get(&crate::types::DataKey::StakeVault) + .unwrap(); + assert_eq!(stored, old_sv); + }); +} diff --git a/contracts/quest-engine/src/types.rs b/contracts/quest-engine/src/types.rs index 531a86f..8ed3ede 100644 --- a/contracts/quest-engine/src/types.rs +++ b/contracts/quest-engine/src/types.rs @@ -43,4 +43,11 @@ pub enum DataKey { RewardPool, IsPaused, StakeVault, + // ── Two-step transfer slots (Issue #20) ─────────────── + /// Pending Admin transfer. + PendingAdmin, + /// Pending RewardPool transfer. + PendingRewardPool, + /// Pending StakeVault transfer. + PendingStakeVault, } diff --git a/contracts/reward-pool/Cargo.toml b/contracts/reward-pool/Cargo.toml index 1fd655a..d63f080 100644 --- a/contracts/reward-pool/Cargo.toml +++ b/contracts/reward-pool/Cargo.toml @@ -14,9 +14,11 @@ doctest = false [dependencies] soroban-sdk = { workspace = true } +contracts-common = { path = "../common", default-features = false } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } +contracts-common = { path = "../common" } [features] default = ["contract"] diff --git a/contracts/reward-pool/README.md b/contracts/reward-pool/README.md index 171da9d..f4f11c3 100644 --- a/contracts/reward-pool/README.md +++ b/contracts/reward-pool/README.md @@ -11,3 +11,25 @@ Central USDC reward distribution with an approved-spender allowlist. - `fund_pool(donor, amount)` — donor must authorize the token transfer. - `emergency_sweep(admin, recovery_wallet)` — admin-only full-balance rescue. - `upgrade_contract(admin, new_wasm_hash)` — admin-only WASM upgrade. + +## Two-Step Admin Transfer (Issue #20) + +The admin role is rotated through `propose → accept` so a typo or +compromised-key incident can be cancelled before locking out the +contract forever. + +- `propose_new_admin(current_admin, proposed)` — admin-only. Stores a + `PendingTransfer` under `DataKey::PendingAdmin` and emits + `TransferProposed { current, proposed, proposed_at }`. +- `accept_admin_ownership(acceptor)` — only the proposed address may + call. Overwrites `DataKey::Admin`, clears the pending record, emits + `TransferAccepted`. +- `cancel_admin_transfer(caller)` — callable by the current admin OR + the (typo'd) proposed address. Clears the pending record, emits + `TransferCancelled`. + +The timelock is **soft**: the proposed address can accept immediately. +Off-chain monitors are expected to alert on `TransferProposed` so +communities can react before acceptance. See +`contracts/common::two_step` for the shared types and events. + diff --git a/contracts/reward-pool/src/lib.rs b/contracts/reward-pool/src/lib.rs index be5cb21..8c09f15 100644 --- a/contracts/reward-pool/src/lib.rs +++ b/contracts/reward-pool/src/lib.rs @@ -28,6 +28,10 @@ pub trait RewardPoolInterface { fn fund_pool(env: Env, donor: Address, amount: i128); fn emergency_sweep(env: Env, admin: Address, recovery_wallet: Address); fn upgrade_contract(env: Env, admin: Address, new_wasm_hash: BytesN<32>); + // ── Two-step admin transfer (Issue #20) ────────────────────────── + fn propose_new_admin(env: Env, current_admin: Address, proposed: Address); + fn accept_admin_ownership(env: Env, acceptor: Address); + fn cancel_admin_transfer(env: Env, caller: Address); } #[contractevent] @@ -78,6 +82,9 @@ pub struct ContractUpgraded { #[cfg(feature = "contract")] mod contract_impl { + use contracts_common::two_step::{ + PendingTransfer, TransferAccepted, TransferCancelled, TransferProposed, + }; use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env}; use crate::types::DataKey; @@ -375,6 +382,101 @@ mod contract_impl { } .publish(&env); } + + // ── Two-step admin transfer (Issue #20) ────────────────────── + // The admin can transfer the admin role without a typo ever + // locking out the contract permanently: a typo is recovered by + // calling `cancel_admin_transfer` before the typo'd address + // calls `accept_admin_ownership`. See contracts/common::two_step. + + /// Stage 1 — propose a new admin. Only the current admin may call. + pub fn propose_new_admin( + env: Env, + current_admin: Address, + proposed: Address, + ) { + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + /// Stage 2 — accept the admin role. Only the proposed address may call. + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + acceptor.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + /// Cancel a pending admin transfer. Callable by either the + /// current admin or by the proposed address (so a typo can be + /// recovered before — or after — the typo'd caller reaches this + /// contract). + pub fn cancel_admin_transfer(env: Env, caller: Address) { + caller.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } } } diff --git a/contracts/reward-pool/src/test.rs b/contracts/reward-pool/src/test.rs index be5c37b..a6fe1b6 100644 --- a/contracts/reward-pool/src/test.rs +++ b/contracts/reward-pool/src/test.rs @@ -548,3 +548,183 @@ fn test_emergency_sweep_large_balance() { assert_eq!(token_client.balance(&client.address), 0); assert_eq!(token_client.balance(&recovery_wallet), 1_000_000); } + +// ── Two-Step Admin Transfer Tests (Issue #20) ─────────────────────────────── + +#[test] +fn test_propose_new_admin_success_emits_event() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin, &token); + + client.propose_new_admin(&admin, &proposed); + + // 1 `TransferProposed` event must have been emitted + let events = env.events().all(); + assert_eq!(events.len(), 2, "init event + propose event"); + + let last = events.last().unwrap(); + let expected_topics: Vec = + (Symbol::new(&env, "transfer_proposed"), admin.clone(), proposed.clone()).into_val(&env); + assert_eq!(last.1, expected_topics); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the admin")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let impostor = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let new_admin = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &new_admin); + + let events_before = env.events().all().len(); + client.accept_admin_ownership(&new_admin); + + // After accept, the new admin can call admin-only setters. + // We verify by adding an approved spender as the new admin. + let spender = Address::generate(&env); + client.add_approved_spender(&new_admin, &spender); + assert_eq!(env.events().all().len(), events_before + 2, "accept + spender"); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &proposed); + client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let new_admin = Address::generate(&env); + + client.initialize(&admin, &token); + client.accept_admin_ownership(&new_admin); +} + +#[test] +fn test_cancel_admin_transfer_by_proposer_recovers_from_typo() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + // Simulate a typo'd "new admin" address. + let typo_address = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &typo_address); + + // Original admin catches the typo and cancels. + client.cancel_admin_transfer(&admin); + + // Live admin is unchanged — admin-only call still works. + let spender = Address::generate(&env); + client.add_approved_spender(&admin, &spender); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_address_recovers() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &typo); + + // The typo'd address itself can cancel — no stranger can squat. + client.cancel_admin_transfer(&typo); + + let spender = Address::generate(&env); + client.add_approved_spender(&admin, &spender); +} + +#[test] +#[should_panic(expected = "Unauthorized: only proposer or current admin can cancel")] +fn test_cancel_admin_transfer_by_random_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + let random = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &proposed); + client.cancel_admin_transfer(&random); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_cancel_admin_transfer_no_pending_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + + client.initialize(&admin, &token); + client.cancel_admin_transfer(&admin); +} + +#[test] +fn test_propose_after_accept_replaces_pending_typo() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let typo = Address::generate(&env); + let correct = Address::generate(&env); + + client.initialize(&admin, &token); + + // Stage 1: admin proposes a typo'd address. + client.propose_new_admin(&admin, &typo); + + // Stage 2: admin replaces the typo with a correct address. + client.propose_new_admin(&admin, &correct); + + // The typo'd address can no longer accept (its proposal was overwritten). + // We assert the correct address can still accept and become live admin. + client.accept_admin_ownership(&correct); + let spender = Address::generate(&env); + client.add_approved_spender(&correct, &spender); +} + +#[test] +fn test_double_accept_panics_after_first_accept_clears_pending() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let new_admin = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &new_admin); + client.accept_admin_ownership(&new_admin); + // Second accept must panic — pending record was cleared. + client.accept_admin_ownership(&new_admin); +} diff --git a/contracts/reward-pool/src/types.rs b/contracts/reward-pool/src/types.rs index bfd71b8..b69395b 100644 --- a/contracts/reward-pool/src/types.rs +++ b/contracts/reward-pool/src/types.rs @@ -7,4 +7,8 @@ pub enum DataKey { Token, Spender(Address), IsPaused, + /// Pending two-step admin transfer. Holds a + /// `contracts_common::two_step::PendingTransfer` while a transfer + /// is in flight (issue #20). + PendingAdmin, } diff --git a/contracts/stake-vault/Cargo.toml b/contracts/stake-vault/Cargo.toml index 67da0e2..1a7e978 100644 --- a/contracts/stake-vault/Cargo.toml +++ b/contracts/stake-vault/Cargo.toml @@ -18,6 +18,8 @@ testutils = ["soroban-sdk/testutils"] [dependencies] soroban-sdk = { workspace = true } +contracts-common = { path = "../common", default-features = false } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } +contracts-common = { path = "../common" } diff --git a/contracts/stake-vault/README.md b/contracts/stake-vault/README.md index 8b48aa5..faaf5c8 100644 --- a/contracts/stake-vault/README.md +++ b/contracts/stake-vault/README.md @@ -9,3 +9,19 @@ Token staking, lock, and multiplier accessor. - `unstake(user)` — releases funds after the lock period elapsed. - `get_multiplier(user)` — basis-points multiplier based on stake tier. - `upgrade_contract(admin, new_wasm_hash)` — admin-only WASM upgrade. + +## Two-Step Admin Transfer (Issue #20) + +The admin role is rotated through `propose → accept` so a typo'd or +compromised key can be cancelled before locking the contract. + +- `propose_new_admin(current_admin, proposed)` — admin-only. +- `accept_admin_ownership(acceptor)` — only the proposed address may + call. +- `cancel_admin_transfer(caller)` — callable by the current admin OR + the (typo'd) proposed address. + +Follows the soft-timelock pattern shared across all Orivex contracts; +see `contracts/common::two_step` for the global event types +(`TransferProposed` / `TransferAccepted` / `TransferCancelled`). + diff --git a/contracts/stake-vault/src/lib.rs b/contracts/stake-vault/src/lib.rs index ea029f8..5b17651 100644 --- a/contracts/stake-vault/src/lib.rs +++ b/contracts/stake-vault/src/lib.rs @@ -58,8 +58,107 @@ pub struct ContractUpgraded { pub new_wasm_hash: BytesN<32>, } +/// Re-exported two-step transfer events (Issue #20). +pub use contracts_common::two_step::{ + TransferAccepted, TransferCancelled, TransferProposed, +}; + #[contractimpl] impl StakeVault { + // ── Two-step admin transfer (Issue #20) ────────────────── + + /// Stage 1 — propose a new admin. Only the current admin may call. + pub fn propose_new_admin( + env: Env, + current_admin: Address, + proposed: Address, + ) { + use contracts_common::two_step::PendingTransfer; + + current_admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + assert!( + current_admin == stored_admin, + "Unauthorized: Caller is not the admin" + ); + + let proposed_at = env.ledger().timestamp(); + env.storage().persistent().set( + &DataKey::PendingAdmin, + &PendingTransfer { + proposed: proposed.clone(), + proposed_at, + }, + ); + + TransferProposed { + current: current_admin, + proposed, + proposed_at, + } + .publish(&env); + } + + /// Stage 2 — accept the admin role. Only the proposed address may call. + pub fn accept_admin_ownership(env: Env, acceptor: Address) { + use contracts_common::two_step::PendingTransfer; + + acceptor.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + assert!( + acceptor == pending.proposed, + "Unauthorized: Acceptor is not the proposed admin" + ); + + let new_admin = pending.proposed.clone(); + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferAccepted { new_value: new_admin }.publish(&env); + } + + /// Cancel a pending admin transfer. Callable by the proposed + /// address or the current admin. + pub fn cancel_admin_transfer(env: Env, caller: Address) { + use contracts_common::two_step::PendingTransfer; + + caller.require_auth(); + + let pending: PendingTransfer = env + .storage() + .persistent() + .get(&DataKey::PendingAdmin) + .expect("No pending admin transfer"); + + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("Not initialized"); + + assert!( + caller == pending.proposed || caller == stored_admin, + "Unauthorized: only proposer or current admin can cancel" + ); + + env.storage().persistent().remove(&DataKey::PendingAdmin); + + TransferCancelled { + cancelled_by: caller, + was_proposed: pending.proposed, + } + .publish(&env); + } /// Initializes the StakeVault with admin and reward token /// addresses and emits `StakeVaultInitialized`. Admin-only at /// deploy time. Re-initialization panics with diff --git a/contracts/stake-vault/src/test.rs b/contracts/stake-vault/src/test.rs index 656fdc8..3f34f72 100644 --- a/contracts/stake-vault/src/test.rs +++ b/contracts/stake-vault/src/test.rs @@ -205,3 +205,133 @@ fn test_get_multiplier() { }); assert_eq!(client.get_multiplier(&user), 200); } + +// ── Two-Step Admin Transfer Tests (Issue #20) ──────────────────────────── + +#[test] +fn test_propose_new_admin_emits_event() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &proposed); + + let events = env.events().all(); + assert_eq!(events.len(), 2, "init event + propose event"); +} + +#[test] +#[should_panic(expected = "Unauthorized: Caller is not the admin")] +fn test_propose_new_admin_unauthorized_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let impostor = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&impostor, &proposed); +} + +#[test] +fn test_accept_admin_ownership_happy_path() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let new_admin = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &new_admin); + client.accept_admin_ownership(&new_admin); + + // After accept, the new admin can perform admin-only operations. + // `upgrade_contract` is admin-only; supply a dummy wasm hash. + let wasm = soroban_sdk::BytesN::from_array(&env, &[0u8; 32]); + client.upgrade_contract(&new_admin, &wasm); +} + +#[test] +#[should_panic(expected = "Unauthorized: Acceptor is not the proposed admin")] +fn test_accept_admin_ownership_wrong_acceptor_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &proposed); + client.accept_admin_ownership(&impostor); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_accept_admin_ownership_no_pending_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let impostor = Address::generate(&env); + + client.initialize(&admin, &token); + client.accept_admin_ownership(&impostor); +} + +#[test] +fn test_cancel_admin_transfer_typo_recovery() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&admin); + + // Admin authority is unchanged. + let wasm = soroban_sdk::BytesN::from_array(&env, &[0u8; 32]); + client.upgrade_contract(&admin, &wasm); +} + +#[test] +fn test_cancel_admin_transfer_by_typo_self_recovery() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let typo = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &typo); + client.cancel_admin_transfer(&typo); + + let wasm = soroban_sdk::BytesN::from_array(&env, &[0u8; 32]); + client.upgrade_contract(&admin, &wasm); +} + +#[test] +#[should_panic( + expected = "Unauthorized: only proposer or current admin can cancel" +)] +fn test_cancel_admin_transfer_by_random_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + let proposed = Address::generate(&env); + let random = Address::generate(&env); + + client.initialize(&admin, &token); + client.propose_new_admin(&admin, &proposed); + client.cancel_admin_transfer(&random); +} + +#[test] +#[should_panic(expected = "No pending admin transfer")] +fn test_cancel_admin_transfer_no_pending_panics() { + let (env, client) = setup(); + let admin = Address::generate(&env); + let token = Address::generate(&env); + + client.initialize(&admin, &token); + client.cancel_admin_transfer(&admin); +} diff --git a/contracts/stake-vault/src/types.rs b/contracts/stake-vault/src/types.rs index 615c54c..addf4dc 100644 --- a/contracts/stake-vault/src/types.rs +++ b/contracts/stake-vault/src/types.rs @@ -13,4 +13,6 @@ pub enum DataKey { Admin, Token, UserStake(Address), + /// Pending two-step admin transfer (Issue #20). + PendingAdmin, }