Security: Execution-time invariant re-validation, approval cleanup, and token flow documentation

contracts/accord/src/lib.rs

@@ -1784,14 +1784,27 @@ impl AccordContract {
                 if !(MIN_OWNER_WEIGHT..=MAX_OWNER_WEIGHT).contains(weight) {
                     return Err(ContractError::InvalidWeight);
                 }
+
+                let owners = read_owners_map(&env)?;
+                let prev_count = owners.len();
+
+                // Re-check at execute time: adding an owner must not push the
+                // owner count past MAX_OWNERS (20). The creation-time check
+                // in create_add_owner_proposal only validates against the
+                // owner count at proposal creation — a concurrent AddOwner
+                // proposal executed beforehand could have already filled the
+                // last slot.
+                if prev_count >= MAX_OWNERS {
+                    return Err(ContractError::InvalidOwners);
+                }
+
                 let current_total = read_total_weight(&env);
                 let new_total = checked_weight_add(current_total, *weight)?;
                 if !owner_weight_within_cap(&env, *weight, new_total) {
                     return Err(ContractError::SingleOwnerWeightCapExceeded);
                 }
 
-                let mut owners = read_owners_map(&env)?;
-                let prev_count = owners.len();
+                let mut owners = owners;
                 owners.set(new_owner.clone(), *weight);
                 let key = owners_key();
                 env.storage().persistent().set(&key, &owners);
@@ -1840,7 +1853,6 @@ impl AccordContract {
                         }
                     }
                 }
-
                 owners.remove(owner_to_remove.clone());
                 let key = owners_key();
                 env.storage().persistent().set(&key, &owners);
@@ -1851,6 +1863,32 @@ impl AccordContract {
                     resulting_total_weight,
                 );
 
+                // Remove the removed owner's approval weight from all
+                // Pending and Ready proposals they previously approved.
+                // Without this, a removed owner's prior votes would
+                // continue counting toward the threshold even after
+                // they are no longer an owner — undermining the M-of-N
+                // model. Terminal proposals (Executed, Expired, Revoked)
+                // are left untouched since their outcome is final.
+                let next_id: u64 = env.storage()
+                    .instance()
+                    .get(&next_id_key())
+                    .unwrap_or(1_u64);
+                for pid in 1_u64..next_id {
+                    if let Ok(mut p) = read_proposal(&env, pid) {
+                        let derived = derive_status(&env, &p);
+                        if matches!(derived, ProposalStatus::Pending | ProposalStatus::Ready)
+                            && read_approval(&env, pid, owner_to_remove)
+                        {
+                            write_approval(&env, pid, owner_to_remove, false);
+                            p.approvals = checked_weight_sub(p.approvals, weight)?;
+                            p.approval_weight = checked_weight_sub(p.approval_weight, weight)?;
+                            p.status = derive_status(&env, &p);
+                            write_proposal(&env, &p);
+                        }
+                    }
+                }
+
                 env.events().publish(
                     (symbol_short!("r_own"),),
                     RemoveOwnerExecutedEvent {

contracts/accord/src/test.rs

@@ -3295,6 +3295,69 @@ fn create_add_owner_proposal_rejects_at_max_owners() {
     );
 }
 
+#[test]
+fn add_owner_execute_rejects_when_cap_reached_by_prior_add() {
+    let env = Env::default();
+    env.mock_all_auths();
+    set_timestamp(&env, NOW);
+
+    let contract_id = env.register(AccordContract, ());
+    let client = AccordContractClient::new(&env, &contract_id);
+
+    // Initialize with MAX_OWNERS - 1 (19) owners.
+    let mut owners = Vec::new(&env);
+    let first_owner = Address::generate(&env);
+    owners.push_back(first_owner.clone());
+    for _ in 1..MAX_OWNERS - 1 {
+        owners.push_back(Address::generate(&env));
+    }
+    let mut weights = Vec::new(&env);
+    for _ in 0..owners.len() {
+        weights.push_back(1);
+    }
+    client.initialize(&owners, &weights, &1, &0);
+    assert_eq!(client.get_owners().len(), MAX_OWNERS - 1);
+
+    // Create two AddOwner proposals for two different new addresses.
+    let new_owner_a = Address::generate(&env);
+    let new_owner_b = Address::generate(&env);
+
+    let p1 = client.create_add_owner_proposal(
+        &first_owner,
+        &new_owner_a,
+        &MIN_OWNER_WEIGHT,
+        &str(&env, "Add owner A"),
+        &DEADLINE,
+    );
+    let p2 = client.create_add_owner_proposal(
+        &first_owner,
+        &new_owner_b,
+        &MIN_OWNER_WEIGHT,
+        &str(&env, "Add owner B"),
+        &DEADLINE,
+    );
+
+    // Approve and execute p1: owner count goes from 19 to 20.
+    client.approve(&first_owner, &p1);
+    client.execute(&first_owner, &p1);
+    assert_eq!(client.get_proposal(&p1).status, ProposalStatus::Executed);
+    assert_eq!(client.get_owners().len(), MAX_OWNERS);
+
+    // Approve and try to execute p2: owner count is already at cap.
+    client.approve(&first_owner, &p2);
+    let res = client.try_execute(&first_owner, &p2);
+    assert_eq!(res, Err(Ok(ContractError::InvalidOwners)));
+
+    // p2 is not marked Executed and remains in Ready state.
+    assert_eq!(
+        client.get_proposal(&p2).status,
+        ProposalStatus::Ready,
+        "proposal rejected at execute time must not be marked Executed"
+    );
+    // Owner count stays at 20.
+    assert_eq!(client.get_owners().len(), MAX_OWNERS);
+}
+
 // ─── Spending Limits (issue #41) ───────────────────────────────────────────────
 
 #[test]
@@ -5251,6 +5314,119 @@ fn remove_owner_execute_emits_remove_owner_event() {
     assert_eq!(event.owner_count, 2);
 }
 
+#[test]
+fn remove_owner_clears_approvals_from_pending_proposals() {
+    let (env, client, owner_a, owner_b, owner_c, _, token_client) = setup(2);
+    let recipient = Address::generate(&env);
+
+    // Owner A creates a transfer proposal.
+    let prop_id = client.create_proposal(
+        &owner_a,
+        &t(&env, &recipient, 1_000_000, &token_client.address),
+        &str(&env, "Transfer"),
+        &DEADLINE,
+        &ProposalCategory::Transfer,
+    );
+
+    // Owner A and owner B approve → Ready (2 approvals = threshold 2).
+    client.approve(&owner_a, &prop_id);
+    client.approve(&owner_b, &prop_id);
+    assert_eq!(
+        client.get_proposal(&prop_id).status,
+        ProposalStatus::Ready
+    );
+
+    // Remove owner B via a separate RemoveOwner proposal.
+    let remove_id = client.create_remove_owner_proposal(
+        &owner_c,
+        &owner_b,
+        &str(&env, "Remove owner_b"),
+        &DEADLINE,
+    );
+    client.approve(&owner_a, &remove_id);
+    client.approve(&owner_c, &remove_id);
+    client.execute(&owner_c, &remove_id);
+
+    // Owner B's approval should have been stripped from the pending proposal.
+    let prop = client.get_proposal(&prop_id);
+    assert_eq!(
+        prop.approvals, 1,
+        "expected only owner_a's weight to remain"
+    );
+    assert_eq!(
+        prop.status,
+        ProposalStatus::Pending,
+        "proposal should fall back to Pending after approver is removed"
+    );
+    assert!(
+        !client.has_approved(&prop_id, &owner_b),
+        "has_approved should be false for removed owner"
+    );
+    assert!(
+        client.has_approved(&prop_id, &owner_a),
+        "has_approved should still be true for remaining owner"
+    );
+}
+
+#[test]
+fn remove_owner_does_not_affect_terminal_proposals() {
+    let (env, client, owner_a, owner_b, owner_c, _, token_client) = setup(2);
+    let recipient = Address::generate(&env);
+
+    // --- Executed proposal (contract already funded by setup) ---
+    let exec_id = client.create_proposal(
+        &owner_a,
+        &t(&env, &recipient, 1_000_000, &token_client.address),
+        &str(&env, "Executed proposal"),
+        &DEADLINE,
+        &ProposalCategory::Transfer,
+    );
+    client.approve(&owner_a, &exec_id);
+    client.approve(&owner_b, &exec_id);
+    client.execute(&owner_c, &exec_id);
+    assert_eq!(
+        client.get_proposal(&exec_id).status,
+        ProposalStatus::Executed
+    );
+
+    // --- Expired proposal ---
+    set_timestamp(&env, NOW); // back to NOW
+    let expire_soon = NOW + 100;
+    let expire_id = client.create_proposal(
+        &owner_c,
+        &t(&env, &recipient, 1_000_000, &token_client.address),
+        &str(&env, "Will expire"),
+        &expire_soon,
+        &ProposalCategory::Transfer,
+    );
+    client.approve(&owner_a, &expire_id);
+    set_timestamp(&env, expire_soon + 1);
+    // Status derives to Expired but is not persisted until a function call touches it.
+
+    // --- Remove owner A (who approved both terminal proposals) ---
+    set_timestamp(&env, expire_soon + 1);
+    let remove_id = client.create_remove_owner_proposal(
+        &owner_b,
+        &owner_a,
+        &str(&env, "Remove owner_a"),
+        &DEADLINE,
+    );
+    client.approve(&owner_b, &remove_id);
+    client.approve(&owner_c, &remove_id);
+    client.execute(&owner_c, &remove_id);
+
+    // Executed proposal is unaffected — status stays Executed.
+    assert_eq!(
+        client.get_proposal(&exec_id).status,
+        ProposalStatus::Executed
+    );
+    // Expired proposal is unaffected — status stays Expired.
+    assert_eq!(
+        client.get_proposal(&expire_id).status,
+        ProposalStatus::Expired
+    );
+}
+
 #[test]
 fn change_threshold_execute_emits_change_threshold_event() {
     let (env, client, owner_a, owner_b, owner_c, _, _) = setup(3);
@@ -6275,17 +6451,20 @@ fn test_quorum_matrix_remove_owner_and_remove_owner_blocked() {
     client.approve(&owners.get(0).unwrap(), &p2);
     client.approve(&owners.get(1).unwrap(), &p2);
 
-    // Execute first removal. Weight drops from 3 to 2.
+    // Execute first removal. Weight drops from 3 to 2. During p1's
+    // execution, owner1's approval weight is also stripped from p2,
+    // dropping p2's approvals from 2 to 1 (< threshold 2).
     client.execute(&owners.get(0).unwrap(), &p1);
 
-    // Execute second removal. Weight would drop to 1, which is below the
-    // current threshold, so execution must be rejected.
+    // p2 is no longer Ready — the approval cleanup during p1's execution
+    // strips owner1's approval weight from p2, dropping p2.approvals
+    // below quorum_weight. execute() fails with ThresholdNotMet before
+    // reaching the RemoveOwner dispatch arm.
     let res = client.try_execute(&owners.get(0).unwrap(), &p2);
-    assert_eq!(res, Err(Ok(ContractError::WouldBreakThreshold)));
-
-    assert_eq!(client.get_total_weight(), 2);
-    assert_eq!(client.get_proposal(&p2).status, ProposalStatus::Ready);
-    assert_eq!(client.get_total_proposals(), 2);
+    assert_eq!(res, Err(Ok(ContractError::ThresholdNotMet)));
+
+    // Invariant preserved: total_weight (2) >= threshold (2).
+    assert!(client.get_total_weight() >= client.get_threshold());
 }
 
 #[test]