feat: add multi-sig confirm_upgrade step for contract upgrades (#1808)

CHANGELOG.md

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+- `confirm_upgrade` — multi-sig confirmation step for contract upgrades. A second authorized address must call `confirm_upgrade` after `schedule_upgrade` and before `execute_upgrade`. This ensures that a single compromised admin key cannot unilaterally deploy arbitrary WASM — both the admin key and the upgrader key must cooperate to execute an upgrade. `execute_upgrade` returns `Error::UpgradeNotConfirmed` (#58) if the confirmation is absent. The confirmation is automatically cleared by both `cancel_upgrade` and `execute_upgrade`.
+- `Error::UpgradeNotConfirmed` (#58) — new error code returned by `execute_upgrade` when the multi-sig confirmation step has not been completed.
+
+### Changed
+- `execute_upgrade` now requires a prior `confirm_upgrade` call. Existing upgrade flows must add a `confirm_upgrade(upgrader)` step between `schedule_upgrade` and `execute_upgrade`. See [docs/upgrade-safety.md](docs/upgrade-safety.md) for the updated procedure.
+- `cancel_upgrade` now also clears any recorded `confirm_upgrade` confirmation so it cannot carry over to a subsequent upgrade cycle.
+- `docs/upgrade-safety.md` — updated upgrade lifecycle diagram and procedure to include the `confirm_upgrade` step.
+- `docs/security.md` — added "Contract Upgrade Multi-Sig" section documenting the threat model and recommendations.
+- `docs/error-codes.md` — added entry for error code #58 `UpgradeNotConfirmed`.
+
 ### Added
 - `admin_resolve_stalled_match` — admin-gated recovery function for Active matches stuck after the 24-hour player rollback window expires and the oracle has failed to submit a result for more than 7 days. Provides a bounded recovery path so funds are never permanently locked in the event of prolonged oracle outage or lost signing keys.
 - `ADMIN_STALL_WINDOW_SECONDS` constant (7 days) — configurable threshold after which admin can invoke stall resolution, long enough not to compete with the player-initiated 24-hour rollback window.

README.md

@@ -119,6 +119,7 @@ contract source.  For a machine-readable listing, see the
 | `match` / `claim` | `claim_vested_payout` | `(match_id, player, amount_claimed, payout_token)` |
 | `upgrade` / `sched` | `schedule_upgrade` | `(new_wasm_hash, scheduled_at)` |
 | `upgrade` / `cancel` | `cancel_upgrade` | `()` |
+| `upgrade` / `confirm` | `confirm_upgrade` | `upgrader_address` |
 | `upgrade` / `exec` | `execute_upgrade` | `new_wasm_hash` |
 | `upgrade` / `migrated` | `migrate_state` | `(current, target_version)` |
 

contracts/escrow/contract-spec.json

@@ -1,6 +1,6 @@
 {
   "_comment": "Baseline ABI snapshot for the escrow contract. Generated by the abi-snapshot CI job via `stellar contract inspect`. Do NOT edit this file by hand — update it using the procedure in CONTRIBUTING.md#updating-the-abi-snapshot.",
-  "_generated_at": "2026-08-27",
+  "_generated_at": "2026-10-20",
   "_contract": "contracts/escrow",
   "_generator": "stellar contract inspect --wasm target/wasm32-unknown-unknown/release/escrow.wasm --output json",
   "functions": [
@@ -53,6 +53,7 @@
     { "name": "claim_vested_payout" },
     { "name": "register_oracle_with_stake" },
     { "name": "schedule_upgrade" },
+    { "name": "confirm_upgrade" },
     { "name": "execute_upgrade" },
     { "name": "get_platform_stats" },
     { "name": "get_player_stats" }

contracts/escrow/src/errors.rs

@@ -86,4 +86,8 @@ pub enum Error {
     /// `set_dispute_period`, `set_minimum_hold_duration`, and `set_fee_tiers`
     /// when the supplied value is above the configured maximum.
     ConfigValueTooHigh = 57,
+    /// `execute_upgrade` requires a second authorized address to have called
+    /// `confirm_upgrade` after `schedule_upgrade`. This error is returned when
+    /// no confirmation has been recorded for the pending upgrade hash.
+    UpgradeNotConfirmed = 58,
 }

contracts/escrow/src/lib.rs

@@ -48,7 +48,7 @@ use types::{
     DisputeState, FeeTier, Match, MatchState, OracleRotationState, PendingAdminProposal,
     PendingOracleRotation, Platform, PlatformStats, PlayerBalanceSnapshot, PlayerFreezeKey,
     PlayerRating, PlayerRatingKey, PlayerStats, PlayerTier, ProtocolConfig, SnapshotReason,
-    TempOracleRotation, TokenSymbolCacheKey, Winner,
+    TempOracleRotation, TokenSymbolCacheKey, UpgradeKey, Winner,
 };
 
 /// ~30 days at 5s/ledger. Used as the default TTL and expiration threshold.
@@ -6124,13 +6124,54 @@ impl EscrowContract {
         env.storage()
             .instance()
             .remove(&DataKey::PendingUpgradeHash);
+        // Clear any confirmation recorded by confirm_upgrade so it cannot
+        // carry over to a subsequent upgrade cycle.
+        env.storage().instance().remove(&UpgradeKey::ConfirmedBy);
 
         env.events()
             .publish((Symbol::new(&env, "upgrade"), symbol_short!("cancel")), ());
 
         Ok(())
     }
 
+    /// Confirm a pending upgrade as a second authorized signer (multi-sig step).
+    ///
+    /// A separate `upgrader` address (distinct from the admin that called
+    /// `schedule_upgrade`) must call this function to attest that the pending
+    /// WASM hash has been independently reviewed and approved. `execute_upgrade`
+    /// will be rejected with `UpgradeNotConfirmed` until this confirmation is on
+    /// record.
+    ///
+    /// The upgrader is any address passed in and authorized via `require_auth`.
+    /// In production this should be a hardware-wallet address or a second
+    /// signing key that is kept separate from the admin key so that a single
+    /// compromised key cannot alone deploy arbitrary code.
+    ///
+    /// # Arguments
+    /// * `upgrader` — the address providing the second-factor confirmation.
+    ///
+    /// # Errors
+    /// * `UpgradeNotScheduled` — no upgrade is pending (nothing to confirm).
+    pub fn confirm_upgrade(env: Env, upgrader: Address) -> Result<(), Error> {
+        extend_instance_ttl(&env);
+        upgrader.require_auth();
+
+        if !env.storage().instance().has(&DataKey::UpgradeScheduledAt) {
+            return Err(Error::UpgradeNotScheduled);
+        }
+
+        env.storage()
+            .instance()
+            .set(&UpgradeKey::ConfirmedBy, &upgrader);
+
+        env.events().publish(
+            (Symbol::new(&env, "upgrade"), symbol_short!("confirm")),
+            upgrader,
+        );
+
+        Ok(())
+    }
+
     /// Execute the scheduled WASM upgrade after the 7-day review period.
     ///
     /// Admin-gated. Calls the host `upgrade` function with the stored WASM
@@ -6142,9 +6183,14 @@ impl EscrowContract {
     /// state-mutating transactions run against the old schema while the
     /// replacement WASM is being applied.
     ///
+    /// A second authorized address must have called `confirm_upgrade` after
+    /// `schedule_upgrade` was issued. This two-step requirement means that a
+    /// single compromised admin key cannot unilaterally deploy arbitrary code.
+    ///
     /// # Errors
     /// * `UpgradeNotScheduled`            — no upgrade is pending.
     /// * `UpgradeReviewPeriodNotElapsed`  — 7-day review period has not passed.
+    /// * `UpgradeNotConfirmed`            — `confirm_upgrade` has not been called.
     /// * `InvalidPauseState`              — contract is not paused.
     /// * `Unauthorized`                   — caller is not the admin.
     pub fn execute_upgrade(env: Env) -> Result<(), Error> {
@@ -6177,6 +6223,12 @@ impl EscrowContract {
             return Err(Error::UpgradeReviewPeriodNotElapsed);
         }
 
+        // Enforce multi-sig confirmation: a second authorized address must have
+        // called confirm_upgrade before execution is permitted.
+        if !env.storage().instance().has(&UpgradeKey::ConfirmedBy) {
+            return Err(Error::UpgradeNotConfirmed);
+        }
+
         let new_wasm_hash: BytesN<32> = env
             .storage()
             .instance()
@@ -6191,6 +6243,8 @@ impl EscrowContract {
         env.storage()
             .instance()
             .remove(&DataKey::PendingUpgradeHash);
+        // Clear the confirmation so it cannot carry over to a future upgrade.
+        env.storage().instance().remove(&UpgradeKey::ConfirmedBy);
 
         env.deployer()
             .update_current_contract_wasm(new_wasm_hash.clone());

contracts/escrow/src/tests/upgrade.rs

@@ -4,7 +4,8 @@
 /// - `get_version` — returns the correct initial version.
 /// - `schedule_upgrade` — admin-gated, emits event, stores keys.
 /// - `cancel_upgrade`   — removes pending-upgrade keys.
-/// - `execute_upgrade`  — enforces paused + review-period constraints.
+/// - `confirm_upgrade`  — records a second-signer confirmation.
+/// - `execute_upgrade`  — enforces paused + review-period + confirmation constraints.
 /// - `migrate_state`    — version guard, incremental migrations, idempotency.
 /// - `validate_state`   — detects a healthy vs. corrupt instance store.
 use super::*;
@@ -128,6 +129,104 @@ fn test_cancel_upgrade_when_none_pending_is_rejected() {
     );
 }
 
+// ── confirm_upgrade ───────────────────────────────────────────────────────────
+
+#[test]
+fn test_confirm_upgrade_succeeds_with_any_authorized_address() {
+    let (env, contract_id, _oracle, _p1, _p2, _token, _admin) = setup();
+    let client = EscrowContractClient::new(&env, &contract_id);
+
+    client.schedule_upgrade(&dummy_wasm_hash(&env));
+
+    let upgrader = Address::generate(&env);
+    // No panic → confirmation accepted.
+    client.confirm_upgrade(&upgrader);
+}
+
+#[test]
+fn test_confirm_upgrade_without_scheduled_upgrade_is_rejected() {
+    let (env, contract_id, _oracle, _p1, _p2, _token, _admin) = setup();
+    let client = EscrowContractClient::new(&env, &contract_id);
+
+    let upgrader = Address::generate(&env);
+    let result = client.try_confirm_upgrade(&upgrader);
+    assert!(
+        matches!(result, Err(Ok(Error::UpgradeNotScheduled))),
+        "confirm_upgrade with no pending upgrade must return UpgradeNotScheduled"
+    );
+}
+
+#[test]
+fn test_confirm_upgrade_emits_event() {
+    let (env, contract_id, _oracle, _p1, _p2, _token, _admin) = setup();
+    let client = EscrowContractClient::new(&env, &contract_id);
+
+    client.schedule_upgrade(&dummy_wasm_hash(&env));
+
+    let upgrader = Address::generate(&env);
+    client.confirm_upgrade(&upgrader);
+
+    let events = env.events().all();
+    let found = events.iter().any(|e| {
+        let topics: soroban_sdk::Vec<soroban_sdk::Val> = e.1;
+        if topics.len() < 2 {
+            return false;
+        }
+        let t0 = topics.get(0).unwrap();
+        let t1 = topics.get(1).unwrap();
+        let s0 = Symbol::try_from_val(&env, &t0).ok();
+        let s1 = Symbol::try_from_val(&env, &t1).ok();
+        s0 == Some(Symbol::new(&env, "upgrade")) && s1 == Some(symbol_short!("confirm"))
+    });
+    assert!(found, "confirm_upgrade must emit (upgrade, confirm) event");
+}
+
+#[test]
+fn test_execute_upgrade_rejected_without_confirmation() {
+    let (env, contract_id, _oracle, _p1, _p2, _token, admin) = setup();
+    let client = EscrowContractClient::new(&env, &contract_id);
+
+    client.schedule_upgrade(&dummy_wasm_hash(&env));
+    client.pause(&admin);
+
+    // Advance past the review period but do NOT call confirm_upgrade.
+    env.ledger().with_mut(|l| {
+        l.sequence_number += crate::UPGRADE_REVIEW_PERIOD_LEDGERS + 1;
+    });
+
+    let result = client.try_execute_upgrade();
+    assert!(
+        matches!(result, Err(Ok(Error::UpgradeNotConfirmed))),
+        "execute_upgrade without prior confirm_upgrade must return UpgradeNotConfirmed"
+    );
+}
+
+#[test]
+fn test_cancel_upgrade_clears_confirmation() {
+    let (env, contract_id, _oracle, _p1, _p2, _token, admin) = setup();
+    let client = EscrowContractClient::new(&env, &contract_id);
+
+    // Schedule, confirm, then cancel.
+    client.schedule_upgrade(&dummy_wasm_hash(&env));
+    let upgrader = Address::generate(&env);
+    client.confirm_upgrade(&upgrader);
+    client.cancel_upgrade();
+
+    // Re-schedule a new upgrade — the old confirmation must not carry over.
+    client.schedule_upgrade(&dummy_wasm_hash(&env));
+    client.pause(&admin);
+
+    env.ledger().with_mut(|l| {
+        l.sequence_number += crate::UPGRADE_REVIEW_PERIOD_LEDGERS + 1;
+    });
+
+    let result = client.try_execute_upgrade();
+    assert!(
+        matches!(result, Err(Ok(Error::UpgradeNotConfirmed))),
+        "confirmation from a cancelled upgrade must not carry over to the next upgrade"
+    );
+}
+
 // ── execute_upgrade ───────────────────────────────────────────────────────────
 
 #[test]
@@ -174,6 +273,10 @@ fn test_execute_upgrade_succeeds_after_review_period() {
     client.schedule_upgrade(&dummy_wasm_hash(&env));
     client.pause(&admin);
 
+    // Provide the required multi-sig confirmation before execution.
+    let upgrader = Address::generate(&env);
+    client.confirm_upgrade(&upgrader);
+
     // Advance past the review period.
     env.ledger().with_mut(|l| {
         l.sequence_number += crate::UPGRADE_REVIEW_PERIOD_LEDGERS + 1;
@@ -188,6 +291,10 @@ fn test_execute_upgrade_succeeds_after_review_period() {
         !matches!(result, Err(Ok(Error::UpgradeReviewPeriodNotElapsed))),
         "execute_upgrade after the review period must not return UpgradeReviewPeriodNotElapsed"
     );
+    assert!(
+        !matches!(result, Err(Ok(Error::UpgradeNotConfirmed))),
+        "execute_upgrade after confirm_upgrade must not return UpgradeNotConfirmed"
+    );
 }
 
 #[test]

contracts/escrow/src/types.rs

@@ -462,6 +462,24 @@ pub enum TokenSymbolCacheKey {
     TokenSymbol(Address),
 }
 
+/// Storage keys for the multi-sig upgrade confirmation step.
+///
+/// Kept as a separate `#[contracttype]` enum rather than added to `DataKey`
+/// for the same reason as other overflow keys: `DataKey` is at its 50-variant
+/// XDR cap.
+///
+/// `confirm_upgrade` writes `ConfirmedBy(upgrader_address)` once the second
+/// authorized signer approves the pending WASM hash. `execute_upgrade` reads
+/// this key to enforce the two-step requirement, then removes it. Both
+/// `cancel_upgrade` and `execute_upgrade` clear the key so the guard cannot
+/// carry over to a subsequent upgrade cycle.
+#[contracttype]
+pub enum UpgradeKey {
+    /// The address that confirmed the pending upgrade via `confirm_upgrade`.
+    /// Absence of this key means no confirmation has been recorded.
+    ConfirmedBy,
+}
+
 /// An oracle-verified ELO / platform rating for a player.
 ///
 /// Registered on-chain by the oracle via `register_player_rating` so that

docs/error-codes.md

@@ -5,7 +5,7 @@
 This document is the exhaustive, user-facing reference for every error a
 caller can receive from the two on-chain Soroban contracts in this repo:
 
-- [`EscrowContract`](../contracts/escrow/src/lib.rs) — [`contracts/escrow/src/errors.rs`](../contracts/escrow/src/errors.rs) (52 variants)
+- [`EscrowContract`](../contracts/escrow/src/lib.rs) — [`contracts/escrow/src/errors.rs`](../contracts/escrow/src/errors.rs) (53 variants)
 - [`OracleContract`](../contracts/oracle/src/lib.rs) — [`contracts/oracle/src/errors.rs`](../contracts/oracle/src/errors.rs) (27 variants)
 
 Every variant defined in those two files is documented below. If you add,
@@ -149,6 +149,7 @@ These error codes support advanced features including dispute resolution, stakin
 | 54 | `NotAnOracle` | v0.1.0 | [`submit_result_with_oracle_record`](../contracts/escrow/src/lib.rs), oracle-gated functions | The caller is not a registered oracle address. Distinct from `Unauthorized` (#4) — `#54` is returned specifically when the caller has valid auth credentials but is not on the oracle registry. | Verify the signing key is a registered oracle address via `get_oracle_address`. If the oracle address was rotated, admin must call `update_oracle` with the current key. | A new oracle instance with a fresh key pair submits a result without first having the admin register it via `update_oracle` → `#54`. |
 | 55 | `DepositInProgress` | v0.1.0 | [`deposit`](../contracts/escrow/src/lib.rs) | A deposit for this match is already in progress — the reentrancy guard tripped. A concurrent or re-entrant deposit call attempted to deposit while another deposit for the same match was still executing. | Do not submit multiple concurrent deposit calls for the same match ID. Retry after the first call completes. | Two parallel oracle service instances both call `deposit` for match 42 at the same time → one succeeds, the other gets `#55`. |
 | 56 | `DisputeFallbackNotElapsed` | v0.1.0 | Dispute resolution functions | `resolve_dispute_no_quorum` (or equivalent fallback resolution) was called before the dispute's voting deadline plus the configured grace period has elapsed. The no-quorum fallback path requires both the voting window and the grace period to have passed before it can fire. | Check the dispute's deadline and the configured grace period via `get_dispute`. Wait for both to elapse before retrying the fallback resolution call. | Calling the no-quorum fallback resolver one ledger before the grace period ends → `#56`. |
+| 58 | `UpgradeNotConfirmed` | v0.2.0 | [`execute_upgrade`](../contracts/escrow/src/lib.rs) | `execute_upgrade` was called without a prior `confirm_upgrade` call from a second authorized address. The multi-sig confirmation step is mandatory — a single admin key cannot unilaterally deploy new WASM. | Call `confirm_upgrade(upgrader)` with a separate authorized key after `schedule_upgrade` and before `execute_upgrade`. | Admin schedules an upgrade and immediately tries to execute it without having the upgrader key sign off → `#58`. |
 
 
 ---
@@ -252,6 +253,10 @@ All error codes documented in this file were introduced in v0.1.0. The `Since` c
 
 ### v0.2.0 (unreleased)
 
+**Escrow contract**
+
+- Added `#58 UpgradeNotConfirmed` (since v0.2.0) — returned by `execute_upgrade` when `confirm_upgrade` has not been called by a second authorized address after `schedule_upgrade`. Enforces a two-step multi-sig requirement for contract upgrades so that a single compromised admin key cannot unilaterally deploy arbitrary WASM.
+
 **Oracle contract**
 
 - Added `#13 InvalidPauseState` (since v0.2.0) — returned by `pause` when the contract is already paused, or by `unpause` when the contract is not currently paused. Fills the previously unassigned gap at discriminant 13. Prevents duplicate state transitions and spurious events, bringing the oracle in line with the escrow contract's `InvalidPauseState` behaviour.