From 9e1d7d62f496a189b1487e68b9b6f251c66c84e2 Mon Sep 17 00:00:00 2001 From: "Akinsuyi Philip." <91294691+Akinsuyiphilip@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:21:23 +0000 Subject: [PATCH] fix: let recovery veto scheduled admin operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Align the settlement timelock with the seven-day recovery window and reject scheduled execution while recovery is pending, preventing compromised-admin upgrades and transfers from racing past recovery. Generated with Codebuff 🤖 Co-Authored-By: Codebuff --- bettapay_common/src/constants.rs | 6 +++-- settlement_contract/src/admin.rs | 12 +++++++++ settlement_contract/src/lib.rs | 8 +++++- .../src/tests/timelock_tests.rs | 27 +++++++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/bettapay_common/src/constants.rs b/bettapay_common/src/constants.rs index d54ae9f3..6cfb3fbe 100644 --- a/bettapay_common/src/constants.rs +++ b/bettapay_common/src/constants.rs @@ -50,6 +50,8 @@ pub const TTL_THRESHOLD_LEDGERS: u32 = LEDGERS_PER_DAY * 14; pub const TTL_BUMP_LEDGERS: u32 = LEDGERS_PER_DAY * 30; /// Cooldown between `initiate_recovery` and `execute_recovery`: seven days, -/// expressed in seconds. Both contracts use the same delay window so they can -/// share a single definition. +/// expressed in seconds. Scheduled settlement administrative operations use a +/// delay of at least this long. This ordering is part of the threat model: +/// recovery must be able to veto compromised-admin upgrades and admin +/// transfers before they execute. pub const RECOVERY_DELAY_SECONDS: u64 = 7 * 24 * 60 * 60; diff --git a/settlement_contract/src/admin.rs b/settlement_contract/src/admin.rs index e3cf31b1..dac0a977 100644 --- a/settlement_contract/src/admin.rs +++ b/settlement_contract/src/admin.rs @@ -96,6 +96,11 @@ impl SettlementContract { ); } + /// Initiates recovery and vetoes every pending scheduled operation. + /// + /// Recovery is intentionally the emergency veto path: once the recovery + /// address authenticates, no operation scheduled under the compromised + /// admin can execute, including an upgrade or admin transfer. pub fn initiate_recovery(env: Env, new_admin: Address) { let recovery_address = read_recovery_address(&env); recovery_address.require_auth(); @@ -113,6 +118,7 @@ impl SettlementContract { env.storage() .instance() .set(&CommonDataKey::PendingRecovery, &pending); + // `PendingRecovery` itself is the veto marker checked by `execute`. events::emit_recovery_initiated(&env, &recovery_address, &new_admin, pending.execute_after); } @@ -283,6 +289,12 @@ impl SettlementContract { /// Executes a previously scheduled administrative operation. pub fn execute(env: Env, operation: Operation) { + if env.storage().instance().has(&CommonDataKey::PendingRecovery) { + // A pending recovery is an emergency veto over all scheduled ops. + // The recovery record remains until recovery execution/cancellation, + // so this also closes the race between the two transactions. + panic_with_error!(&env, SettlementError::ExecutionNotReady); + } let op_hash: BytesN<32> = env.crypto().sha256(&operation.clone().to_xdr(&env)).into(); let key = DataKey::ScheduledOperation(op_hash.clone()); diff --git a/settlement_contract/src/lib.rs b/settlement_contract/src/lib.rs index 1c4b1d6e..d1300c9e 100644 --- a/settlement_contract/src/lib.rs +++ b/settlement_contract/src/lib.rs @@ -219,7 +219,13 @@ pub(crate) const RULE_TTL_BUMP: u32 = LEDGERS_PER_DAY * 30; pub(crate) const MERCHANT_TTL_THRESHOLD: u32 = LEDGERS_PER_DAY * 14; pub(crate) const MERCHANT_TTL_BUMP: u32 = LEDGERS_PER_DAY * 30; -pub(crate) const DEFAULT_TIMELOCK_DELAY_SECONDS: u64 = 2 * 24 * 60 * 60; // 48 hours +/// Minimum delay for scheduled administrative operations. +/// +/// This matches the seven-day recovery window so the recovery address has +/// time to replace compromised admins before a scheduled upgrade or admin +/// transfer can execute. The recovery path is the veto authority for pending +/// schedules; ordinary admin cancellation remains available as well. +pub(crate) const DEFAULT_TIMELOCK_DELAY_SECONDS: u64 = 7 * 24 * 60 * 60; // Settlement-specific TTL policy for short-lived reads of admin / governance / // recovery addresses. Deliberately shorter than the protocol defaults so that diff --git a/settlement_contract/src/tests/timelock_tests.rs b/settlement_contract/src/tests/timelock_tests.rs index 26e3373e..1669f177 100644 --- a/settlement_contract/src/tests/timelock_tests.rs +++ b/settlement_contract/src/tests/timelock_tests.rs @@ -1,6 +1,7 @@ //! Regression coverage for the settlement administrative timelock. use crate::{Operation, DEFAULT_TIMELOCK_DELAY_SECONDS}; +use bettapay_common::constants::RECOVERY_DELAY_SECONDS; use soroban_sdk::testutils::{Address as _, Ledger}; use soroban_sdk::Address; @@ -27,6 +28,32 @@ fn scheduled_operation_executes_only_after_delay() { assert!(client.try_execute(&operation).is_err()); } +#[test] +fn recovery_vetoes_scheduled_operation_before_timelock_expiry() { + let (env, client, admins, recovery) = setup(); + let operation = Operation::TransferAdmin( + soroban_sdk::vec![&env, Address::generate(&env)], + 1, + ); + let admin = admins.get(0).unwrap(); + + client.schedule(&admin, &operation, &DEFAULT_TIMELOCK_DELAY_SECONDS); + env.ledger().with_mut(|ledger| { + ledger.timestamp += RECOVERY_DELAY_SECONDS; + }); + client.initiate_recovery(Address::generate(&env)); + + // Recovery begins at the same boundary as the timelock and must win the + // transaction race: a scheduled operation cannot execute while recovery + // is pending, even when its nominal delay has elapsed. + env.ledger().with_mut(|ledger| { + ledger.timestamp += DEFAULT_TIMELOCK_DELAY_SECONDS; + }); + assert!(client.try_execute(&operation).is_err()); + assert_eq!(client.get_admin(), admins); + assert_eq!(client.get_recovery_address(), recovery); +} + #[test] fn schedule_rejects_non_admin_and_insufficient_delay() { let (env, client, admins, merchant) = setup();