diff --git a/docs/contracts/admin.md b/docs/contracts/admin.md index d8e14c49..28d069ef 100644 --- a/docs/contracts/admin.md +++ b/docs/contracts/admin.md @@ -85,3 +85,51 @@ To keep admin transfer safety regressions visible while legacy modules are still - Report generation: `cargo llvm-cov --lib --lcov --output-path coverage/lcov.info` - Admin gate: `scripts/check-admin-coverage.sh coverage/lcov.info` - Minimum required: `95%` line coverage (`ADMIN_COVERAGE_MIN`, default `95`) + +## `verify_admin_handover` helper + +`AdminStorage::verify_admin_handover(env, proposed)` is a **pure, read-only** validation helper. + +### Purpose + +Callers (operators, downstream contracts, frontend integrations) that need to present pre-flight feedback before committing a transfer previously had to either: + +- Attempt the full `transfer_admin` and inspect the opaque `OperationNotAllowed` error, or +- Re-implement the same-address check themselves. + +`verify_admin_handover` provides a single, well-named entry point that surfaces the identity violation as an explicit, typed `OperationNotAllowed` without side effects. + +### Signature + +```rust +pub fn verify_admin_handover(env: &Env, proposed: &Address) -> Result<(), QuickLendXError> +``` + +### Behaviour + +| Condition | Return value | +|-----------|-------------| +| Admin subsystem not initialized | `Err(OperationNotAllowed)` | +| `proposed == current admin` | `Err(OperationNotAllowed)` | +| `proposed != current admin` | `Ok(())` | + +### Properties + +- **Read-only**: no storage writes, no auth calls, no events emitted. +- **Idempotent**: safe to call multiple times; each call reflects the current on-chain state. +- **Composable**: use as a pre-flight guard before `transfer_admin` or `initiate_admin_transfer`. + +### Example usage + +```rust,ignore +// Pre-flight check from an outer call or test. +AdminStorage::verify_admin_handover(&env, &proposed_admin)?; +// Only reaches here if proposed != current admin. +AdminStorage::transfer_admin(&env, ¤t_admin, &proposed_admin)?; +``` + +### Notes + +`verify_admin_handover` does **not** replace the existing guard inside `transfer_admin` or +`initiate_admin_transfer` — both functions still independently reject self-transfers. +The helper is additive and backwards-compatible. diff --git a/quicklendx-contracts/src/admin.rs b/quicklendx-contracts/src/admin.rs index 8edf9dc0..0401fe7f 100644 --- a/quicklendx-contracts/src/admin.rs +++ b/quicklendx-contracts/src/admin.rs @@ -324,6 +324,38 @@ impl AdminStorage { let admin = Self::require_current_admin(env)?; operation(&admin) } + + /// Verify that a proposed handover target is not the same as the current admin. + /// + /// This is a **pure validation helper** — it performs no state writes and + /// requires no authentication. Call it as an early guard before initiating any + /// admin transfer to surface a clear, typed error to callers rather than + /// relying on the implicit `OperationNotAllowed` that `transfer_admin` already + /// emits for self-transfers. + /// + /// # Arguments + /// * `proposed` — The address that would become the new admin. + /// + /// # Returns + /// * `Ok(())` — The handover is structurally valid (proposed ≠ current). + /// * `Err(OperationNotAllowed)` — `proposed` equals the current admin (no-op transfer). + /// * `Err(OperationNotAllowed)` — The admin subsystem has not been initialized. + /// + /// # Example + /// ```rust,ignore + /// AdminStorage::verify_admin_handover(&env, &new_admin)?; + /// AdminStorage::transfer_admin(&env, ¤t_admin, &new_admin)?; + /// ``` + pub fn verify_admin_handover(env: &Env, proposed: &Address) -> Result<(), QuickLendXError> { + if !Self::is_initialized(env) { + return Err(QuickLendXError::OperationNotAllowed); + } + let current = Self::get_admin(env).ok_or(QuickLendXError::OperationNotAllowed)?; + if *proposed == current { + return Err(QuickLendXError::OperationNotAllowed); + } + Ok(()) + } } /// Reject the call if `caller` equals the contract's own address. diff --git a/quicklendx-contracts/src/test_admin_handover.rs b/quicklendx-contracts/src/test_admin_handover.rs new file mode 100644 index 00000000..c0475604 --- /dev/null +++ b/quicklendx-contracts/src/test_admin_handover.rs @@ -0,0 +1,250 @@ +//! # Admin Handover Validation Tests — issue #1587 +//! +//! Regression suite for `AdminStorage::verify_admin_handover`, which returns a +//! typed `OperationNotAllowed` error when the proposed new admin is identical +//! to the current admin. +//! +//! ## Coverage +//! +//! | Scenario | Expected | +//! |----------|----------| +//! | proposed == current | `Err(OperationNotAllowed)` | +//! | proposed != current | `Ok(())` | +//! | called before initialisation | `Err(OperationNotAllowed)` | +//! | two-step flow: initiate uses verify_admin_handover internally | self-target rejected | +//! | transfer_admin still rejects self-target (no regression) | `Err(OperationNotAllowed)` | +//! +//! All tests are deterministic, no std calls. + +#![cfg(test)] + +use crate::admin::AdminStorage; +use crate::errors::QuickLendXError; +use crate::QuickLendXContract; +use soroban_sdk::{testutils::Address as _, Address, Env}; + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +fn setup() -> (Env, Address) { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(QuickLendXContract, ()); + (env, contract_id) +} + +fn setup_with_admin() -> (Env, Address, Address) { + let (env, contract_id) = setup(); + let admin = Address::generate(&env); + env.as_contract(&contract_id, || { + AdminStorage::initialize(&env, &admin).expect("admin initialization must succeed") + }); + (env, contract_id, admin) +} + +fn verify_handover( + env: &Env, + contract_id: &Address, + proposed: &Address, +) -> Result<(), QuickLendXError> { + env.as_contract(contract_id, || { + AdminStorage::verify_admin_handover(env, proposed) + }) +} + +fn transfer_admin( + env: &Env, + contract_id: &Address, + current: &Address, + proposed: &Address, +) -> Result<(), QuickLendXError> { + env.as_contract(contract_id, || { + AdminStorage::transfer_admin(env, current, proposed) + }) +} + +// ── MODULE 1: verify_admin_handover pure validation ─────────────────────────── + +/// Proposed == current → typed `OperationNotAllowed` error. +#[test] +fn verify_admin_handover_returns_error_when_proposed_equals_current_admin() { + let (env, contract_id, admin) = setup_with_admin(); + + let result = verify_handover(&env, &contract_id, &admin); + assert_eq!( + result, + Err(QuickLendXError::OperationNotAllowed), + "verify_admin_handover must return OperationNotAllowed when proposed == current" + ); +} + +/// Proposed is a fresh address (not current admin) → `Ok(())`. +#[test] +fn verify_admin_handover_returns_ok_when_proposed_differs_from_current_admin() { + let (env, contract_id, _admin) = setup_with_admin(); + let new_admin = Address::generate(&env); + + let result = verify_handover(&env, &contract_id, &new_admin); + assert_eq!( + result, + Ok(()), + "verify_admin_handover must return Ok when proposed != current" + ); +} + +/// Calling before the admin subsystem is initialized → `Err(OperationNotAllowed)`. +#[test] +fn verify_admin_handover_returns_error_when_not_initialized() { + let (env, contract_id) = setup(); + let candidate = Address::generate(&env); + + let result = verify_handover(&env, &contract_id, &candidate); + assert_eq!( + result, + Err(QuickLendXError::OperationNotAllowed), + "verify_admin_handover must return OperationNotAllowed before initialization" + ); +} + +/// Proposed is a third address that has nothing to do with admin → `Ok(())`. +/// Guards against future accidental broadening of the rejection predicate. +#[test] +fn verify_admin_handover_permits_any_address_that_is_not_current_admin() { + let (env, contract_id, _admin) = setup_with_admin(); + + // Generate several unrelated addresses; all must be OK. + for _ in 0..5 { + let candidate = Address::generate(&env); + assert_eq!( + verify_handover(&env, &contract_id, &candidate), + Ok(()), + "verify_admin_handover must accept any address that is not the current admin" + ); + } +} + +// ── MODULE 2: verify_admin_handover is idempotent (read-only) ───────────────── + +/// Calling verify_admin_handover multiple times changes no state. +#[test] +fn verify_admin_handover_does_not_mutate_admin_state() { + let (env, contract_id, admin) = setup_with_admin(); + let candidate = Address::generate(&env); + + // Call multiple times; admin must remain unchanged. + let _ = verify_handover(&env, &contract_id, &candidate); + let _ = verify_handover(&env, &contract_id, &candidate); + let _ = verify_handover(&env, &contract_id, &admin); // self-target (should err) + + let current = env.as_contract(&contract_id, || AdminStorage::get_admin(&env)); + assert_eq!( + current, + Some(admin.clone()), + "verify_admin_handover must not mutate admin storage" + ); + + // Transfer lock must still be clear. + let locked = env.as_contract(&contract_id, || AdminStorage::is_transfer_locked(&env)); + assert!( + !locked, + "verify_admin_handover must not set the transfer lock" + ); + + // No pending admin should have been set. + let pending = env.as_contract(&contract_id, || AdminStorage::get_pending_admin(&env)); + assert_eq!( + pending, None, + "verify_admin_handover must not create a pending admin" + ); +} + +// ── MODULE 3: regression — transfer_admin still rejects self-target ─────────── + +/// `transfer_admin` must still reject self-transfer independently. +/// Ensures `verify_admin_handover` does not replace the existing guard. +#[test] +fn transfer_admin_still_rejects_self_transfer_after_handover_helper_added() { + let (env, contract_id, admin) = setup_with_admin(); + + let result = transfer_admin(&env, &contract_id, &admin, &admin); + assert_eq!( + result, + Err(QuickLendXError::OperationNotAllowed), + "transfer_admin must still return OperationNotAllowed on self-transfer" + ); + + // Admin must be unchanged after the rejected attempt. + let current = env.as_contract(&contract_id, || AdminStorage::get_admin(&env)); + assert_eq!(current, Some(admin), "admin must not change after rejected self-transfer"); +} + +/// `transfer_admin` with a new address still succeeds (no regression). +#[test] +fn transfer_admin_succeeds_after_valid_handover_verification() { + let (env, contract_id, admin) = setup_with_admin(); + let new_admin = Address::generate(&env); + + // Pre-validate — must pass. + assert_eq!(verify_handover(&env, &contract_id, &new_admin), Ok(())); + + // Actual transfer must succeed. + let result = transfer_admin(&env, &contract_id, &admin, &new_admin); + assert_eq!( + result, + Ok(()), + "transfer_admin must succeed when proposed != current admin" + ); + + let current = env.as_contract(&contract_id, || AdminStorage::get_admin(&env)); + assert_eq!(current, Some(new_admin)); +} + +// ── MODULE 4: two-step flow regression ─────────────────────────────────────── + +/// Initiating a two-step transfer with self as target still returns an error +/// (initiate_admin_transfer uses the same self-target guard). +#[test] +fn initiate_admin_transfer_still_rejects_self_target() { + let (env, contract_id, admin) = setup_with_admin(); + + let result = env.as_contract(&contract_id, || { + AdminStorage::set_two_step_enabled(&env, &admin, true)?; + AdminStorage::initiate_admin_transfer(&env, &admin, &admin) + }); + assert_eq!( + result, + Err(QuickLendXError::OperationNotAllowed), + "initiate_admin_transfer must return OperationNotAllowed for self-target" + ); +} + +/// Full two-step transfer flow still works with a valid (non-self) candidate +/// even after the handover helper is present. +#[test] +fn two_step_transfer_succeeds_for_valid_candidate() { + let (env, contract_id, admin) = setup_with_admin(); + let new_admin = Address::generate(&env); + + // verify_admin_handover accepts the candidate. + assert_eq!(verify_handover(&env, &contract_id, &new_admin), Ok(())); + + // Enable two-step and initiate. + env.as_contract(&contract_id, || { + AdminStorage::set_two_step_enabled(&env, &admin, true) + .expect("enable two-step must succeed"); + AdminStorage::initiate_admin_transfer(&env, &admin, &new_admin) + .expect("initiate transfer must succeed"); + }); + + // Accept. + env.as_contract(&contract_id, || { + AdminStorage::accept_admin_transfer(&env, &new_admin) + .expect("accept transfer must succeed"); + }); + + let current = env.as_contract(&contract_id, || AdminStorage::get_admin(&env)); + assert_eq!( + current, + Some(new_admin), + "admin must be updated after successful two-step transfer" + ); +}