Skip to content

Harden dispute arbitration: M-of-N committee, evidence binding, time-lock, bond - #799

Merged
fejilaup-cloud merged 3 commits into
AtomicIP:mainfrom
liamscroxx-svg:fix/dispute-arbitration-hardening-781
Aug 25, 2026
Merged

Harden dispute arbitration: M-of-N committee, evidence binding, time-lock, bond#799
fejilaup-cloud merged 3 commits into
AtomicIP:mainfrom
liamscroxx-svg:fix/dispute-arbitration-hardening-781

Conversation

@liamscroxx-svg

Copy link
Copy Markdown
Contributor

Summary

arbitrate_dispute let a single trusted Address (set via set_arbitrator) move disputed funds instantly, with zero binding to the evidence submitted via submit_dispute_evidence. This contradicted docs/threat-model.md's stated mitigations for the dispute-resolution trust boundary: a 2-of-3 multisig, a 48-hour time-lock between ruling and fund release, and a non-refundable dispute bond. None of that existed in code.

This PR:

  • Replaces the single-Address arbitrator with an M-of-N committee (set_arbitrator, minimum 2-of-3 per the threat model). arbitrate_dispute now requires threshold committee signers to jointly co-authorize a call in one transaction.
  • Binds the ruling to evidence: arbitrate_dispute rejects entering a ruling if DisputeEvidence is empty, and the evidence hashes actually considered (read from storage, not signer-supplied) are recorded in RulingEnteredEvent.
  • Splits ruling from execution: arbitrate_dispute now only records a pending ruling; a new execute_ruling moves funds only after a 48-hour delay (ProtocolConfig.arbitration_ruling_delay_secs) has elapsed. A new cancel_pending_ruling lets the same committee threshold void a ruling within that window.
  • Adds a non-refundable dispute bond to submit_dispute_evidence (the greater of 1 XLM-equivalent in stroops or 10% of swap price), settled in execute_ruling: the winning party is refunded, the losing party's bond is forfeited.
  • Adds a distinct audit event per transition: ArbitratorCommitteeSetEvent, RulingEnteredEvent, RulingCancelledEvent, RulingExecutedEvent, plus DisputeBondDepositedEvent/ForfeitedEvent/RefundedEvent.
  • Preserves submit_dispute_evidence/get_dispute_evidence's existing evidence-submission/retrieval behavior.

Known-gap disclosures (found during review, out of scope for this PR)

Research surfaced that the dispute-resolution trust boundary is wider than set_arbitrator/arbitrate_dispute alone. Handled as follows, and documented in docs/threat-model.md:

  • batch_arbitrate_swaps never validated its arbitrator argument against any stored arbitrator at all — any caller could drain any disputed swap through it, fully bypassing the system above. It is now disabled unconditionally (always reverts) pending a follow-up issue to migrate it onto the committee model.
  • arbitrate_swap (a collateral-aware, untested duplicate of the old single-key arbitrate_dispute) becomes permanently unreachable now that set_arbitrator no longer populates the legacy SwapArbitrator key it reads. This also closes a pre-existing double-payout risk: it never checked swap status, so it could previously still be called after resolve_dispute had already paid out a swap.
  • resolve_dispute, auto_refund_timeout, and admin_rollback_swap can each move a swap out of Disputed without going through a committee ruling. Left untouched, this would orphan any dispute bond already deposited. All three now call a shared cleanup path that refunds any outstanding bond in full (no forfeiture — no ruling occurred) and clears in-flight committee/ruling state.
  • resolve_dispute remains a real, disclosed, not-fixed-here bypass: the contract's own Admin role is still single-key and can resolve any disputed swap directly, skipping the committee/evidence/timelock/bond system entirely. docs/threat-model.md's Admin Collusion section is updated to say so explicitly rather than implying full mitigation.
  • Dispute-bond forfeitures are routed to the current Admin address rather than protocol_config().treasury — the latter is a confirmed pre-existing bug (a hardcoded placeholder address, store_protocol_config is a no-op stub) that this PR does not attempt to fix; routing real forfeited value through it would have been a new, self-inflicted loss path.

Test infrastructure

arbitration_tests.rs was disabled (// mod arbitration_tests;, commented out) due to 3 pre-existing, unrelated compile errors from an old merge conflict (a dropped commit_ip argument, and two calls to an accept_swap_with_quantity function that no longer exists). Re-enabled it, fixed the commit_ip call, and removed the two obsolete tests that referenced the removed function (they tested a tiered-pricing feature not present in the current SwapRecord). All other currently-disabled test modules are left untouched — that's separate, pre-existing tech debt outside this issue's scope.

Test plan

  • cargo build --workspace --verbose — passes
  • cargo test --workspace --verbose — 282 tests pass, 0 failed, 0 unrelated regressions (66 in atomic_swap, including 27 in the re-enabled arbitration_tests, up from the 39 that ran before this PR touched nothing)
  • New tests cover: committee validation (size/threshold/duplicate signers), evidence-binding (EvidenceRequired), insufficient/non-committee signers, double-pending-ruling rejection, premature execute_ruling rejection, cancel within/after the time-lock window, bond deposit/forfeit/refund on both ruling outcomes, and bond refund via the bypass-path cleanup (resolve_dispute)
  • batch_arbitrate_swaps's disabled behavior is asserted directly

No lint/format/typecheck step exists in .github/workflows/ci.yml beyond build+test, so those two are the full local-check surface for this repo.

Closes #781

liamscroxx-svg and others added 3 commits August 25, 2026 02:29
…lock, bond

set_arbitrator/arbitrate_dispute previously let a single trusted key move
disputed funds instantly with no binding to submitted evidence, contradicting
docs/threat-model.md's claimed 2-of-3 multisig, 48h time-lock, and
non-refundable dispute bond mitigations. This closes that gap:

- set_arbitrator now designates an M-of-N committee (min 2-of-3) instead of a
  single Address.
- arbitrate_dispute becomes ruling entry, not fund movement: requires
  threshold committee co-authorization and requires evidence to already be
  submitted (read from storage, not signer-supplied).
- New execute_ruling moves funds only after a 48h delay past ruling entry;
  new cancel_pending_ruling lets the committee void a ruling within that
  window.
- submit_dispute_evidence now charges a non-refundable bond (max of 1 XLM in
  stroops or 10% of price) on first submission per party, settled in
  execute_ruling: winner refunded, loser forfeited.
- resolve_dispute, auto_refund_timeout, and admin_rollback_swap (bypass paths
  that can move a swap out of Disputed without a committee ruling) now refund
  any outstanding bond and clear in-flight committee/ruling state instead of
  orphaning it.
- batch_arbitrate_swaps never validated its arbitrator argument at all and is
  now disabled unconditionally, since leaving it live would bypass everything
  above; arbitrate_swap becomes unreachable now that set_arbitrator no longer
  populates the legacy single-key storage it read, which also closes a
  pre-existing double-payout path (it never checked swap status).
- docs/threat-model.md's Admin Collusion / False Dispute Submission sections
  updated to reflect exactly what's implemented vs. still open (contract
  Admin role is still single-key; resolve_dispute remains an admin-direct
  bypass of the new safeguards).

Also re-enables arbitration_tests.rs (disabled since a prior merge conflict
over unrelated compile errors) with tests for the new committee/evidence/
timelock/bond behavior plus the bypass-path bond cleanup.
…lib.rs

The M-of-N committee/timelock/bond error variants (NotACommitteeSigner,
DuplicateSigner, InsufficientSignatures, CommitteeSizeTooSmall,
EvidenceRequired, RulingAlreadyPending, NoPendingRuling,
TimelockNotElapsed, RulingFinalized, BatchArbitrationDisabled) were only
added to the errors.rs reference mirror, not to the authoritative
#[contracterror] enum in lib.rs that the new arbitration code actually
references, breaking the build. Also fixes a discriminant collision in
errors.rs where the mirrored codes reused 54 (already OracleDeviationExceeded).
@liamscroxx-svg
liamscroxx-svg force-pushed the fix/dispute-arbitration-hardening-781 branch from a20821b to 63daf53 Compare August 25, 2026 05:50
@fejilaup-cloud
fejilaup-cloud merged commit 14573fc into AtomicIP:main Aug 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dispute resolution is a single-key instant ruling with none of the threat model's documented safeguards implemented

2 participants