fix(program): validate end_round token-account destinations + align tie-break with spec#2
Merged
Conversation
…ie-break with spec end_round accepted caller-supplied winner_token_account and treasury_token_account without verifying their SPL Token "owner" field matched the computed winner / configured treasury. SPL Token's Transfer CPI only checks mint-match between from/to, so anyone authorized to call end_round (any player in turn_order, not just authority/dealer) could redirect both the pot and the rake to their own ATA for the same mint. - Adds verify_token_account helper in utils/accounts.rs (checks SPL Token program ownership, mint at bytes 0..32, and owner field at bytes 32..64). - Wires it into end_round at all three Transfer sites: rake -> treasury, winner payout -> winner, rollover sweep -> treasury. - Adds InvalidTokenAccount error variant. Tie-break alignment (related, same redirect class): The winner-selection loop used strict `>`, leaving winner_index = None on ties or all-zero scores. The unvalidated transfer then paid out to whatever winner_token_account the caller passed -- a second instance of the same redirect vulnerability. Switching to `winner_index.is_none() || score > highest_score` makes the first stayed player in turn_order win ties, which is the documented design intent (EXECUTION_PLAN.md Task 1.12.2 step 3, Task 1.12.3 test #2). The implementation had drifted from the spec; this realigns it. Verified: cargo fmt + cargo build --all-targets clean, 42/42 lib tests pass, 11/11 phase2 integration tests pass. The three previously-passing integration tests (test_full_round_with_prize_distribution, test_vault_empty_after_payout, test_full_game_lifecycle_with_all_mechanics) had relied on the redirect bug to pay out under tied zero-scores; they now pass through the documented tie-break path.
georgedonnelly
force-pushed
the
fix/end-round-token-account-validation
branch
from
bee53b6 to
cc70c71
Compare
Member
|
Beautiful catch, thank you sir! Really appreciate it! |
georgedonnelly
added a commit
that referenced
this pull request
Apr 25, 2026
…follow-up) Add two litesvm rejection tests that hold PR #2's verify_token_account fix in place. PR #2 closed a Critical token-account-redirect vulnerability in `end_round` (caller-supplied destination + no owner check → any player in turn_order could redirect the pot or rake to their own same-mint ATA). Without these tests, a future refactor that drops a verify_token_account call would still pass `cargo test`. Tests: - `test_end_round_rejects_wrong_owner_winner_ata` — covers the winner payout wire (`end_round.rs:200`). Passes a legitimate finished round but substitutes an attacker-owned ATA as `winner_token_account`. - `test_end_round_rejects_wrong_owner_treasury_ata` — covers the rake wire (`end_round.rs:187`) independently. Passes a legitimate winner ATA but a wrong-owner treasury. The rollover-sweep wire (line 223) shares the same helper invocation pattern and is covered by extension. Both tests: - Assert `Custom(33)` (`InvalidTokenAccount`) via the centralized `INVALID_TOKEN_ACCOUNT_CODE` constant — extracts the magic discriminant to one place with a doc-comment explaining the position-in-enum coupling to `program/src/errors.rs`. - Assert post-rejection balances on vault, attacker ATA, and treasury. The rake CPI fires and succeeds before the winner-side check rejects, so the treasury balance assertion is the strongest invariant locked down: if Solana's atomic rollback ever broke, this would catch it. Setup deduplicated into a `setup_finished_round` helper. Also records the audit completion in docs/EXECUTION_PLAN.md as item 5 of the PR follow-ups block — sweep of all four SPL Token Transfer CPIs in the program found no other vulnerabilities of this class (3 in end_round now guarded; 1 in join_round transfers to a deterministic vault PDA, not caller-supplied). Verified: cargo build --all-targets clean, phase2 suite 11/11 → 13/13. No source files in program/ change — the fix is already on main from PR #2.
georgedonnelly
added a commit
that referenced
this pull request
Apr 27, 2026
…e 4.3 fix v2) Phase 4.3 dry-run #2 reported failure even though the deploy was fine: ✓ pushflip-pod active ✓ pushflip-vite active ✓ pushflip-faucet active [deploy] public-URL smoke check ✗ https://play.pushflip.xyz/ returned HTTP 502 (expected 200) Root cause: systemd reports `is-active` the moment the podman ExecStart returns (container in init), but the process inside (`serve` for vite, Hono for faucet) needs another 1-3 seconds to bind to its TCP port. nginx hits the upstream before the bind completes → 502 → script fails the smoke check → user runs the rollback command unnecessarily. Fix: replace the single curl with a smoke_check() function that retries up to 5 times with 2s / 4s / 6s / 8s backoff (max 20s extra wait). Same defensive pattern as the existing systemctl is-active loop above. The backoff doesn't add wallclock to a healthy deploy (first attempt succeeds), only to the timing-tight cases. Verified: site is up at https://play.pushflip.xyz/ — the previous "failed" deploy was fully successful, just smoke-checked too eagerly.
georgedonnelly
added a commit
that referenced
this pull request
Apr 28, 2026
Pre-Mainnet 5.2 / Phase 4. Wraps the existing Dealer class (dealer/src/dealer.ts) in a Hono daemon that signs commit_deck on chain and serves card reveals to the frontend. Decisions locked in 2026-04-28 per docs/wiki/operations/dealer-runbook.md. Dealer service: - dealer/src/service.ts: Hono daemon. Endpoints GET /health, GET /round/:gameId, GET /reveal/:gameId/:roundNumber/:leafIndex. Loads env config, opens RPC + WS, holds a single Dealer instance for one game_id. Boot-time SOL balance check + ZK-artifact pre-flight, fail-fast on missing env or missing keypair. Binds explicitly to 127.0.0.1 so nginx is the only ingress. - dealer/src/commit-tx.ts: extracts the commit_deck submission path (shuffle -> build ix with COMMIT_DECK_COMPUTE_LIMIT=400_000 CU bump -> blockhash -> sign -> confirm) so the auto-commit loop and any future caller share one implementation. - dealer/Dockerfile: same install discipline as faucet/Dockerfile after Lessons #54-#56 (npm install, --network=host on build, --ignore-scripts, workspace:* rewrite, retry loop, cache mount). ZK artifacts baked into the image at /app/zk-artifacts. - dealer/package.json: adds @hono/node-server, @solana/kit, @solana-program/compute-budget, hono, @pushflip/client. Auto-commit poll loop (Decision #2): - Polls GameSession every 5s; commits when !round_active && !deck_committed && active_player_count >= 2 && round_number != committedRoundNumber. - commitInFlight mutex is claimed BEFORE the first await so two setInterval ticks cannot race past the check while one is mid- fetch (heavy-duty review H2). - Reset branch checks !roundActive && !deckCommitted to distinguish "round ended" from "committed-waiting-for-start"; resetting on roundActive alone would wipe the local Merkle tree the moment commit_deck lands but before start_round runs (H1). - Manual POST /commit/:gameId deliberately NOT exposed: it would have been reachable unauthenticated via the public /api/dealer/* nginx prefix and lacked the player-count guard the auto-loop has (H3). Re-add behind a shared-secret header if ever needed. Frontend wiring (Decision #4): - app/src/hooks/use-game-actions.ts: hit() now reads on-chain round_number + draw_counter, fetches the matching reveal from the dealer (5s timeout, hex-decoded Merkle proof, schema validated), then submits the hit instruction. resolveDealerUrl mirrors resolveFaucetUrl - VITE_DEALER_URL required in prod, rejected if cross-origin, localhost:3002 default in dev. - HEX_SIBLING_HASH regex hoisted to module scope per biome's useTopLevelRegex. init_game configurability (Decision #1): - scripts/init-game.ts accepts DEALER_PUBKEY env var. When set, the on-chain dealer field of GameSession points at the dedicated dealer keypair instead of the CLI wallet. Mirrors the 5.0.7 faucet pattern: tucker compromise yields "can re-shuffle this game's deck", not arbitrary token movement. Defaults to wallet.address. Heavy-duty review fixes applied before commit: - C1: dealer/Dockerfile rewrite list now includes dealer/package.json (was missing - npm install would have failed with EUNSUPPORTEDPROTOCOL on workspace:*, Lesson #55 redux). - M3: Dockerfile comment env var corrected to ZK_ARTIFACTS_DIR. - M2: hex regex no longer case-insensitive.
georgedonnelly
added a commit
that referenced
this pull request
Apr 28, 2026
Update the dealer runbook now that the five open decisions are resolved and the implementation has landed: - Decision #1: dedicated dealer keypair (Option C - new game with pushflip-dealer.json, separate blast radius from CLI wallet). - Decision #2: polling auto-commit (5s tick, MIN_PLAYERS_TO_COMMIT guard). Manual /commit endpoint NOT exposed (heavy-duty review H3). - Decision #3: commit_deck submission extracted to dealer/src/commit-tx.ts. - Decision #4: frontend hit() fetches reveal from /api/dealer/reveal/:gameId/:roundNumber/:leafIndex. - Decision #5: nginx /api/dealer/* proxy block. Full diff against the live play.pushflip.xyz.conf inlined in the new "nginx diff" section so a future deploy doesn't need to re-derive it. Also notes the explicit 127.0.0.1 binding in service.ts (so nginx is the only ingress despite Network=host) and updates the deploy steps to point at the inline diff.
RamirezAlex
added a commit
that referenced
this pull request
May 8, 2026
…O_TO_SEEKER Phase 2) Adds the MWA adapter to the wallet provider so a TWA-wrapped pushflip running on Seeker / Saga can authorize via Seed Vault, satisfying prerequisite #2 of `docs/GO_TO_SEEKER.md`. - `@solana-mobile/wallet-adapter-mobile@2.2.8` added as runtime dep. RN peer-dep warning is harmless: the package has separate browser and react-native conditional exports; we only resolve the browser bundle. Bundle delta is ~220 bytes (most code was already pulled in via shared @Solana deps). - `wallet-provider.tsx` constructs `SolanaMobileWalletAdapter` inside `useMemo([])` — `BaseWalletProvider` tears down internal state when the wallets array reference changes, so a stable instance is load- bearing. Defaults from the adapter's helper factories (`createDefaultAddressSelector` / `createDefaultAuthorizationResultCache` / `createDefaultWalletNotFoundHandler`) cover the Seeker happy path without bespoke config. - `appIdentity.uri = window.location.origin` so dev (localhost:5173) and production (play.pushflip.xyz) both work without an env-var step. MUST match the assetlinks.json domain Phase 3 publishes — mismatch fails the MWA handshake silently (same shape as Lesson #46). - Adapter is registered unconditionally — its `readyState` reports `Unsupported` on non-Android contexts, so the wallet modal hides it automatically. Following Solana Mobile's documented pattern; gating ourselves would duplicate detection the adapter already does. - New `MWA_CHAIN = "solana:devnet"` constant in `lib/constants.ts` decoupled from `RPC_ENDPOINT` (a private mainnet RPC + `solana:mainnet` is a valid combo). Becomes a build-time env when the mainnet decision (Open Question #1 in GO_TO_SEEKER.md) lands. Spike conclusion (the M2.1 question that blocked starting Phase 2): **Lesson #46 fix is adapter-agnostic.** `wallet-bridge.ts` rebuilds `lifetimeConstraint` from the original Kit message, not from the signed result, so MWA-returned `VersionedTransaction` instances flow through the same `compile → sign → fromVersionedTransaction → re-merge lifetime` path as Phantom/Solflare. No bridge changes required. Verified: typecheck + full-repo lint + production build + 5/5 tests all clean. Bundle size: 930.32 → 930.54 KB (gzip 286.27 → 286.27 KB). Deferred to Phase 2 acceptance (per GO_TO_SEEKER.md, requires Android hardware/emulator): - Real-device test on Saga (compatibility floor). - M2.1 spike's runtime validation — sign one joinRound tx end-to-end via the MWA fake-wallet emulator. - Confirm `appIdentity.uri` matches assetlinks.json once Phase 3 publishes it.
RamirezAlex
added a commit
that referenced
this pull request
May 8, 2026
Status flips from PLANNED to IN PROGRESS. Adds a "Where we are (2026-05-08)" section per the EXECUTION_PLAN living-status pattern; marks prerequisites #1 (PWA shell) and #2 (MWA code-side) as ✅ DONE with commit refs (f132dcb, e3585cd); appends ✅ DONE markers to the Phase 1 + Phase 2 section headings. Phase 2 section gains an implementation-deviations subsection documenting two intentional departures from the original spec, each with reason: 1. appIdentity.uri derived from window.location.origin instead of a hardcoded play.pushflip.xyz — strictly more flexible (dev + prod + future staging hosts auto-track), same security property since the assetlinks contract is enforced at the production deploy domain level. 2. Adapter registered unconditionally instead of behind an Android- detection gate — adapter's readyState reports Unsupported on non-Android, modal hides it; gating ourselves would duplicate detection the adapter already does. M2.1 spike conclusion captured: Lesson #46 fix is adapter-agnostic (wallet-bridge.ts rebuilds lifetimeConstraint from the original Kit message, not the signed result), so MWA-returned VersionedTransaction flows through the same path as Phantom/Solflare. No bridge changes required. "Biggest unknowns at draft time" → "Biggest unknowns now"; the Seed Vault integration cost item is downgraded to "partially answered" with strikethrough preserving the original — code-level integration is mechanical (~30 lines + one constant); runtime cost still unverified pending Android hardware. Phase 5 brand-asset prerequisite gets a partial-credit note: the 1024×1024 source asset called out as a Phase 4 dApp Store requirement is already generated as part of the Phase 1 icon set (app/public/pwa-1024x1024.png), ahead of schedule.
7 tasks
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
2 participants
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.
end_round accepted caller-supplied winner_token_account and treasury_token_account without verifying their SPL Token "owner" field matched the computed winner / configured treasury. SPL Token's Transfer CPI only checks mint-match between from/to, so anyone authorized to call end_round (any player in turn_order, not just authority/dealer) could redirect both the pot and the rake to their own ATA for the same mint.
Tie-break alignment (related, same redirect class):
The winner-selection loop used strict
>, leaving winner_index = None on ties or all-zero scores. The unvalidated transfer then paid out to whatever winner_token_account the caller passed -- a second instance of the same redirect vulnerability. Switching towinner_index.is_none() || score > highest_scoremakes the first stayed player in turn_order win ties, which is the documented design intent (EXECUTION_PLAN.md Task 1.12.2 step 3, Task 1.12.3 test #2). The implementation had drifted from the spec; this realigns it.Verified: cargo fmt + cargo build --all-targets clean, 42/42 lib tests pass, 11/11 phase2 integration tests pass. The three previously-passing integration tests (test_full_round_with_prize_distribution, test_vault_empty_after_payout, test_full_game_lifecycle_with_all_mechanics) had relied on the redirect bug to pay out under tied zero-scores; they now pass through the documented tie-break path.