mirror of
https://github.com/Such-Software/wownerogue.git
synced 2026-10-19 07:08:33 +02:00
security: fix batch-payout tx_hash collision and double-pay risk
transfer_split sends one on-chain tx to many destinations, so payout rows in a batch share a tx_hash. The old UNIQUE index on payouts.tx_hash made the 2nd+ row fail to record, leaving funds sent but rows stranded in 'processing' and re-sendable by the retry service. - Migration 015 drops the payouts tx_hash unique index (idempotency is enforced by idx_payouts_one_per_game) and extends that guard to cover 'processing' so no second payable row can be inserted mid-flight. - Batch completion now updates the whole batch in a single transaction via id = ANY(...), so rows can't be left half-completed. Per-user stats use BIGINT arithmetic in SQL (no JS float). - Ambiguous batch failures are marked 'needs_review' (which the retry service does NOT pick up) instead of 'failed', preventing a double-send when transfer_split may have already broadcast. Operator is alerted. Adds batchPayout.test.js covering the shared-tx_hash completion and the needs_review failure path. Full suite: 12 suites / 115 tests.
This commit is contained in:
@@ -19,7 +19,7 @@ Key: `S` ≤ half day · `M` ~1–3 days · `L` ~1 week+ · 🔴 launch-blocker
|
||||
|
||||
- [x] **0.1 🔴 S — Verify amount received ≥ expected before confirming.** ✅ Gate on `status.complete` in `paymentHandlers.js`; underpaid → stay pending + `payment_underpaid` warning (once). `received_amount` now recorded for single-game + credits. Regression test added (`payment_flow.integration.test.js`). _113 tests green._
|
||||
- [x] **0.2 🔴 M — Make provably-fair real.** ✅ Per-game seeded RNG (`seedToInt`) threaded through `dungeon.js` (Digger reseed + all `Math.random()` → `rng`), `monster.js` (`seededShuffle`), `game.js` (chase + generation). `DungeonGenerator.regenerateFromSeed` + `layoutFingerprint`; `/api/verify` regenerates & returns fingerprint. Determinism test suite added (`provablyFair.test.js`). _Verifier can now reproduce the exact dungeon from the seed._
|
||||
- [ ] **0.3 🔴 M — Fix batch-payout `tx_hash` collision + double-pay.** Stop writing one `tx_hash` to every row (`src/game/gameModeManager.js:1306`). Use per-destination hashes or a `batch_id`; ensure on-chain rows are never re-sent by retry. *Test: 3-dest batch → all completed, retry finds nothing.*
|
||||
- [x] **0.3 🔴 M — Fix batch-payout `tx_hash` collision + double-pay.** ✅ Migration 015 drops the payouts `tx_hash` unique index (batches legitimately share a tx_hash) and extends the one-per-game guard to cover `processing`. Batch completion now happens in ONE transaction via `id = ANY` (no row stranded mid-batch); BIGINT stat math (no float). Ambiguous batch failures → `needs_review` (retry service won't re-send) + operator alert. New `batchPayout.test.js` covers both paths.
|
||||
- [ ] **0.4 🔴 S — Harden identity transport.** Move `anon_token` from query string to handshake `auth` (`src/network/sessionManager.js:24`); call `rotateToken` on resume.
|
||||
- [ ] **0.5 🟠 S — Single-game disconnect recovery.** Extend `recoverPendingPayments` (`src/network/sessionManager.js:158`) to handle `single_game`; stop silently deleting in-progress paid games (`src/network/socketHandlers.js:787`).
|
||||
- [ ] **0.6 🟠 S — Client + admin XSS and CSP.** Escape chat/admin fields (`html/js/network/socketHandlers.js:291,323`; `html/admin.html`); add CSP headers.
|
||||
|
||||
+33
-18
@@ -1300,37 +1300,52 @@ class GameModeManager {
|
||||
}
|
||||
} else {
|
||||
// Multiple payouts — batch via transfer_split with multiple destinations
|
||||
const ids = pending.rows.map(p => p.id);
|
||||
try {
|
||||
const result = await this.walletService.processBatchPayout(destinations);
|
||||
|
||||
// All destinations in one transfer_split typically produce 1 tx
|
||||
const txHash = result.tx_hash_list[0];
|
||||
// transfer_split sends ONE on-chain tx to all destinations; every payout
|
||||
// row in this batch legitimately shares that tx_hash (see migration 015).
|
||||
const txHash = (result.tx_hash_list && result.tx_hash_list[0]) || null;
|
||||
const feePerPayout = result.totalFee
|
||||
? Math.ceil(result.totalFee / pending.rows.length)
|
||||
: null;
|
||||
|
||||
// Store tx_hash + status + user stats atomically for each payout
|
||||
for (const p of pending.rows) {
|
||||
await this.db.withTransaction(async (client) => {
|
||||
// Mark the ENTIRE batch completed in a SINGLE transaction so we never
|
||||
// leave some rows completed and others stranded mid-batch (which the old
|
||||
// per-row loop did when the shared tx_hash hit the unique index).
|
||||
await this.db.withTransaction(async (client) => {
|
||||
await client.query(
|
||||
`UPDATE payouts SET tx_hash = $1, fee = $2, batch_id = $3, status = 'completed', processed_at = NOW() WHERE id = ANY($4)`,
|
||||
[txHash, feePerPayout, batchId, ids]
|
||||
);
|
||||
// Per-row stat increments inside the same transaction. Amounts are passed
|
||||
// as DB values so Postgres does the BIGINT arithmetic (no JS float).
|
||||
for (const p of pending.rows) {
|
||||
await client.query(
|
||||
`UPDATE payouts SET tx_hash = $1, fee = $2, batch_id = $3, status = 'completed', processed_at = NOW() WHERE id = $4`,
|
||||
[txHash, feePerPayout, batchId, p.id]
|
||||
);
|
||||
await client.query(
|
||||
`UPDATE users SET total_amount_won = total_amount_won + $1, total_payouts_received = COALESCE(total_payouts_received, 0) + 1 WHERE id = $2`,
|
||||
`UPDATE users SET total_amount_won = total_amount_won + $1::bigint, total_payouts_received = COALESCE(total_payouts_received, 0) + 1 WHERE id = $2`,
|
||||
[p.amount, p.user_id]
|
||||
);
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
console.log(`💸 Batch payout completed: ${pending.rows.length} payouts in tx ${txHash}`);
|
||||
} catch (err) {
|
||||
console.error(`❌ Batch payout failed (${pending.rows.length} payouts):`, err.message);
|
||||
// Mark all as failed — retry service will pick them up individually
|
||||
for (const p of pending.rows) {
|
||||
await this.db.query(
|
||||
`UPDATE payouts SET status = 'failed', last_error = $1, last_retry_at = NOW() WHERE id = $2`,
|
||||
[err.message, p.id]
|
||||
).catch(() => {});
|
||||
// CRITICAL: a batch failure is ambiguous — transfer_split may have already
|
||||
// broadcast on-chain even though the RPC response errored. These rows have no
|
||||
// tx_hash, so the retry service's blockchain guard can't protect them and an
|
||||
// auto-retry could DOUBLE-PAY. Mark 'needs_review' (which the retry service
|
||||
// does NOT pick up) and alert an operator instead.
|
||||
await this.db.query(
|
||||
`UPDATE payouts SET status = 'needs_review', last_error = $1, last_retry_at = NOW() WHERE id = ANY($2)`,
|
||||
[String(err.message).slice(0, 500), ids]
|
||||
).catch(() => {});
|
||||
if (this.alertService && typeof this.alertService.sendAlert === 'function') {
|
||||
this.alertService.sendAlert('batch_payout_failed', {
|
||||
subject: '⚠️ Batch payout failed — manual review required',
|
||||
html: `<p>A batch of ${pending.rows.length} payout(s) failed and was marked <b>needs_review</b> to avoid a possible double-payout.</p>`
|
||||
+ `<p>Payout IDs: ${ids.join(', ')}</p><p>Error: ${err.message}</p>`
|
||||
}).catch(() => {});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,20 @@
|
||||
-- Migration 015: Allow batched payouts to share a tx_hash; tighten one-per-game guard
|
||||
--
|
||||
-- `transfer_split` sends ONE on-chain transaction to many destinations, so multiple
|
||||
-- payout rows in a batch legitimately share the same tx_hash. The old UNIQUE index on
|
||||
-- payouts.tx_hash (migration 008) made the 2nd+ row in a batch fail to record its
|
||||
-- tx_hash, leaving funds already sent but rows stranded in 'processing' — and at risk
|
||||
-- of being re-sent by the retry service (a double-payout vector).
|
||||
--
|
||||
-- Payout idempotency does NOT depend on tx_hash uniqueness; it is enforced by
|
||||
-- idx_payouts_one_per_game (one active payout per game). So we drop the payouts
|
||||
-- tx_hash unique index. (The payments.tx_hash unique index stays — incoming payments
|
||||
-- are 1:1 with a transaction.)
|
||||
DROP INDEX IF EXISTS idx_payouts_tx_hash_unique;
|
||||
|
||||
-- Extend the one-active-payout-per-game guard to also cover 'processing', so a second
|
||||
-- payable row cannot be inserted for a game while its payout is mid-flight.
|
||||
DROP INDEX IF EXISTS idx_payouts_one_per_game;
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS idx_payouts_one_per_game
|
||||
ON payouts(game_id)
|
||||
WHERE game_id IS NOT NULL AND status IN ('pending', 'processing', 'completed');
|
||||
@@ -0,0 +1,111 @@
|
||||
/**
|
||||
* Batch payout dispatch tests (Phase 0.3).
|
||||
*
|
||||
* transfer_split sends ONE on-chain tx to many destinations, so every payout row in a
|
||||
* batch shares that tx_hash. These tests guard:
|
||||
* - the whole batch is marked completed with the shared tx_hash in one transaction
|
||||
* (no row stranded mid-batch by a unique-index collision), and
|
||||
* - an ambiguous batch failure is marked 'needs_review' (NOT 'failed'), so the retry
|
||||
* service won't re-send and double-pay.
|
||||
*/
|
||||
|
||||
const GameModeManager = require('../src/game/gameModeManager');
|
||||
|
||||
const createMockDb = () => {
|
||||
const mockClient = { query: jest.fn().mockResolvedValue({ rows: [] }) };
|
||||
return {
|
||||
query: jest.fn().mockResolvedValue({ rows: [] }),
|
||||
withTransaction: jest.fn().mockImplementation(async (cb) => cb(mockClient)),
|
||||
_mockClient: mockClient
|
||||
};
|
||||
};
|
||||
|
||||
const createMockPaymentConfig = () => ({
|
||||
getConfig: () => ({
|
||||
paymentsEnabled: true,
|
||||
currency: { symbol: 'WOW', decimals: 11 },
|
||||
modes: {
|
||||
direct: { enabled: true, price: 100000000000n },
|
||||
credits: { enabled: true, creditsPerGame: 1, packages: [{ id: 'small', credits: 10, price: '500000000000', bonus: 0 }] }
|
||||
},
|
||||
payouts: {
|
||||
rules: {
|
||||
direct: { enabled: true, multipliers: { escape: 2, escapeWithTreasure: 3 } },
|
||||
credits: { enabled: false, multipliers: { escape: 1.5, escapeWithTreasure: 2 }, baseValue: 50000000000n }
|
||||
}
|
||||
},
|
||||
preferences: { preferCreditsFirst: true },
|
||||
earlyEntry: { enabled: false }
|
||||
}),
|
||||
getLegacyGameMode: () => 'PAID_SINGLE',
|
||||
eventBus: { on: () => {} }
|
||||
});
|
||||
|
||||
function buildGmm(walletService) {
|
||||
const db = createMockDb();
|
||||
const debugManager = { CONSOLE_LOGGING: false, getCurrentBlockHeight: () => 1 };
|
||||
const gmm = new GameModeManager(db, walletService, debugManager, createMockPaymentConfig());
|
||||
return { gmm, db };
|
||||
}
|
||||
|
||||
const TWO_PENDING = [
|
||||
{ id: 10, user_id: 1, game_id: 'g1', payout_address: 'addrA', amount: '200000000000', multiplier: 2, reason: 'escape' },
|
||||
{ id: 11, user_id: 2, game_id: 'g2', payout_address: 'addrB', amount: '300000000000', multiplier: 3, reason: 'escape' }
|
||||
];
|
||||
|
||||
describe('Batch payout dispatch', () => {
|
||||
test('marks the whole batch completed with the shared tx_hash in one transaction', async () => {
|
||||
const walletService = {
|
||||
processPayout: jest.fn(),
|
||||
processBatchPayout: jest.fn().mockResolvedValue({ tx_hash_list: ['txSHARED'], totalFee: 200 })
|
||||
};
|
||||
const { gmm, db } = buildGmm(walletService);
|
||||
|
||||
// First withTransaction = gather: SELECT returns 2 pending rows, then UPDATE ... processing.
|
||||
db._mockClient.query
|
||||
.mockResolvedValueOnce({ rows: TWO_PENDING }) // SELECT ... FOR UPDATE SKIP LOCKED
|
||||
.mockResolvedValue({ rows: [] }); // everything else
|
||||
|
||||
await gmm._processPendingPayouts();
|
||||
|
||||
// The batch RPC was called once with both destinations.
|
||||
expect(walletService.processBatchPayout).toHaveBeenCalledTimes(1);
|
||||
expect(walletService.processBatchPayout.mock.calls[0][0]).toHaveLength(2);
|
||||
|
||||
// The completion UPDATE set the shared tx_hash on the whole batch via id = ANY(...).
|
||||
const calls = db._mockClient.query.mock.calls.map(c => c[0]);
|
||||
const batchUpdate = calls.find(sql => /UPDATE payouts SET tx_hash/i.test(sql) && /id = ANY/i.test(sql));
|
||||
expect(batchUpdate).toBeDefined();
|
||||
expect(batchUpdate).toMatch(/status = 'completed'/);
|
||||
|
||||
const batchUpdateCall = db._mockClient.query.mock.calls.find(c => /UPDATE payouts SET tx_hash/i.test(c[0]) && /id = ANY/i.test(c[0]));
|
||||
expect(batchUpdateCall[1][0]).toBe('txSHARED'); // tx_hash param
|
||||
expect(batchUpdateCall[1][3]).toEqual([10, 11]); // ids array
|
||||
|
||||
// Per-user stats incremented for each winner inside the same transaction (BIGINT, no float).
|
||||
const statUpdates = calls.filter(sql => /UPDATE users SET total_amount_won/i.test(sql));
|
||||
expect(statUpdates.length).toBe(2);
|
||||
});
|
||||
|
||||
test('an ambiguous batch failure is marked needs_review, not failed', async () => {
|
||||
const walletService = {
|
||||
processPayout: jest.fn(),
|
||||
processBatchPayout: jest.fn().mockRejectedValue(new Error('RPC timeout'))
|
||||
};
|
||||
const { gmm, db } = buildGmm(walletService);
|
||||
|
||||
db._mockClient.query
|
||||
.mockResolvedValueOnce({ rows: TWO_PENDING })
|
||||
.mockResolvedValue({ rows: [] });
|
||||
|
||||
await gmm._processPendingPayouts();
|
||||
|
||||
// Failure handling uses db.query (outside a transaction) to mark the batch needs_review.
|
||||
const failCall = db.query.mock.calls.find(c => /needs_review/i.test(c[0]));
|
||||
expect(failCall).toBeDefined();
|
||||
expect(failCall[0]).toMatch(/id = ANY/i);
|
||||
// Must NOT mark them 'failed' (which the retry service would auto-resend).
|
||||
const failedCall = db.query.mock.calls.find(c => /status = 'failed'/i.test(c[0]));
|
||||
expect(failedCall).toBeUndefined();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user