diff --git a/.cargo/audit.toml b/.cargo/audit.toml new file mode 100644 index 000000000..d263e725b --- /dev/null +++ b/.cargo/audit.toml @@ -0,0 +1,9 @@ +# `h2` is only present through the off-chain `credence_admin_cli` RPC client: +# soroban-client 0.5.8 -> reqwest 0.11 -> hyper 0.14 -> h2 0.3. The advisory +# is an unbounded-empty-DATA-frame client availability issue; it cannot affect +# contract WASM, contract state, or submitted transactions. The upstream +# soroban-client release line still requires reqwest 0.11, while the advisory +# fix requires h2 >= 0.4.16. Reassess this exception when soroban-client moves +# to reqwest/h2 0.4 or later. +[advisories] +ignore = ["RUSTSEC-2026-0258"] diff --git a/contracts/admin/ATOMICITY.md b/contracts/admin/ATOMICITY.md new file mode 100644 index 000000000..77479ae32 --- /dev/null +++ b/contracts/admin/ATOMICITY.md @@ -0,0 +1,22 @@ +# Admin operation atomicity + +Administrative transactions execute in Soroban's atomic invocation boundary: +if an entrypoint returns an error, its storage writes and contract events are +rolled back together. The admin contract validates all ownership-transfer +preconditions before its first ownership write. + +## Ownership invariant + +`transfer_ownership` records only a pending proposal. `accept_ownership` +rechecks that the pending candidate is currently an active, unsuspended +`SuperAdmin` after the timelock. A candidate removed, demoted, deactivated, or +suspended during that window cannot receive ownership. The failed acceptance +leaves the owner, pending owner, proposal timestamp, and emitted event set +unchanged, so the current owner can recover by cancelling or replacing the +proposal through the existing transfer flow. + +This is compatible with the public interface and storage layout: no migration +is required. It intentionally adds one failure condition at acceptance for a +proposal whose candidate is no longer currently eligible. The security model +assumes Soroban preserves atomicity for a failed invocation and that the +ledger timestamp is the authoritative timelock and suspension clock. diff --git a/contracts/admin/src/lib.rs b/contracts/admin/src/lib.rs index 5049ea0ed..170b48676 100644 --- a/contracts/admin/src/lib.rs +++ b/contracts/admin/src/lib.rs @@ -23,6 +23,8 @@ pub mod pausable; +#[cfg(test)] +mod test_atomic_rollback; /// Event schema regression tests — verifies topic/data layout for every /// role event without involving contract storage. #[cfg(test)] @@ -773,20 +775,10 @@ impl AdminContract { panic_with_error!(&e, ContractError::InvalidPauseAction); } - // Verify new owner is a SuperAdmin - let new_owner_info: AdminInfo = e - .storage() - .instance() - .get(&DataKey::AdminInfo(new_owner.clone())) - .unwrap_or_else(|| panic_with_error!(&e, ContractError::NotAdmin)); - - if new_owner_info.role != AdminRole::SuperAdmin { - panic_with_error!(&e, ContractError::NotAdmin); - } - - if !new_owner_info.active { - panic_with_error!(&e, ContractError::AlreadyDeactivated); - } + // A suspended or deactivated admin must not be able to receive durable + // ownership. This check is repeated by `accept_ownership`, because the + // candidate's status can change during the timelock. + Self::require_effective_super_admin(&e, &new_owner); // Store pending owner and proposal timestamp for timelock e.storage() @@ -852,6 +844,12 @@ impl AdminContract { panic_with_error!(&e, ContractError::TimelockNotReady); } + // Revalidate immediately before the first ownership write. A proposal + // is only an intent: its candidate may have been removed, deactivated, + // suspended, or demoted while the timelock elapsed. Failing here leaves + // the owner, pending owner, timestamp, and event stream untouched. + Self::require_effective_super_admin(&e, &pending_owner); + // Get current owner for event emission let previous_owner: Address = e .storage() @@ -1157,6 +1155,30 @@ impl AdminContract { } } + /// Require an owner candidate to be an effective SuperAdmin now. + /// + /// Ownership proposals are intentionally revalidated at acceptance, not + /// trusted based on the state at proposal time. This preserves the + /// two-step transfer API while preventing a stale proposal from granting + /// durable authority to an inactive, suspended, removed, or demoted admin. + fn require_effective_super_admin(e: &Env, candidate: &Address) { + let admin_info: AdminInfo = e + .storage() + .instance() + .get(&DataKey::AdminInfo(candidate.clone())) + .unwrap_or_else(|| panic_with_error!(e, ContractError::NotAdmin)); + + if admin_info.role != AdminRole::SuperAdmin { + panic_with_error!(e, ContractError::NotAdmin); + } + if !admin_info.active { + panic_with_error!(e, ContractError::AlreadyDeactivated); + } + if e.ledger().timestamp() < admin_info.suspended_until { + panic_with_error!(e, ContractError::AdminSuspended); + } + } + /// Historical role check: verify that `actor` held at least `role` at /// ledger timestamp `at_ledger`. /// diff --git a/contracts/admin/src/test_atomic_rollback.rs b/contracts/admin/src/test_atomic_rollback.rs new file mode 100644 index 000000000..aaa497dc1 --- /dev/null +++ b/contracts/admin/src/test_atomic_rollback.rs @@ -0,0 +1,70 @@ +//! Regression tests for atomic ownership-transfer failure handling. +//! +//! These use the generated contract client (`try_accept_ownership`) so the +//! assertions exercise Soroban's transaction boundary rather than a direct +//! Rust call. A rejected acceptance must retain the current owner, proposal, +//! and event stream exactly as they were before the attempted transaction. + +use crate::*; +use soroban_sdk::{ + testutils::{Address as _, Ledger as _}, + Address, Env, +}; + +fn setup() -> (Env, Address, AdminContractClient<'static>, Address, Address) { + let env = Env::default(); + let contract_id = env.register_contract(None, AdminContract); + let client = AdminContractClient::new(&env, &contract_id); + let owner = Address::generate(&env); + let candidate = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&owner, &1, &100); + client.add_admin(&owner, &candidate, &AdminRole::SuperAdmin); + client.transfer_ownership(&owner, &candidate); + env.ledger() + .with_mut(|ledger| ledger.timestamp += OWNERSHIP_TRANSFER_TIMELOCK); + + (env, contract_id, client, owner, candidate) +} + +#[test] +fn rejected_acceptance_rolls_back_when_candidate_is_deactivated() { + let (env, contract_id, client, owner, candidate) = setup(); + // Failure injection at the acceptance boundary: a candidate can become + // inactive through a future administrative recovery path. We write that + // terminal state directly because peer SuperAdmins cannot deactivate one + // another through the public permission model. + env.as_contract(&contract_id, || { + let mut info: AdminInfo = env + .storage() + .instance() + .get(&DataKey::AdminInfo(candidate.clone())) + .unwrap(); + info.active = false; + env.storage() + .instance() + .set(&DataKey::AdminInfo(candidate.clone()), &info); + }); + + let events_before = env.events().all().len(); + assert!(client.try_accept_ownership(&candidate).is_err()); + + assert_eq!(client.get_owner(), owner); + assert_eq!(client.get_pending_owner(), Some(candidate)); + assert_eq!(env.events().all().len(), events_before); +} + +#[test] +fn rejected_acceptance_rolls_back_when_candidate_is_suspended() { + let (env, _contract_id, client, owner, candidate) = setup(); + let suspension_end = env.ledger().timestamp() + 1; + client.suspend_admin(&owner, &candidate, &suspension_end); + + let events_before = env.events().all().len(); + assert!(client.try_accept_ownership(&candidate).is_err()); + + assert_eq!(client.get_owner(), owner); + assert_eq!(client.get_pending_owner(), Some(candidate)); + assert_eq!(env.events().all().len(), events_before); +}