fix: add storage non-mutation coverage to multisig_approval_init guar…

contracts/milestone-escrow/src/admin_override_cancel_tests.rs

@@ -16,11 +16,12 @@
 //!   - Error::InvalidAmount is returned (not a panic) for edge-case amounts.
 //!   - Terminal milestones are correctly skipped in every scenario.
 
+#![cfg(test)]
+
 use super::*;
 use crate::test::setup_funded_escrow;
 use crate::{DataKey, Error, MilestoneEscrowClient, MilestoneStatus};
-use soroban_sdk::testutils::Address as _;
-use soroban_sdk::{token, vec, Address, Env};
+use soroban_sdk::{testutils::Address as _, token, vec, Address, Env};
 
 // ────────────────────────────────────────────────────────────────────────────
 // Issue #383: admin_override_cancel_release storage footprint
@@ -120,8 +121,7 @@ fn test_cancel_release_requires_cancel_lock() {
     let env = Env::default();
     env.mock_all_auths();
 
-    let (_, _, _, admin_addr, _, _, client) =
-        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    let (_, _, _, admin_addr, _, _, client) = setup_funded_escrow(&env, vec![&env, 1_000_i128]);
 
     let result = client.try_admin_override_cancel_release(&admin_addr);
     assert_eq!(result, Err(Ok(Error::InvalidStatus)));
@@ -133,8 +133,7 @@ fn test_cancel_release_unauthorized_caller_rejected() {
     let env = Env::default();
     env.mock_all_auths();
 
-    let (client_addr, _, _, _, _, _, client) =
-        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    let (client_addr, _, _, _, _, _, client) = setup_funded_escrow(&env, vec![&env, 1_000_i128]);
     client.cancel_escrow(&client_addr);
 
     let attacker = Address::generate(&env);
@@ -227,8 +226,7 @@ fn test_cancel_refund_requires_cancel_lock() {
     let env = Env::default();
     env.mock_all_auths();
 
-    let (_, _, _, admin_addr, _, _, client) =
-        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    let (_, _, _, admin_addr, _, _, client) = setup_funded_escrow(&env, vec![&env, 1_000_i128]);
 
     let result = client.try_admin_override_cancel_refund(&admin_addr);
     assert_eq!(result, Err(Ok(Error::InvalidStatus)));
@@ -332,8 +330,7 @@ fn test_cancel_refund_multiple_milestones_sum_correctly() {
     env.mock_all_auths();
 
     let amounts = vec![&env, 100_i128, 200_i128, 300_i128, 400_i128];
-    let (client_addr, _, _, admin_addr, token_id, _, client) =
-        setup_funded_escrow(&env, amounts);
+    let (client_addr, _, _, admin_addr, token_id, _, client) = setup_funded_escrow(&env, amounts);
 
     client.cancel_escrow(&client_addr);
 
@@ -365,4 +362,3 @@ fn test_cancel_refund_minimum_valid_amount() {
 
     assert_eq!(token.balance(&client_addr), client_before + 1);
 }
-

contracts/milestone-escrow/src/lib.rs

@@ -216,6 +216,11 @@ pub enum DataKey {
     /// cleared by `multisig_admin_override_release` or
     /// `multisig_admin_override_refund`.
     MultisigLocked,
+    /// Temporary key: cumulative extension seconds applied to a Delivered
+    /// milestone. Written by `extend_milestone_deadline`, read by
+    /// `claim_auto_release` and `time_until_auto_release`. Uses temporary
+    /// storage because the extension is deadline-scoped workflow state whose
+    /// ledger footprint cost should not persist beyond the auto-release window.
     MilestoneTimeExtension(u32),
     /// Instance key for the cancel_escrow lock.
     CancelLock,
@@ -1334,7 +1339,7 @@ impl MilestoneEscrow {
 
     fn load_time_extension(env: &Env, index: u32) -> u64 {
         env.storage()
-            .persistent()
+            .temporary()
             .get(&DataKey::MilestoneTimeExtension(index))
             .unwrap_or(0)
     }
@@ -2037,8 +2042,7 @@ impl MilestoneEscrow {
         let new_extension = current_extension
             .checked_add(extra_seconds)
             .ok_or(Error::InvalidExtension)?;
-
-        env.storage().persistent().set(
+        env.storage().temporary().set(
             &DataKey::MilestoneTimeExtension(milestone_index),
             &new_extension,
         );
@@ -2995,6 +2999,26 @@ impl MilestoneEscrow {
             return Err(Error::InvalidAddress);
         }
 
+        // Reject if the contract is emergency-paused.
+        let emergency_paused = env
+            .storage()
+            .instance()
+            .get::<_, bool>(&DataKey::Ep)
+            .unwrap_or(false);
+        if emergency_paused {
+            return Err(Error::Paused);
+        }
+
+        // Reject a duplicate cancel — CancelLock already active.
+        let already_locked = env
+            .storage()
+            .instance()
+            .get::<_, bool>(&DataKey::CancelLock)
+            .unwrap_or(false);
+        if already_locked {
+            return Err(Error::EscrowLocked);
+        }
+
         caller.require_auth();
         let meta = Self::load_job_meta(&env)?;
 
@@ -5519,6 +5543,17 @@ impl MilestoneEscrow {
             return Err(Error::InvalidStatus);
         }
 
+        // Reject pathological operands before any arithmetic (issue #395).
+        // `amount` / `released_amount` are signed i128 values read from
+        // storage; guarding them here, alongside the checked_sub and
+        // `remaining <= 0` guards below, guarantees no input — including
+        // `i128::MAX` / `i128::MIN` — can cause a wrap or an unhandled panic.
+        if milestone.amount < 0 || milestone.released_amount < 0 {
+            return Err(Error::InvalidAmount);
+        }
+        if milestone.released_amount > milestone.amount {
+            return Err(Error::InvalidAmount);
+        }
         let remaining = milestone
             .amount
             .checked_sub(milestone.released_amount)
@@ -5586,7 +5621,11 @@ impl MilestoneEscrow {
     /// 4. `milestone_index` must be in range (`InvalidMilestone`).
     /// 5. Milestone must not already be `Released` or `Refunded`
     ///    (`InvalidStatus`).
-    /// 6. Remaining balance must be > 0 (`InvalidAmount`).
+    /// 6. Amount arithmetic is fully checked — pathological `i128` operands
+    ///    (negative `amount` / `released_amount`, or `released_amount` beyond
+    ///    `amount`, including `i128::MAX` / `i128::MIN`) return `InvalidAmount`
+    ///    without panic or wrap.
+    /// 7. Remaining balance must be > 0 (`InvalidAmount`).
     ///
     /// # Parameters
     /// * `admin`           – Must match `DataKey::Admin`.
@@ -5633,6 +5672,17 @@ impl MilestoneEscrow {
             return Err(Error::InvalidStatus);
         }
 
+        // Reject pathological operands before any arithmetic (issue #395).
+        // `amount` / `released_amount` are signed i128 values read from
+        // storage; guarding them here, alongside the checked_sub and
+        // `remaining <= 0` guards below, guarantees no input — including
+        // `i128::MAX` / `i128::MIN` — can cause a wrap or an unhandled panic.
+        if milestone.amount < 0 || milestone.released_amount < 0 {
+            return Err(Error::InvalidAmount);
+        }
+        if milestone.released_amount > milestone.amount {
+            return Err(Error::InvalidAmount);
+        }
         let remaining = milestone
             .amount
             .checked_sub(milestone.released_amount)

contracts/milestone-escrow/src/multisig_admin_override_refund_tests.rs

@@ -140,3 +140,135 @@ fn locked_admin_override_refunds_client_and_clears_lock() {
     assert_eq!(after.client_balance, before.client_balance + 1_000);
     assert_eq!(after.contract_balance, before.contract_balance - 1_000);
 }
+
+// ── arithmetic hardening (issue #395) ────────────────────────────────────────
+
+/// Overwrite the persistent `Milestone(index)` entry directly with an
+/// adversarial value. `amount` / `released_amount` are signed i128 values that
+/// the normal flow would never produce, so they are injected straight into
+/// storage to prove the checked arithmetic returns a typed error instead of
+/// panicking or wrapping.
+fn set_milestone_raw(env: &Env, contract_id: &Address, index: u32, milestone: &Milestone) {
+    env.as_contract(contract_id, || {
+        env.storage()
+            .persistent()
+            .set(&DataKey::Milestone(index), milestone);
+    });
+}
+
+fn unlocked_milestone(amount: i128, released_amount: i128) -> Milestone {
+    Milestone {
+        amount,
+        released_amount,
+        status: MilestoneStatus::Pending,
+        delivered_at: 0,
+    }
+}
+
+/// A milestone whose `amount` is `i128::MIN` must be rejected with
+/// `Error::InvalidAmount` before any arithmetic runs — it must not panic or
+/// wrap.
+#[test]
+fn refund_negative_amount_returns_invalid_amount_without_panic() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (client_addr, _, _, admin_addr, token_id, contract_id, client) =
+        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    client.multisig_lock(&admin_addr);
+
+    let milestone = unlocked_milestone(i128::MIN, 0);
+    set_milestone_raw(&env, &contract_id, 0, &milestone);
+
+    let result = client.try_multisig_admin_override_refund(&admin_addr, &0u32);
+    assert_eq!(result, Err(Ok(Error::InvalidAmount)));
+
+    assert!(client.is_multisig_locked());
+    assert_eq!(refund_event_count(&env), 0);
+}
+
+/// A negative `released_amount` (e.g. `i128::MIN`) is a pathological operand:
+/// subtracting it could overflow `i128::MAX`. It must be rejected with
+/// `Error::InvalidAmount` rather than panic.
+#[test]
+fn refund_negative_released_amount_returns_invalid_amount_without_panic() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (client_addr, _, _, admin_addr, token_id, contract_id, client) =
+        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    client.multisig_lock(&admin_addr);
+
+    let milestone = unlocked_milestone(i128::MAX, i128::MIN);
+    set_milestone_raw(&env, &contract_id, 0, &milestone);
+
+    let result = client.try_multisig_admin_override_refund(&admin_addr, &0u32);
+    assert_eq!(result, Err(Ok(Error::InvalidAmount)));
+
+    assert!(client.is_multisig_locked());
+    assert_eq!(refund_event_count(&env), 0);
+}
+
+/// `amount == i128::MAX` with a positive `released_amount` would previously
+/// risk wrapping; the checked_sub + `remaining <= 0` guards must yield
+/// `Error::InvalidAmount` for any over-full (or equal) released_amount.
+#[test]
+fn refund_released_exceeds_amount_returns_invalid_amount_without_panic() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (client_addr, _, _, admin_addr, token_id, contract_id, client) =
+        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    client.multisig_lock(&admin_addr);
+
+    let milestone = unlocked_milestone(i128::MAX, i128::MAX);
+    set_milestone_raw(&env, &contract_id, 0, &milestone);
+
+    // released_amount == amount → remaining == 0 → InvalidAmount.
+    let result = client.try_multisig_admin_override_refund(&admin_addr, &0u32);
+    assert_eq!(result, Err(Ok(Error::InvalidAmount)));
+    assert!(client.is_multisig_locked());
+
+    // released_amount > amount → remaining < 0 → InvalidAmount (no wrap).
+    let milestone2 = unlocked_milestone(100, 200);
+    set_milestone_raw(&env, &contract_id, 0, &milestone2);
+    let result = client.try_multisig_admin_override_refund(&admin_addr, &0u32);
+    assert_eq!(result, Err(Ok(Error::InvalidAmount)));
+    assert!(client.is_multisig_locked());
+
+    assert_eq!(refund_event_count(&env), 0);
+}
+
+/// Valid amounts must produce results identical to before the hardening.
+/// A partially-released milestone refunds exactly `amount - released_amount`.
+#[test]
+fn refund_valid_amount_equals_amount_minus_released_amount() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (client_addr, _, _, admin_addr, token_id, contract_id, client) =
+        setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+    client.multisig_lock(&admin_addr);
+
+    // Inject an already partially-released milestone (released 400 of 1000).
+    // Remaining refund == 1000 - 400 == 600.
+    let milestone = unlocked_milestone(1_000, 400);
+    set_milestone_raw(&env, &contract_id, 0, &milestone);
+
+    let token = token::Client::new(&env, &token_id);
+    let client_before = token.balance(&client_addr);
+
+    client.multisig_admin_override_refund(&admin_addr, &0u32);
+
+    // Read the event tally first: every later `client.*` / `token.*` call is
+    // itself a contract invocation, and the test env's event buffer reflects
+    // the most recent one.
+    assert_eq!(refund_event_count(&env), 1);
+
+    assert!(!client.is_multisig_locked());
+    let job = client.get_job();
+    let ms = job.milestones.get(0).unwrap();
+    assert_eq!(ms.status, MilestoneStatus::Refunded);
+    assert_eq!(ms.released_amount, 1_000);
+    assert_eq!(token.balance(&client_addr), client_before + 600);
+}