fix(prediction-market): systemic accounting — solvent cancel refunds, live referral lookup, reentrancy guard - #33
Conversation
|
@fredericklamar342-prog
Since the PR scope only changes I'd suggest either:
I would also add an adversarial reentrancy regression test before considering defect 4 fully verified. The 48 passing tests are good for the cases covered by this patch, but they don't demonstrate resolution of the two issue areas that aren't touched by the PR. |
… live referral lookup, reentrancy guard cancel_market reclaimed fees with a global formula that could zero out fees earned by unrelated markets and refunded gross even when the referrer had already been paid (insolvent). Each bet now records its per-user refundable (= gross minus any referral fee already paid out), cancellation releases only this market's fee share and tracks the outstanding refund balance, and cancel_refund pays exactly the refundable. The HasReferrer cache is removed so a referrer registered after the first bet is honored. All external-calling entry points are wrapped in a reentrancy lock and place_bet writes state before calling out (CEI).
bccc88c to
b6da62c
Compare
|
@Muyideen-js Branch: fix/issue-1-systemic-accounting What was fixed: Tests: test_cancel_preserves_unrelated_market_fees, test_cancel_referrer_backed_refund_excludes_paid_fee, test_referral_registered_after_first_bet_honored, plus the adversarial test_reentrancy_guard_rejects_reentrant_bet (a malicious referral re-enters place_bet; the call is rejected and the bet counted once). Verified 51/51. Body changed to: Partially addresses #1 — defects 2 & 5 concern leaderboard / pulse_token and are tracked separately. |
Summary
Three interacting accounting/state-invariant defects let a cancellation corrupt the platform's fee bookkeeping, defraud referrers, and expose the contract to cross-contract reentrancy:
cancel_marketreclaimed fees with a global formula (fees_in_pool = net_pool * 200 / 9800) that could zero out the entire accumulator — including fees earned by unrelated, non-cancelled markets — and refunded the full gross even for referrer-backed bets whose referral fee had already been paid out (insolvent refund).HasReferrercache was never invalidated, so a user who registered a referrer after their first bet permanently diverted the 50 bps referral fee away from the referrer.place_betperformed external calls (XLM transfers,referral.credit) before writing bet state, enabling reentrancy that observed partially-updated state.Root Cause
Cancellation refunds were not funded by the market's actual physical funds (deposits minus referral payouts), the referrer lookup was cached forever with no invalidation path, and state writes were ordered after external calls.
Implementation
Refundable(market, user) = gross − referral fee paid out— exactly what the market physically holds for that bettor.cancel_marketsums the market's refundable, releases only this market's fee share from the global accumulator (acc −= refundable − net_pool), and tracks the outstanding balance inCancelState;cancel_refundpays exactly the refundable, so a cancelled market can never drain other markets' funds or the platform's accumulator.HasReferrercache is removed; the registry is consulted on every bet, so a late-registered referrer is honored from then on.Lockguards every external-calling entry point (place_bet,resolve_market,cancel_market,cancel_refund,claim,withdraw_fees), andplace_betwrites bet/market state before calling out.Security / Accounting Invariant
For every market,
Σ refundable == physical funds held by the market; cancellation releases exactly the market's own fee share; and no entry point can be re-entered mid-transaction.Tests
test_cancel_preserves_unrelated_market_fees— cancelling A leaves B's fees intact.test_cancel_referrer_backed_refund_excludes_paid_fee— solvent refund; referrer keeps the fee.test_referral_registered_after_first_bet_honored— late referrer is paid.test_reentrancy_guard_rejects_reentrant_bet— an adversarial referral contract re-entersplace_betmid-execution; the re-entrant call is rejected and the bet is counted exactly once (no double-counting). Note: the Soroban host rejects same-contract re-entry (Error(Context, InvalidAction)), so the contract's ownLockguard is defense-in-depth behind the host's protection — the test proves the end-to-end property holds.Verification
cargo test -p prediction_market→ 51 passed, 0 failed (rebased on currentmain, which includes the merged [CRITICAL] Payout rounding leaves dust permanently trapped in the contract — sum of payouts never equals the pool #2 settlement changes)git diff --check→ cleanScope
Only
prediction_market/src/lib.rs,prediction_market/src/tests.rs, and the new test snapshot fixtures are changed. No other crate or issue is touched.Issue
Partially addresses #1 — this PR resolves defects 1, 3, and 4 of #1 (accounting/referral/reentrancy in
prediction_market). Defects 2 and 5 of #1 concernleaderboardandpulse_tokenrespectively and are out of scope for this PR; they should be tracked separately.