fix(parameters): secure multisig against stale approvals, self-degrading quorum, and admin bypass (#107)

context/progress-tracker.md

@@ -16,7 +16,20 @@ Update this file after every completed contract change, fix, or architectural de
 
 ## Completed
 
-### Security: Close Unauthenticated Admin-Claim Branch in Reputation `set_admin()`
+### Issue #107 — Parameters-Contract Multisig: Snapshot Sovereignty, Elevated Quorum & Two-Step Configuration
+- **Problem:** Three flaws combined to make the parameters-contract multisig (`contracts/parameters-contract/src/lib.rs`) theater, not security: (1) `configure_multisig()` needed only the single admin key, so one compromised key could silently swap the signer set and sidestep the multisig entirely; (2) proposals stored `approvals: Vec<Address>` and `execute()` only checked `approvals.len() >= threshold`, so approvals from signers *removed* since approval remained counted — revoked signers kept veto/exec power over in-flight proposals (a colluding signer + one stale approval could rewrite `interest bps`, `min_guarantee_percent`, `grace_period_seconds`, etc.); (3) `UpdateSigners` executed against the current config with no escalation guard, so a 2-of-3 committee needed only 2 old-threshold approvals to install a weaker 2-of-2 (or admit a colluder).
+- **Fix (`parameters-contract`):**
+  - **Signer-set snapshot on every proposal.** `Proposal` gained `snapshot: Vec<Address>` (the eligible signer set at proposal time) and `invalidated: bool`. `propose()` records the snapshot; `approve()` rejects signers not present in the snapshot (`ApproverNotEligible = 19`); `execute()` re-validates **every** stored approval against the snapshot **and** the current signer set before counting it (`require_eligible_approvers`), so removed signers' approvals are never counted and newly-added signers can't influence older proposals. `access::require_signer` still gates approval-time current membership.
+  - **Elevated quorum for signer-set changes.** `required_quorum()` returns `threshold + 1` (capped at the full signer count → unanimity for already-unanimous sets) for `UpdateSigners` actions; all other actions keep `threshold`. A committee can no longer cheapen its own gate with old-threshold approvals (`ThresholdNotMet = 17` when unmet).
+  - **Two-step `configure_multisig` → `confirm_multisig`.** `configure_multisig` (admin, one-time) now only records a *pending* config and emits a prominent `MSCONFPR` event carrying the full proposed signer set; the set is only activated by the explicit `confirm_multisig` step (admin, emits the existing `MSCONFIG`). A single admin call can no longer silently swap the signer set — the intent is broadcast on-chain before any change is applied, and the initial config remains one-time-locked.
+  - **In-flight signer-set proposals invalidated on signer-set change.** New `PROPIDS` instance index tracks proposals; when a signer-set change executes (`do_update_signers`), every *other* non-executed `UpdateSigners` proposal is voided (`invalidated = true`, `ProposalInvalidated = 18`, emitted via new `PROPIVLD` event). Non-signer proposals survive but any stale approver blocks execution via the membership re-validation. `test_snapshots/` are gitignored per repo convention.
+- **Files:** `contracts/parameters-contract/src/lib.rs`, `contracts/parameters-contract/src/storage.rs`, `contracts/parameters-contract/src/types.rs`, `contracts/parameters-contract/src/events.rs`, `contracts/parameters-contract/src/errors.rs`, `contracts/parameters-contract/src/tests.rs`, `contracts/creditline-contract/src/tests.rs` (added `confirm_multisig()` between configure and propose in the governance integration test), `context/progress-tracker.md`. `events.rs` and `errors.rs` were necessarily touched for the additive event/error surface required by the fix; the existing event symbols (`MSCONFIG`, `PROPNEW`, `PROPAPPR`, `PROPEXEC`, etc.) and error codes 1–17 are preserved.
+- **New tests (34 parameters + full workspace green):**
+  - **Exploit reproduction (end-to-end):** `test_stale_approval_from_removed_signer_is_never_counted` — 2-of-3 set, s1 proposes params + s2 approves (stale), s2 removed via `UpdateSigners`, then the old proposal's `execute` must fail and params must remain unchanged. **Pre-fix this test fails** (the proposal executed, rewriting params — the exact exploit); **post-fix it passes** (removed signer's approval is never counted). Also: `test_execute_rejects_proposal_whose_approver_was_removed` (#19), `test_removed_signer_cannot_approve_after_removal` (#10), `test_newly_added_signer_cannot_approve_old_proposal` (#19).
+  - **Elevated quorum:** `test_update_signers_requires_elevated_quorum` (#17 with 2-of-3 + 2 approvals), `test_update_signers_with_elevated_quorum_executes`, `test_signer_change_quorum_capped_at_full_committee_for_unanimous_set` (2-of-2 needs unanimity), `test_update_signers_via_proposal` updated to collect 3 approvals and assert snapshot.
+  - **Two-step config:** `test_configure_multisig_two_step_propose_confirm_emits_prominent_events` (MSCONFPR before activation, MSCONFIG after confirm, not-active-after-propose), `test_confirm_multisig_without_propose_fails` (#9), `test_configure_multisig_cannot_repropose_without_confirm` (#8), `test_confirm_multisig_cannot_be_called_twice` (#8), updated 2-step setup in `setup_multisig`/`test_three_of_three_with_full_committee_approval`.
+  - **Invalidation sweep:** `test_in_flight_signer_set_proposals_are_invalidated_on_signer_change` (+PROPIVLD event), `test_approve_rejects_invalidated_signer_set_proposal` (#18), `test_parameter_proposals_survive_signer_change_but_stale_approvals_revalidated`.
+- **Verification:** `cargo build --locked` (zero errors), `cargo test --locked` → **399 passed, 0 failed** workspace-wide (parameters 34, creditline 143, liquidity-pool 109, reputation 60, vendor-registry 26, vouching 27).
 - **Problem:** `set_admin()` in `contracts/reputation-contract/src/lib.rs` contained an unauthenticated fallback branch when no admin was stored, allowing anyone to claim admin of the reputation contract without authorization.
 - **Fix (`reputation-contract`):**
   - Added explicit, one-time `initialize(env, admin) -> Result<(), ReputationError>` function that requires `admin.require_auth()` and checks `storage::has_admin(&env)` (rejects re-initialization with `AlreadyInitialized = 11`).

contracts/creditline-contract/src/tests.rs

@@ -2705,6 +2705,7 @@ fn test_parameters_contract_controls_guarantee_thresholds() {
         &soroban_sdk::vec![&t.env, signer_a.clone(), signer_b.clone()],
         &2u32,
     );
+    t.parameters.confirm_multisig();
     let proposal_id = t
         .parameters
         .propose(&signer_a, &ProposalAction::UpdateParameters(params));

contracts/parameters-contract/src/errors.rs

@@ -21,4 +21,6 @@ pub enum ParametersError {
     ProposalAlreadyExecuted = 15,
     DuplicateSignature = 16,
     ThresholdNotMet = 17,
+    ProposalInvalidated = 18,
+    ApproverNotEligible = 19,
 }

contracts/parameters-contract/src/events.rs

@@ -1,13 +1,15 @@
 use soroban_sdk::{symbol_short, Address, Env, Symbol};
 
-use crate::types::ProtocolParameters;
+use crate::types::{MultisigConfig, ProtocolParameters};
 
 const PARAMS_UPDATED: Symbol = symbol_short!("PARMUPDT");
 const ADMIN_UPDATED: Symbol = symbol_short!("PARMADMN");
 const MS_CONFIGURED: Symbol = symbol_short!("MSCONFIG");
+const MS_CONFIGURE_PROPOSED: Symbol = symbol_short!("MSCONFPR");
 const PROP_CREATED: Symbol = symbol_short!("PROPNEW");
 const PROP_APPROVED: Symbol = symbol_short!("PROPAPPR");
 const PROP_EXECUTED: Symbol = symbol_short!("PROPEXEC");
+const PROP_INVALIDATED: Symbol = symbol_short!("PROPIVLD");
 
 pub fn emit_parameters_updated(env: &Env, admin: &Address, params: &ProtocolParameters) {
     env.events().publish(
@@ -41,6 +43,16 @@ pub fn emit_multisig_configured(env: &Env, threshold: u32, num_signers: u32) {
         .publish((MS_CONFIGURED,), (threshold, num_signers));
 }
 
+/// Prominent event emitted when an admin proposes a (pending) multisig config.
+/// Publishes the full signer set so the intent is observable on-chain before
+/// any change is applied.
+pub fn emit_multisig_configure_proposed(env: &Env, admin: &Address, config: &MultisigConfig) {
+    env.events().publish(
+        (MS_CONFIGURE_PROPOSED, admin),
+        (config.threshold, config.signers.clone()),
+    );
+}
+
 pub fn emit_proposal_created(env: &Env, id: u64, proposer: &Address) {
     env.events().publish((PROP_CREATED, proposer), id);
 }
@@ -52,3 +64,7 @@ pub fn emit_proposal_approved(env: &Env, id: u64, signer: &Address, approvals: u
 pub fn emit_proposal_executed(env: &Env, id: u64) {
     env.events().publish((PROP_EXECUTED,), id);
 }
+
+pub fn emit_proposal_invalidated(env: &Env, id: u64) {
+    env.events().publish((PROP_INVALIDATED,), id);
+}

contracts/parameters-contract/src/lib.rs

@@ -38,20 +38,45 @@ impl ParametersContract {
         Self::initialize(env, admin, default_parameters());
     }
 
+    /// Step 1 of a two-step multisig configuration. Admin-only: records a
+    /// pending signer set and emits a prominent on-chain event before anything
+    /// is applied. A single admin key can therefore no longer swap the signer
+    /// set silently — the intent is broadcast in a dedicated `MSCONFPR` event,
+    /// and the change is only applied by the explicit `confirm_multisig` step.
     pub fn configure_multisig(env: Env, signers: Vec<Address>, threshold: u32) {
         let admin = storage::get_admin(&env).unwrap_or_else(|err| panic_with_error!(&env, err));
         admin.require_auth();
         access::require_admin(&env, &admin);
 
-        if storage::has_multisig(&env) {
+        if storage::has_multisig(&env) || storage::has_pending_multisig(&env) {
             panic_with_error!(&env, ParametersError::MultisigAlreadyConfigured);
         }
 
         let config = MultisigConfig { signers, threshold };
         Self::validate_multisig_config(&env, &config);
 
+        Self::enter_non_reentrant(&env);
+        storage::set_pending_multisig(&env, &config);
+        events::emit_multisig_configure_proposed(&env, &admin, &config);
+        Self::exit_non_reentrant(&env);
+    }
+
+    /// Step 2 of the two-step multisig configuration. Admin-only: applies the
+    /// pending signer set previously proposed via `configure_multisig`.
+    pub fn confirm_multisig(env: Env) {
+        let admin = storage::get_admin(&env).unwrap_or_else(|err| panic_with_error!(&env, err));
+        admin.require_auth();
+        access::require_admin(&env, &admin);
+
+        if storage::has_multisig(&env) {
+            panic_with_error!(&env, ParametersError::MultisigAlreadyConfigured);
+        }
+        let config =
+            storage::get_pending_multisig(&env).unwrap_or_else(|err| panic_with_error!(&env, err));
+
         Self::enter_non_reentrant(&env);
         storage::set_multisig(&env, &config);
+        storage::clear_pending_multisig(&env);
         events::emit_multisig_configured(&env, config.threshold, config.signers.len());
         Self::exit_non_reentrant(&env);
     }
@@ -70,6 +95,12 @@ impl ParametersContract {
             _ => {}
         }
 
+        // Snapshot the current eligible signer set so approvals can be
+        // re-validated against it at approve() and execute() time — a signer
+        // removed (or added) after this point cannot influence this proposal.
+        let config = storage::get_multisig(&env).unwrap_or_else(|err| panic_with_error!(&env, err));
+        let snapshot = config.signers;
+
         let now = env.ledger().timestamp();
         let expires_at = now
             .checked_add(PROPOSAL_TTL_SECONDS)
@@ -84,11 +115,14 @@ impl ParametersContract {
             action,
             proposer: proposer.clone(),
             approvals,
+            snapshot,
             created_at: now,
             expires_at,
             executed: false,
+            invalidated: false,
         };
         storage::set_proposal(&env, &proposal);
+        storage::add_active_proposal(&env, id);
         events::emit_proposal_created(&env, id, &proposer);
         id
     }
@@ -103,33 +137,48 @@ impl ParametersContract {
         if proposal.executed {
             panic_with_error!(&env, ParametersError::ProposalAlreadyExecuted);
         }
+        if proposal.invalidated {
+            panic_with_error!(&env, ParametersError::ProposalInvalidated);
+        }
         if env.ledger().timestamp() > proposal.expires_at {
             panic_with_error!(&env, ParametersError::ProposalExpired);
         }
         if proposal.approvals.contains(&signer) {
             panic_with_error!(&env, ParametersError::DuplicateSignature);
         }
+        // The signer is currently a member (checked above), but it must also
+        // have been a member when the proposal was created.
+        if !proposal.snapshot.contains(&signer) {
+            panic_with_error!(&env, ParametersError::ApproverNotEligible);
+        }
 
         proposal.approvals.push_back(signer.clone());
         storage::set_proposal(&env, &proposal);
         events::emit_proposal_approved(&env, proposal_id, &signer, proposal.approvals.len());
     }
 
-    /// Execute a proposal once it has collected at least `threshold` approvals.
-    /// Permissionless — the collected approvals are the authorization.
+    /// Execute a proposal once it has collected the required number of
+    /// eligible approvals. Permissionless — the collected approvals are the
+    /// authorization.
     pub fn execute(env: Env, proposal_id: u64) {
         let mut proposal =
             storage::get_proposal(&env, proposal_id).unwrap_or_else(|err| panic_with_error!(&env, err));
 
         if proposal.executed {
             panic_with_error!(&env, ParametersError::ProposalAlreadyExecuted);
         }
+        if proposal.invalidated {
+            panic_with_error!(&env, ParametersError::ProposalInvalidated);
+        }
         if env.ledger().timestamp() > proposal.expires_at {
             panic_with_error!(&env, ParametersError::ProposalExpired);
         }
 
         let config = storage::get_multisig(&env).unwrap_or_else(|err| panic_with_error!(&env, err));
-        if proposal.approvals.len() < config.threshold {
+        // Re-validate every stored approval against the proposal's snapshot AND
+        // the current signer set; any approver removed since is never counted.
+        Self::require_eligible_approvers(&env, &config, &proposal);
+        if proposal.approvals.len() < Self::required_quorum(&config, &proposal.action) {
             panic_with_error!(&env, ParametersError::ThresholdNotMet);
         }
 
@@ -138,7 +187,7 @@ impl ParametersContract {
             ProposalAction::UpdateParameters(p) => Self::do_update_parameters(&env, &p),
             ProposalAction::SetAdmin(a) => Self::do_set_admin(&env, &a),
             ProposalAction::Upgrade(h) => Self::do_upgrade(&env, h),
-            ProposalAction::UpdateSigners(c) => Self::do_update_signers(&env, &c),
+            ProposalAction::UpdateSigners(c) => Self::do_update_signers(&env, &c, proposal.id),
         }
 
         proposal.executed = true;
@@ -186,10 +235,61 @@ impl ParametersContract {
         events::emit_contract_upgraded(env, old, new);
     }
 
-    fn do_update_signers(env: &Env, config: &MultisigConfig) {
+    fn do_update_signers(env: &Env, config: &MultisigConfig, executing_proposal_id: u64) {
         Self::validate_multisig_config(env, config);
         storage::set_multisig(env, config);
         events::emit_multisig_configured(env, config.threshold, config.signers.len());
+        // Any other in-flight proposal targeting the signer set is now stale —
+        // its snapshot no longer reflects the active committee. Void it so it
+        // cannot quietly execute against an outdated configuration.
+        Self::invalidate_signer_set_proposals(env, executing_proposal_id);
+    }
+
+    fn invalidate_signer_set_proposals(env: &Env, executing_proposal_id: u64) {
+        for id in storage::get_active_proposals(env).iter() {
+            if id == executing_proposal_id {
+                continue;
+            }
+            if let Ok(proposal) = storage::get_proposal(env, id) {
+                let is_signer_action = matches!(&proposal.action, ProposalAction::UpdateSigners(_));
+                if !proposal.executed && !proposal.invalidated && is_signer_action {
+                    let mut invalidated = proposal;
+                    invalidated.invalidated = true;
+                    storage::set_proposal(env, &invalidated);
+                    events::emit_proposal_invalidated(env, id);
+                }
+            }
+        }
+    }
+
+    /// Rejects a proposal if any stored approver is no longer a member of both
+    /// the proposal-time snapshot and the current signer set. This is what
+    /// guarantees removed signers' approvals are never counted at execution.
+    fn require_eligible_approvers(env: &Env, config: &MultisigConfig, proposal: &Proposal) {
+        for i in 0..proposal.approvals.len() {
+            let approver = proposal.approvals.get_unchecked(i);
+            if !proposal.snapshot.contains(&approver) || !config.signers.contains(&approver) {
+                panic_with_error!(env, ParametersError::ApproverNotEligible);
+            }
+        }
+    }
+
+    /// Required quorum for a proposal. Signer-set changes are escalation-guarded:
+    /// they need `threshold + 1` approvals, capped at the full signer count
+    /// (i.e. unanimity for already-unanimous sets), so a committee can never
+    /// cheapen its own gate with old-threshold approvals.
+    fn required_quorum(config: &MultisigConfig, action: &ProposalAction) -> u32 {
+        match action {
+            ProposalAction::UpdateSigners(_) => {
+                let n = config.signers.len();
+                config
+                    .threshold
+                    .checked_add(1)
+                    .map(|elevated| elevated.min(n))
+                    .unwrap_or(n)
+            }
+            _ => config.threshold,
+        }
     }
 
     fn validate_parameters(env: &Env, params: &ProtocolParameters) {

contracts/parameters-contract/src/storage.rs

@@ -1,4 +1,4 @@
-use soroban_sdk::{panic_with_error, symbol_short, Address, Env, Symbol};
+use soroban_sdk::{panic_with_error, symbol_short, Address, Env, Symbol, Vec};
 
 use crate::errors::ParametersError;
 use crate::types::{DataKey, MultisigConfig, Proposal, ProtocolParameters};
@@ -8,6 +8,8 @@ pub const PARAMS_KEY: Symbol = symbol_short!("PARAMS");
 pub const REENTRANCY_LOCK: Symbol = symbol_short!("LOCKED");
 pub const VERSION_KEY: Symbol = symbol_short!("VERSION");
 pub const MULTISIG_KEY: Symbol = symbol_short!("MSIG");
+pub const PENDING_MULTISIG_KEY: Symbol = symbol_short!("PMSCFG");
+pub const ACTIVE_PROPOSALS_KEY: Symbol = symbol_short!("PROPIDS");
 pub const PROP_CNT_KEY: Symbol = symbol_short!("PROPCNT");
 
 // TTL constants (in ledgers — 1 ledger ≈ 5 seconds on mainnet)
@@ -75,6 +77,43 @@ pub fn set_multisig(env: &Env, config: &MultisigConfig) {
     env.storage().instance().set(&MULTISIG_KEY, config);
 }
 
+/// Pending (proposed-but-not-confirmed) multisig config for the two-step
+/// `configure_multisig` → `confirm_multisig` flow. Instance storage.
+pub fn has_pending_multisig(env: &Env) -> bool {
+    env.storage().instance().has(&PENDING_MULTISIG_KEY)
+}
+
+pub fn get_pending_multisig(env: &Env) -> Result<MultisigConfig, ParametersError> {
+    env.storage()
+        .instance()
+        .get(&PENDING_MULTISIG_KEY)
+        .ok_or(ParametersError::MultisigNotConfigured)
+}
+
+pub fn set_pending_multisig(env: &Env, config: &MultisigConfig) {
+    env.storage().instance().set(&PENDING_MULTISIG_KEY, config);
+}
+
+pub fn clear_pending_multisig(env: &Env) {
+    env.storage().instance().remove(&PENDING_MULTISIG_KEY);
+}
+
+/// Index of every proposal ever created (by id). Used to sweep and invalidate
+/// in-flight signer-set proposals when the signer set changes. Instance
+/// storage — proposals themselves are persistent.
+pub fn get_active_proposals(env: &Env) -> Vec<u64> {
+    env.storage()
+        .instance()
+        .get(&ACTIVE_PROPOSALS_KEY)
+        .unwrap_or(Vec::new(env))
+}
+
+pub fn add_active_proposal(env: &Env, id: u64) {
+    let mut ids = get_active_proposals(env);
+    ids.push_back(id);
+    env.storage().instance().set(&ACTIVE_PROPOSALS_KEY, &ids);
+}
+
 pub fn next_proposal_id(env: &Env) -> u64 {
     let id: u64 = env.storage().instance().get(&PROP_CNT_KEY).unwrap_or(0u64);
     let next = id