refactor(multisig): harden auth and precondition guards in multisig_approve (#354)

contracts/milestone-escrow/src/lib.rs

@@ -3845,23 +3845,29 @@ impl MilestoneEscrow {
     /// Record an approval from one of the registered signers for the given
     /// proposal.  Idempotent — calling twice from the same signer has no
     /// effect and is not an error.
+    ///
+    /// # Checks (in order)
+    /// Authorization and source-state guards run **before** any job or
+    /// token ledger entry is read or written, so a rejected call cannot
+    /// mutate storage:
+    /// 1. `signer.require_auth()` — the transaction must be signed by
+    ///    `signer` (`Unauthorized` if missing).
+    /// 2. `signer` must be one of the registered multisig signers
+    ///    (`Unauthorized`).
+    /// 3. Contract token balance must be > 0 (`MultiSigEmptyBalance`).
+    ///
+    /// # Errors
+    /// * `NotInitialized`       – `multisig_approval_init` has not been called.
+    /// * `Unauthorized`         – `signer` did not sign, or is not a
+    ///   registered signer.
+    /// * `MultiSigEmptyBalance` – Contract token balance is ≤ 0.
     pub fn multisig_approve(
         env: Env,
         signer: Address,
         proposal_id: u32,
     ) -> Result<MultiSigApprovalState, Error> {
         signer.require_auth();
 
-        // Boundary guard: an approval collected against an empty escrow has
-        // no funds behind it, so block processing until the contract holds
-        // a positive token balance.
-        let meta = Self::load_job_meta(&env)?;
-        let token_client = token::Client::new(&env, &meta.token);
-        let contract_balance = token_client.balance(&env.current_contract_address());
-        if contract_balance <= 0 {
-            return Err(Error::MultiSigEmptyBalance);
-        }
-
         let signers: Vec<Address> = env
             .storage()
             .instance()
@@ -3874,12 +3880,23 @@ impl MilestoneEscrow {
             .get(&DataKey::MultiSigThreshold)
             .ok_or(Error::NotInitialized)?;
 
-        // Find the signer's index in the list (O(n) but n ≤ 32).
+        // Reject callers who are not registered signers before touching any
+        // job/token ledger entry (find the signer's index, O(n) but n ≤ 32).
         let signer_index = signers
             .iter()
             .position(|s| s == signer)
             .ok_or(Error::Unauthorized)?;
 
+        // Boundary guard: an approval collected against an empty escrow has
+        // no funds behind it, so block processing until the contract holds
+        // a positive token balance.
+        let meta = Self::load_job_meta(&env)?;
+        let token_client = token::Client::new(&env, &meta.token);
+        let contract_balance = token_client.balance(&env.current_contract_address());
+        if contract_balance <= 0 {
+            return Err(Error::MultiSigEmptyBalance);
+        }
+
         // Read the current bitmap from temporary storage (default: 0 = no approvals).
         let mut bitmap: u32 = env
             .storage()

contracts/milestone-escrow/src/test.rs

@@ -7787,6 +7787,50 @@ fn test_multisig_approve_emits_structured_event() {
     assert_eq!(matched, 1);
 }
 
+/// Auth guard: an unregistered signer is rejected with `Unauthorized` and
+/// leaves the proposal's approval bitmap untouched.
+#[test]
+fn test_multisig_approve_unauthorized_fails() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let (client, _admin, _signers) = setup_multisig(&env, 2);
+
+    let impostor = Address::generate(&env);
+    let result = client.try_multisig_approve(&impostor, &50u32);
+    assert_eq!(result, Err(Ok(Error::Unauthorized)));
+
+    let state = client.is_multisig_approved(&50u32);
+    assert!(!state.approved);
+    assert_eq!(state.approvals, 0);
+    assert_eq!(state.bitmap, 0);
+}
+
+/// Precondition guard: an illegal source state (zero contract balance) is
+/// rejected with `MultiSigEmptyBalance` and leaves the proposal's approval
+/// bitmap untouched.
+#[test]
+fn test_multisig_approve_illegal_source_state_fails() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    // Deliberately unfunded: setup_escrow_for_multisig initialises the job
+    // but never calls `fund`, so the contract token balance is zero.
+    let (client, admin) = setup_escrow_for_multisig(&env);
+    let signer1 = Address::generate(&env);
+    let signer2 = Address::generate(&env);
+    let signers = vec![&env, signer1.clone(), signer2.clone()];
+    client.multisig_approval_init(&admin, &signers, &2u32);
+
+    let result = client.try_multisig_approve(&signer1, &51u32);
+    assert_eq!(result, Err(Ok(Error::MultiSigEmptyBalance)));
+
+    let state = client.is_multisig_approved(&51u32);
+    assert!(!state.approved);
+    assert_eq!(state.approvals, 0);
+    assert_eq!(state.bitmap, 0);
+}
+
 /// Admin: unauthorised caller cannot initialise multisig.
 #[test]
 fn test_multisig_approval_init_unauthorized_fails() {