diff --git a/context/progress-tracker.md b/context/progress-tracker.md index e20f7e6..0f5aeca 100644 --- a/context/progress-tracker.md +++ b/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
` 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
` (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`). diff --git a/contracts/creditline-contract/src/tests.rs b/contracts/creditline-contract/src/tests.rs index 32b6631..da6d34f 100644 --- a/contracts/creditline-contract/src/tests.rs +++ b/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)); diff --git a/contracts/parameters-contract/src/errors.rs b/contracts/parameters-contract/src/errors.rs index ce27739..b443b48 100644 --- a/contracts/parameters-contract/src/errors.rs +++ b/contracts/parameters-contract/src/errors.rs @@ -21,4 +21,6 @@ pub enum ParametersError { ProposalAlreadyExecuted = 15, DuplicateSignature = 16, ThresholdNotMet = 17, + ProposalInvalidated = 18, + ApproverNotEligible = 19, } diff --git a/contracts/parameters-contract/src/events.rs b/contracts/parameters-contract/src/events.rs index f3dab52..0769672 100644 --- a/contracts/parameters-contract/src/events.rs +++ b/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); +} diff --git a/contracts/parameters-contract/src/lib.rs b/contracts/parameters-contract/src/lib.rs index 837b6bc..ed90c1e 100644 --- a/contracts/parameters-contract/src/lib.rs +++ b/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
, 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,20 +137,29 @@ 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)); @@ -124,12 +167,18 @@ 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); } 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) { diff --git a/contracts/parameters-contract/src/storage.rs b/contracts/parameters-contract/src/storage.rs index 6f2c3e6..7fa45fb 100644 --- a/contracts/parameters-contract/src/storage.rs +++ b/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 { + 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 { + 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 diff --git a/contracts/parameters-contract/src/tests.rs b/contracts/parameters-contract/src/tests.rs index d62cd32..0f20ac7 100644 --- a/contracts/parameters-contract/src/tests.rs +++ b/contracts/parameters-contract/src/tests.rs @@ -37,10 +37,27 @@ fn setup_multisig() -> ( let s3 = Address::generate(&env); let signers: Vec
= vec![&env, s1.clone(), s2.clone(), s3.clone()]; client.configure_multisig(&signers, &2u32); + client.confirm_multisig(); (env, client, admin, s1, s2, s3) } +fn signer_update_action(signers: Vec
, threshold: u32) -> ProposalAction { + ProposalAction::UpdateSigners(MultisigConfig { signers, threshold }) +} + +fn has_event(env: &Env, symbol: &str) -> bool { + let events: soroban_sdk::Vec<(Address, soroban_sdk::Vec, soroban_sdk::Val)> = + env.events().all(); + for e in events.iter() { + let topic: soroban_sdk::Symbol = e.1.get_unchecked(0).into_val(env); + if topic == soroban_sdk::Symbol::new(env, symbol) { + return true; + } + } + false +} + #[test] fn test_initialize_defaults() { let (_env, client, admin) = setup(); @@ -96,6 +113,74 @@ fn test_configure_multisig_stores_committee() { assert!(config.signers.contains(&s3)); } +#[test] +fn test_configure_multisig_two_step_propose_confirm_emits_prominent_events() { + let (env, client, admin) = setup(); + client.initialize_defaults(&admin); + + let s1 = Address::generate(&env); + let s2 = Address::generate(&env); + let s3 = Address::generate(&env); + let signers: Vec
= vec![&env, s1.clone(), s2.clone(), s3.clone()]; + + client.configure_multisig(&signers, &2u32); + // Step 1 alone must NOT activate the committee — a single admin call can + // no longer silently swap the set. (Event checks must run immediately + // after the emitting call: each contract invocation resets the test event + // log.) + assert!(has_event(&env, "MSCONFPR"), "MSCONFPR event not found"); + assert_eq!( + client.try_get_multisig(), + Err(Ok(ParametersError::MultisigNotConfigured)) + ); + + client.confirm_multisig(); + assert!(has_event(&env, "MSCONFIG"), "MSCONFIG event not found"); + let config = client.get_multisig(); + assert_eq!(config.threshold, 2); + assert_eq!(config.signers.len(), 3); + + // The confirmed committee governs proposals as expected. + let id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); + client.approve(&s2, &id); + client.execute(&id); + assert!(client.get_proposal(&id).executed); +} + +#[test] +#[should_panic(expected = "Error(Contract, #9)")] // MultisigNotConfigured +fn test_confirm_multisig_without_propose_fails() { + let (_env, client, admin) = setup(); + client.initialize_defaults(&admin); + + client.confirm_multisig(); +} + +#[test] +#[should_panic(expected = "Error(Contract, #8)")] // MultisigAlreadyConfigured +fn test_configure_multisig_cannot_repropose_without_confirm() { + let (env, client, admin) = setup(); + client.initialize_defaults(&admin); + + let s1 = Address::generate(&env); + let s2 = Address::generate(&env); + client.configure_multisig(&vec![&env, s1.clone(), s2.clone()], &2u32); + client.configure_multisig(&vec![&env, s1, s2], &2u32); +} + +#[test] +#[should_panic(expected = "Error(Contract, #8)")] // MultisigAlreadyConfigured +fn test_confirm_multisig_cannot_be_called_twice() { + let (env, client, admin) = setup(); + client.initialize_defaults(&admin); + + let s1 = Address::generate(&env); + let s2 = Address::generate(&env); + client.configure_multisig(&vec![&env, s1, s2], &2u32); + client.confirm_multisig(); + client.confirm_multisig(); +} + #[test] #[should_panic(expected = "Error(Contract, #11)")] // InvalidThreshold fn test_configure_multisig_rejects_threshold_below_two() { @@ -238,7 +323,7 @@ fn test_set_admin_via_proposal() { #[test] fn test_update_signers_via_proposal() { - let (env, client, _admin, s1, s2, _s3) = setup_multisig(); + let (env, client, _admin, s1, s2, s3) = setup_multisig(); let n1 = Address::generate(&env); let n2 = Address::generate(&env); @@ -248,7 +333,9 @@ fn test_update_signers_via_proposal() { }; let id = client.propose(&s1, &ProposalAction::UpdateSigners(new_config)); + // Signer-set changes need elevated quorum (threshold + 1 = 3 for 2-of-3). client.approve(&s2, &id); + client.approve(&s3, &id); client.execute(&id); let config = client.get_multisig(); @@ -257,6 +344,260 @@ fn test_update_signers_via_proposal() { assert!(config.signers.contains(&n2)); // Old signers are no longer part of the committee. assert!(!config.signers.contains(&s1)); + + // The proposal recorded a snapshot of the original eligible signer set. + let proposal = client.get_proposal(&id); + assert!(!proposal.invalidated); + assert_eq!(proposal.snapshot.len(), 3); + assert!(proposal.snapshot.contains(&s1)); + assert!(proposal.snapshot.contains(&s2)); + assert!(proposal.snapshot.contains(&s3)); +} + +#[test] +#[should_panic(expected = "Error(Contract, #17)")] // ThresholdNotMet +fn test_update_signers_requires_elevated_quorum() { + let (env, client, _admin, s1, s2, _s3) = setup_multisig(); + + let n1 = Address::generate(&env); + let n2 = Address::generate(&env); + let id = client.propose( + &s1, + &signer_update_action(vec![&env, n1, n2], 2), + ); + // s1 (proposer auto-approves) + s2 = 2 approvals, but signer changes in a + // 2-of-3 set require threshold + 1 = 3. + client.approve(&s2, &id); + client.execute(&id); +} + +#[test] +fn test_update_signers_with_elevated_quorum_executes() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let n1 = Address::generate(&env); + let n2 = Address::generate(&env); + let id = client.propose( + &s1, + &signer_update_action(vec![&env, n1.clone(), n2.clone()], 2), + ); + client.approve(&s2, &id); + client.approve(&s3, &id); + client.execute(&id); + + let config = client.get_multisig(); + assert!(config.signers.contains(&n1)); + assert!(config.signers.contains(&n2)); + assert!(!config.signers.contains(&s1)); + assert!(!config.signers.contains(&s2)); +} + +#[test] +fn test_signer_change_quorum_capped_at_full_committee_for_unanimous_set() { + let (env, client, admin) = setup(); + client.initialize_defaults(&admin); + + let s1 = Address::generate(&env); + let s2 = Address::generate(&env); + client.configure_multisig(&vec![&env, s1.clone(), s2.clone()], &2u32); + client.confirm_multisig(); + + let n1 = Address::generate(&env); + let n2 = Address::generate(&env); + let id = client.propose( + &s1, + &signer_update_action(vec![&env, n1, n2], 2), + ); + // 2-of-2 committee: elevated quorum min(2 + 1, 2) = 2 = unanimity. One + // approval (the proposer) is not enough. + assert!(client.try_execute(&id).is_err()); + client.approve(&s2, &id); + client.execute(&id); + assert_eq!(client.get_multisig().signers.len(), 2); +} + +#[test] +fn test_stale_approval_from_removed_signer_is_never_counted() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let params = ProtocolParameters { + min_guarantee_percent: 30, + ..default_parameters() + }; + + // s1 proposes a parameter change and s2 approves it — 2-of-3 reached. + let id = client.propose(&s1, &ProposalAction::UpdateParameters(params.clone())); + client.approve(&s2, &id); + + // The signer set changes and removes s2. s2 consents to its own removal, + // so the change proposal reaches the elevated quorum of 3. + let new_set = MultisigConfig { + signers: vec![&env, s1.clone(), s3.clone()], + threshold: 2, + }; + let change_id = client.propose(&s2, &ProposalAction::UpdateSigners(new_set)); + client.approve(&s1, &change_id); + client.approve(&s3, &change_id); + client.execute(&change_id); + assert!(!client.get_multisig().signers.contains(&s2)); + + // s2's stale approval on the parameter proposal must NOT count. + let proposal = client.get_proposal(&id); + assert_eq!(proposal.approvals.len(), 2); // s1 + s2 + assert!(proposal.snapshot.contains(&s2)); // it was a signer at propose time + assert!(client.try_execute(&id).is_err()); + // The exploit would have rewritten parameters; post-fix they are intact. + assert_eq!(client.get_parameters(), default_parameters()); +} + +#[test] +#[should_panic(expected = "Error(Contract, #19)")] // ApproverNotEligible +fn test_execute_rejects_proposal_whose_approver_was_removed() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); + client.approve(&s2, &id); // s2's approval will go stale. + + let new_set = MultisigConfig { + signers: vec![&env, s1.clone(), s3.clone()], + threshold: 2, + }; + let change_id = client.propose(&s3, &ProposalAction::UpdateSigners(new_set)); + client.approve(&s1, &change_id); + client.approve(&s2, &change_id); + client.execute(&change_id); + + // execute() re-validates every historical approver: s2 is gone. + client.execute(&id); +} + +#[test] +#[should_panic(expected = "Error(Contract, #19)")] // ApproverNotEligible +fn test_newly_added_signer_cannot_approve_old_proposal() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); + + // Replace the committee with a brand-new signer s4 (elevated quorum). + let s4 = Address::generate(&env); + let new_set = MultisigConfig { + signers: vec![&env, s3.clone(), s4.clone()], + threshold: 2, + }; + let change_id = client.propose(&s2, &ProposalAction::UpdateSigners(new_set)); + client.approve(&s1, &change_id); + client.approve(&s3, &change_id); + client.execute(&change_id); + + // s4 is a current signer but was not a member at proposal time. + client.approve(&s4, &id); +} + +#[test] +#[should_panic(expected = "Error(Contract, #10)")] // NotSigner +fn test_removed_signer_cannot_approve_after_removal() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); + + let new_set = MultisigConfig { + signers: vec![&env, s1.clone(), s3.clone()], + threshold: 2, + }; + let change_id = client.propose(&s2, &ProposalAction::UpdateSigners(new_set)); + client.approve(&s1, &change_id); + client.approve(&s3, &change_id); + client.execute(&change_id); + + // s2 is no longer a current signer — approve() rejects it outright. + client.approve(&s2, &id); +} + +#[test] +fn test_in_flight_signer_set_proposals_are_invalidated_on_signer_change() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + // In-flight signer-set proposal A (never executes). + let a_id = client.propose( + &s1, + &signer_update_action(vec![&env, s1.clone(), s2.clone()], 2), + ); + + // Signer-set proposal B executes first. + let b_config = MultisigConfig { + signers: vec![&env, s2.clone(), s3.clone()], + threshold: 2, + }; + let b_id = client.propose(&s2, &ProposalAction::UpdateSigners(b_config.clone())); + client.approve(&s1, &b_id); + client.approve(&s3, &b_id); + client.execute(&b_id); + + // The signer change voids every other in-flight signer-set proposal. + // (Event checks must run immediately after the emitting call: each + // contract invocation resets the test event log.) + assert!(has_event(&env, "PROPIVLD"), "PROPIVLD event not found"); + + // Proposal A is stale now: voided, cannot collect approval or execute. + assert!(client.get_proposal(&a_id).invalidated); + assert!(client.try_execute(&a_id).is_err()); + + // The executed proposal that drove the change is NOT flagged invalidated. + let b = client.get_proposal(&b_id); + assert!(b.executed); + assert!(!b.invalidated); + assert_eq!(client.get_multisig(), b_config); +} + +#[test] +#[should_panic(expected = "Error(Contract, #18)")] // ProposalInvalidated +fn test_approve_rejects_invalidated_signer_set_proposal() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + let a_id = client.propose( + &s1, + &signer_update_action(vec![&env, s1.clone(), s2.clone()], 2), + ); + + let b_id = client.propose( + &s2, + &signer_update_action(vec![&env, s2.clone(), s3.clone()], 2), + ); + client.approve(&s1, &b_id); + client.approve(&s3, &b_id); + client.execute(&b_id); + + // a_id was invalidated by the signer change — a fresh approval must fail. + client.approve(&s2, &a_id); +} + +#[test] +fn test_parameter_proposals_survive_signer_change_but_stale_approvals_revalidated() { + let (env, client, _admin, s1, s2, s3) = setup_multisig(); + + // Parameter proposal by s1. + let p_id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); + + // Remove s1 via signer change (elevated quorum: all three approve). + let new_set = MultisigConfig { + signers: vec![&env, s2.clone(), s3.clone()], + threshold: 2, + }; + let change_id = client.propose(&s1, &ProposalAction::UpdateSigners(new_set)); + client.approve(&s2, &change_id); + client.approve(&s3, &change_id); + client.execute(&change_id); + + // Parameter proposals are NOT invalidated by a signer change... + assert!(!client.get_proposal(&p_id).invalidated); + // ...but the stale proposer approval (s1, now removed) blocks execution. + assert!(client.try_execute(&p_id).is_err()); + + // A still-eligible signer can add an approval, yet the stale s1 approval + // means the proposal can never execute — signers re-propose to move on. + client.approve(&s2, &p_id); + assert!(client.try_execute(&p_id).is_err()); + assert_eq!(client.get_parameters(), default_parameters()); } #[test] @@ -273,17 +614,7 @@ fn test_upgrade_via_proposal_increments_version() { client.approve(&s2, &id); client.execute(&id); - let events: soroban_sdk::Vec<(Address, soroban_sdk::Vec, soroban_sdk::Val)> = - env.events().all(); - let mut found = false; - for e in events.iter() { - let topic: soroban_sdk::Symbol = e.1.get_unchecked(0).into_val(&env); - if topic == soroban_sdk::Symbol::new(&env, "CONTRACTUPGRADED") { - found = true; - break; - } - } - assert!(found, "CONTRACTUPGRADED event not found"); + assert!(has_event(&env, "CONTRACTUPGRADED"), "CONTRACTUPGRADED event not found"); } #[test] @@ -295,6 +626,7 @@ fn test_three_of_three_with_full_committee_approval() { let s2 = Address::generate(&env); let s3 = Address::generate(&env); client.configure_multisig(&vec![&env, s1.clone(), s2.clone(), s3.clone()], &3u32); + client.confirm_multisig(); let id = client.propose(&s1, &ProposalAction::UpdateParameters(default_parameters())); client.approve(&s2, &id); diff --git a/contracts/parameters-contract/src/types.rs b/contracts/parameters-contract/src/types.rs index 73c0af7..3f3e43c 100644 --- a/contracts/parameters-contract/src/types.rs +++ b/contracts/parameters-contract/src/types.rs @@ -68,9 +68,18 @@ pub struct Proposal { pub action: ProposalAction, pub proposer: Address, pub approvals: Vec
, + /// The signer set captured at proposal time. Every stored approval must be + /// a member of this snapshot AND of the current signer set at approve() and + /// execute() time, so signers removed (or added) since the proposal was + /// created can never lend it stale or illegitimate approval power. + pub snapshot: Vec
, pub created_at: u64, pub expires_at: u64, pub executed: bool, + /// Set to true when an in-flight signer-set proposal is voided because the + /// signer set changed before it could execute. Invalidated proposals can + /// neither collect new approvals nor be executed. + pub invalidated: bool, } #[contracttype]