Harden dispute arbitration: M-of-N committee, evidence binding, time-lock, bond - #799
Merged
fejilaup-cloud merged 3 commits intoAug 25, 2026
Conversation
…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
force-pushed
the
fix/dispute-arbitration-hardening-781
branch
from
August 25, 2026 05:50
a20821b to
63daf53
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
arbitrate_disputelet a single trustedAddress(set viaset_arbitrator) move disputed funds instantly, with zero binding to the evidence submitted viasubmit_dispute_evidence. This contradicteddocs/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:
Addressarbitrator with an M-of-N committee (set_arbitrator, minimum 2-of-3 per the threat model).arbitrate_disputenow requiresthresholdcommittee signers to jointly co-authorize a call in one transaction.arbitrate_disputerejects entering a ruling ifDisputeEvidenceis empty, and the evidence hashes actually considered (read from storage, not signer-supplied) are recorded inRulingEnteredEvent.arbitrate_disputenow only records a pending ruling; a newexecute_rulingmoves funds only after a 48-hour delay (ProtocolConfig.arbitration_ruling_delay_secs) has elapsed. A newcancel_pending_rulinglets the same committee threshold void a ruling within that window.submit_dispute_evidence(the greater of 1 XLM-equivalent in stroops or 10% of swap price), settled inexecute_ruling: the winning party is refunded, the losing party's bond is forfeited.ArbitratorCommitteeSetEvent,RulingEnteredEvent,RulingCancelledEvent,RulingExecutedEvent, plusDisputeBondDepositedEvent/ForfeitedEvent/RefundedEvent.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_disputealone. Handled as follows, and documented indocs/threat-model.md:batch_arbitrate_swapsnever validated itsarbitratorargument 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-keyarbitrate_dispute) becomes permanently unreachable now thatset_arbitratorno longer populates the legacySwapArbitratorkey it reads. This also closes a pre-existing double-payout risk: it never checked swap status, so it could previously still be called afterresolve_disputehad already paid out a swap.resolve_dispute,auto_refund_timeout, andadmin_rollback_swapcan each move a swap out ofDisputedwithout 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_disputeremains a real, disclosed, not-fixed-here bypass: the contract's ownAdminrole 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.Adminaddress rather thanprotocol_config().treasury— the latter is a confirmed pre-existing bug (a hardcoded placeholder address,store_protocol_configis 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.rswas disabled (// mod arbitration_tests;, commented out) due to 3 pre-existing, unrelated compile errors from an old merge conflict (a droppedcommit_ipargument, and two calls to anaccept_swap_with_quantityfunction that no longer exists). Re-enabled it, fixed thecommit_ipcall, and removed the two obsolete tests that referenced the removed function (they tested a tiered-pricing feature not present in the currentSwapRecord). 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— passescargo test --workspace --verbose— 282 tests pass, 0 failed, 0 unrelated regressions (66 inatomic_swap, including 27 in the re-enabledarbitration_tests, up from the 39 that ran before this PR touched nothing)EvidenceRequired), insufficient/non-committee signers, double-pending-ruling rejection, prematureexecute_rulingrejection, 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 directlyNo lint/format/typecheck step exists in
.github/workflows/ci.ymlbeyond build+test, so those two are the full local-check surface for this repo.Closes #781