refactor(upgrade): enforce auth and state preconditions before WASM upgrade (#352)

contracts/milestone-escrow/src/lib.rs

@@ -3143,18 +3143,21 @@ impl MilestoneEscrow {
         Ok(())
     }
 
+    /// Upgrade the contract's WASM to `new_wasm_hash`.
+    ///
+    /// # Business rules
+    /// Caller authorization and pause/lock preconditions are checked before
+    /// any storage mutation or WASM upgrade, so a rejected call leaves the
+    /// contract's storage and installed code untouched.
+    ///
+    /// # Errors
+    /// * `NotInitialized` – Admin key has never been stored.
+    /// * `Unauthorized`   – `admin` is not the stored admin.
+    /// * `Paused`         – The contract is currently emergency-paused.
+    /// * `EscrowLocked`   – A cancel is in progress and holds the cancel lock.
     pub fn upgrade(env: Env, admin: Address, new_wasm_hash: BytesN<32>) -> Result<(), Error> {
-        admin.require_auth();
-
-        let stored_admin: Address = env
-            .storage()
-            .persistent()
-            .get(&DataKey::Admin)
-            .ok_or(Error::NotInitialized)?;
-
-        if admin != stored_admin {
-            return Err(Error::Unauthorized);
-        }
+        Self::require_admin(&env, &admin)?;
+        Self::ensure_not_paused(&env)?;
 
         env.deployer().update_current_contract_wasm(new_wasm_hash);
 

contracts/milestone-escrow/src/test.rs

@@ -5619,6 +5619,49 @@ fn test_upgrade_admin_auth_check_passes() {
     assert_ne!(result, Err(Ok(Error::Unauthorized)));
 }
 
+/// Issue #352: an unauthorized caller must be rejected by the guard clause
+/// at the very top of `upgrade`, before the version counter (the only
+/// storage key `upgrade` mutates on success) is touched.
+#[test]
+fn test_upgrade_unauthorized_caller_mutates_no_storage() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (_, _, _, _, _, _, client) = setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+
+    let version_before = client.version();
+
+    let bad_actor = Address::generate(&env);
+    let fake_hash = soroban_sdk::BytesN::from_array(&env, &[0u8; 32]);
+    let result = client.try_upgrade(&bad_actor, &fake_hash);
+
+    assert_eq!(result, Err(Ok(Error::Unauthorized)));
+    assert_eq!(client.version(), version_before);
+}
+
+/// Issue #352: `upgrade` must be blocked while the contract is
+/// emergency-paused, failing with the specific `Paused` error rather than
+/// proceeding to the WASM upgrade / version bump.
+#[test]
+fn test_upgrade_while_paused_fails_with_typed_error() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (_, _, _, admin_addr, _, _, client) = setup_funded_escrow(&env, vec![&env, 1_000_i128]);
+
+    client.emergency_pause(&admin_addr);
+    assert!(client.is_emergency_paused());
+
+    let version_before = client.version();
+
+    let fake_hash = soroban_sdk::BytesN::from_array(&env, &[0u8; 32]);
+    let result = client.try_upgrade(&admin_addr, &fake_hash);
+
+    assert_eq!(result, Err(Ok(Error::Paused)));
+    assert_eq!(client.version(), version_before);
+    assert!(client.is_emergency_paused());
+}
+
 // ============================================================================
 // add_whitelisted_token ΓÇö comprehensive boundary / negative / edge-case tests
 // ============================================================================