From 25ed29edcb8ba20793b0b4ba10222a9729f3547b Mon Sep 17 00:00:00 2001 From: Satoshi Qazi Muhammed Date: Sat, 25 Jul 2026 02:44:02 -0700 Subject: [PATCH] security(p2p): freeze trade/offer fund-routing columns + atomic release (2 verify-found bypasses) + tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bounded adversarial re-verification of the new fund-custody auth found two real bypasses that the fix rounds missed; both fixed here, plus executable proof. - BYPASS #1 (CRITICAL, external): p2p_fiat_trades/p2p_fiat_offers kept USING(true) anon UPDATE, and confirm-payment / admin_resolve_dispute read the fund DESTINATION (buyer_id), amount and token straight from those rows. An anon attacker could `UPDATE p2p_fiat_trades SET buyer_id=` so the honest seller's correctly- signed release paid the attacker. Fix: BEFORE UPDATE triggers freeze the fund-routing columns (offer_id, seller_id, buyer_id, crypto_amount, fiat_amount, price_per_unit, escrow_locked_amount on trades; seller_id, token, amount_crypto on offers) for any non-service-role writer — trigger-level, independent of RLS/grants; status/proof/chat stay open. (Corrects the earlier triage that dismissed these tables as non-custodial.) - BYPASS #2 (double-release TOCTOU): confirm-payment checked status then released non-atomically, so two concurrent differently-nonced valid requests could both release and drain other buyers' escrow. Fix: atomic compare-and-swap of payment_sent->completed BEFORE the escrow move (loser gets 409; revert on release failure). - Hardening: REVOKE EXECUTE on the dead legacy resolve_p2p_dispute mover from PUBLIC/anon/authenticated. - Proof: 34 executable tests (Deno) — identity-auth unit tests (sr25519 sign/verify, challenge money-param binding, citizen/visa ownership, nonce single-use+concurrency, freshness) + per-handler authorization-flow tests (bad-sig/cross-id/wrong-state/replay, CAS double-release). All pass. --- .../__tests__/authorization-flow.test.ts | 275 ++++++++++++++++ .../_shared/__tests__/identity-auth.test.ts | 298 ++++++++++++++++++ .../functions/confirm-payment/index.ts | 51 ++- ...5040000_freeze_trade_financial_columns.sql | 109 +++++++ 4 files changed, 716 insertions(+), 17 deletions(-) create mode 100644 web/supabase/functions/__tests__/authorization-flow.test.ts create mode 100644 web/supabase/functions/_shared/__tests__/identity-auth.test.ts create mode 100644 web/supabase/migrations/20260725040000_freeze_trade_financial_columns.sql diff --git a/web/supabase/functions/__tests__/authorization-flow.test.ts b/web/supabase/functions/__tests__/authorization-flow.test.ts new file mode 100644 index 00000000..919a3941 --- /dev/null +++ b/web/supabase/functions/__tests__/authorization-flow.test.ts @@ -0,0 +1,275 @@ +// Handler-flow authorization tests. +// +// Run: deno test -A web/supabase/functions/__tests__/authorization-flow.test.ts +// +// The edge-function index.ts files call serve() at import time (binding a port +// and constructing a live Supabase client from env), so they cannot be imported +// directly in a unit test. These tests instead re-run each handler's EXACT guard +// sequence against the REAL _shared gate functions + a stateful in-memory Supabase +// mock, proving the composed money-move gates hold: bad signature rejected, a +// verified signer acting on ANOTHER user's id rejected, replayed nonce rejected, +// wrong trade state rejected, the fund DESTINATION is read from the server-side +// trade row, and the confirm-payment compare-and-swap prevents double-release. + +import { assertEquals, assert } from 'https://deno.land/std@0.224.0/assert/mod.ts' +import { Keyring } from 'npm:@pezkuwi/keyring@14.0.25' +import { cryptoWaitReady } from 'npm:@pezkuwi/util-crypto@14.0.25' +import { stringToU8a, u8aToHex, u8aWrapBytes } from 'npm:@pezkuwi/util@14.0.25' + +import { + identityToUUID, + verifyWalletSignature, + buildWithdrawChallenge, + buildConfirmPaymentChallenge, + assertIdentityOwnedByWallet, + consumeNonce, + isFreshTimestamp, +} from '../_shared/identity-auth.ts' + +await cryptoWaitReady() +const kr = new Keyring({ type: 'sr25519' }) +const seller = kr.addFromUri('//Seller') +const attacker = kr.addFromUri('//Attacker') + +// --------------------------------------------------------------------------- +// Stateful in-memory Supabase mock (trades + offers + visa + nonces + rpc) +// --------------------------------------------------------------------------- +interface Trade { + id: string; seller_id: string; buyer_id: string; offer_id: string + crypto_amount: number; status: string +} +interface Offer { id: string; token: string } + +function makeDb(init: { trades?: Trade[]; offers?: Offer[]; visa?: { visa_number: string; wallet_address: string; status: string }[] }) { + const trades = new Map((init.trades ?? []).map(t => [t.id, { ...t }])) + const offers = new Map((init.offers ?? []).map(o => [o.id, o])) + const visa = init.visa ?? [] + const nonces = new Set() + const releaseCalls: any[] = [] + + const client: any = { + releaseCalls, + tradesState: trades, + from(table: string) { + if (table === 'p2p_visa') { + const f: Record = {} + const b: any = { + select() { return b }, eq(c: string, v: string) { f[c] = v; return b }, + async maybeSingle() { + const row = visa.find(r => r.visa_number === f['visa_number'] && r.wallet_address === f['wallet_address'] && r.status === f['status']) + return { data: row ?? null, error: null } + }, + } + return b + } + if (table === 'p2p_challenge_nonces') { + return { async insert(row: { nonce: string }) { if (nonces.has(row.nonce)) return { error: { code: '23505' } }; nonces.add(row.nonce); return { error: null } } } + } + if (table === 'p2p_fiat_trades') { + const f: Record = {} + let updateVals: Record | null = null + const b: any = { + select() { return b }, + update(vals: Record) { updateVals = vals; return b }, + eq(c: string, v: string) { f[c] = v; return b }, + async single() { const t = trades.get(f['id']); return { data: t ?? null, error: t ? null : { message: 'not found' } } }, + // Terminal for the CAS update: .update().eq('id').eq('status').select('id') + then(resolve: (r: any) => void) { + // Only reached for the update+select CAS chain. + const t = trades.get(f['id']) + if (!t || !updateVals) { resolve({ data: [], error: null }); return } + // compare-and-swap: status filter must still match current row. + if (f['status'] !== undefined && t.status !== f['status']) { resolve({ data: [], error: null }); return } + Object.assign(t, updateVals) + resolve({ data: [{ id: t.id }], error: null }) + }, + } + return b + } + if (table === 'p2p_fiat_offers') { + const f: Record = {} + const b: any = { select() { return b }, eq(c: string, v: string) { f[c] = v; return b }, async single() { const o = offers.get(f['id']); return { data: o ?? null, error: o ? null : { message: 'nf' } } } } + return b + } + throw new Error('unexpected table ' + table) + }, + async rpc(fn: string, args: any) { + if (fn === 'release_escrow_internal') { + releaseCalls.push(args) + return { data: { success: true }, error: null } + } + throw new Error('unexpected rpc ' + fn) + }, + } + return client +} + +const getPeopleApiStub = async () => ({ query: { tiki: { citizenNft: async () => ({ isEmpty: true }) } } }) as any + +// --------------------------------------------------------------------------- +// Faithful re-implementation of confirm-payment/index.ts guard sequence +// (kept in lockstep with the edge function, including the CAS fix). +// --------------------------------------------------------------------------- +async function runConfirmPayment(db: any, body: any): Promise<{ status: number; body: any }> { + const J = (status: number, obj: any) => ({ status, body: obj }) + const { tradeId, sellerIdentity, signerAddress, signature, timestamp, nonce } = body + if (!tradeId || !sellerIdentity || !signerAddress || !signature || !nonce || typeof timestamp !== 'number') + return J(401, { success: false, error: 'Missing authorization fields' }) + if (!isFreshTimestamp(timestamp)) return J(401, { success: false, error: 'expired' }) + const challenge = buildConfirmPaymentChallenge({ tradeId, sellerIdentity, signerAddress, timestamp, nonce }) + if (!(await verifyWalletSignature(challenge, signature, signerAddress))) return J(401, { success: false, error: 'Invalid signature' }) + const ownership = await assertIdentityOwnedByWallet(db, sellerIdentity, signerAddress, getPeopleApiStub) + if (!ownership.ok) return J(403, { success: false, error: ownership.error }) + const sellerUserId = await identityToUUID(sellerIdentity) + const { data: trade, error: tradeErr } = await db.from('p2p_fiat_trades').select('*').eq('id', tradeId).single() + if (tradeErr || !trade) return J(404, { success: false, error: 'Trade not found' }) + if (trade.seller_id !== sellerUserId) return J(403, { success: false, error: 'Only the trade seller can release this escrow' }) + if (trade.status !== 'payment_sent') return J(400, { success: false, error: 'not awaiting confirmation' }) + const { data: offer } = await db.from('p2p_fiat_offers').select('token').eq('id', trade.offer_id).single() + if (!offer?.token) return J(400, { success: false, error: 'Offer/token not found' }) + if (!(await consumeNonce(db, nonce, 'confirm_payment'))) return J(401, { success: false, error: 'replay' }) + const nowIso = new Date().toISOString() + const { data: claimed } = await db.from('p2p_fiat_trades') + .update({ status: 'completed', seller_confirmed_at: nowIso, escrow_released_at: nowIso, completed_at: nowIso }) + .eq('id', tradeId).eq('status', 'payment_sent').select('id') + if (!claimed || claimed.length === 0) return J(409, { success: false, error: 'already released' }) + const { data: releaseRes } = await db.rpc('release_escrow_internal', { + p_from_user_id: trade.seller_id, p_to_user_id: trade.buyer_id, p_token: offer.token, + p_amount: trade.crypto_amount, p_reference_type: 'trade', p_reference_id: tradeId, + }) + if (!releaseRes?.success) return J(400, { success: false, error: 'Release failed' }) + return J(200, { success: true, sellerId: trade.seller_id, buyerId: trade.buyer_id, amount: trade.crypto_amount }) +} + +async function setupTrade() { + const sellerId = await identityToUUID('V-100001') + const buyerId = await identityToUUID('V-200002') + const db = makeDb({ + trades: [{ id: 'T1', seller_id: sellerId, buyer_id: buyerId, offer_id: 'O1', crypto_amount: 50, status: 'payment_sent' }], + offers: [{ id: 'O1', token: 'HEZ' }], + visa: [{ visa_number: 'V-100001', wallet_address: seller.address, status: 'active' }], + }) + return { db, sellerId, buyerId } +} + +function sign(pair: any, msg: string) { return u8aToHex(pair.sign(u8aWrapBytes(msg))) } + +// --------------------------------------------------------------------------- +// confirm-payment (escrow RELEASE) tests +// --------------------------------------------------------------------------- +Deno.test('confirm-payment: missing signature -> 401', async () => { + const { db } = await setupTrade() + const r = await runConfirmPayment(db, { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, timestamp: Date.now(), nonce: 'cp-11112222' }) + assertEquals(r.status, 401) +}) + +Deno.test('confirm-payment: invalid signature -> 401 (no release)', async () => { + const { db } = await setupTrade() + const r = await runConfirmPayment(db, { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, signature: '0xdeadbeef', timestamp: Date.now(), nonce: 'cp-11112222' }) + assertEquals(r.status, 401) + assertEquals(db.releaseCalls.length, 0) +}) + +Deno.test('confirm-payment: valid signer who does NOT own the seller identity -> 403 (no release)', async () => { + const { db } = await setupTrade() + const ts = Date.now(), nonce = 'cp-attacker1' + // Attacker signs a well-formed challenge for the seller identity, with a valid + // signature over it — but the attacker wallet does not own visa V-100001. + const challenge = buildConfirmPaymentChallenge({ tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: attacker.address, timestamp: ts, nonce }) + const r = await runConfirmPayment(db, { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: attacker.address, signature: sign(attacker, challenge), timestamp: ts, nonce }) + assertEquals(r.status, 403) + assertEquals(db.releaseCalls.length, 0) +}) + +Deno.test('confirm-payment: verified seller BUT trade not in payment_sent -> 400 (no release)', async () => { + const { db } = await setupTrade() + db.tradesState.get('T1').status = 'pending' + const ts = Date.now(), nonce = 'cp-state001' + const challenge = buildConfirmPaymentChallenge({ tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, timestamp: ts, nonce }) + const r = await runConfirmPayment(db, { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, signature: sign(seller, challenge), timestamp: ts, nonce }) + assertEquals(r.status, 400) + assertEquals(db.releaseCalls.length, 0) +}) + +Deno.test('confirm-payment: happy path releases to the trade-row buyer_id (destination from server row)', async () => { + const { db, sellerId, buyerId } = await setupTrade() + const ts = Date.now(), nonce = 'cp-happy001' + const challenge = buildConfirmPaymentChallenge({ tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, timestamp: ts, nonce }) + const r = await runConfirmPayment(db, { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, signature: sign(seller, challenge), timestamp: ts, nonce }) + assertEquals(r.status, 200) + assertEquals(db.releaseCalls.length, 1) + assertEquals(db.releaseCalls[0].p_from_user_id, sellerId) + assertEquals(db.releaseCalls[0].p_to_user_id, buyerId) // funds go to the row's buyer_id + assertEquals(db.releaseCalls[0].p_amount, 50) +}) + +Deno.test('confirm-payment: replayed nonce after success -> 401 (no second release)', async () => { + const { db } = await setupTrade() + const ts = Date.now(), nonce = 'cp-replay01' + const challenge = buildConfirmPaymentChallenge({ tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, timestamp: ts, nonce }) + const body = { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, signature: sign(seller, challenge), timestamp: ts, nonce } + const r1 = await runConfirmPayment(db, body) + assertEquals(r1.status, 200) + // Reset status so ONLY the nonce guard can stop the replay (proving replay defense). + db.tradesState.get('T1').status = 'payment_sent' + const r2 = await runConfirmPayment(db, body) + assertEquals(r2.status, 401) + assertEquals(db.releaseCalls.length, 1) +}) + +Deno.test('confirm-payment: concurrent double-release (two nonces) -> only ONE release via CAS', async () => { + const { db } = await setupTrade() + const ts = Date.now() + const mk = (nonce: string) => { + const c = buildConfirmPaymentChallenge({ tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, timestamp: ts, nonce }) + return { tradeId: 'T1', sellerIdentity: 'V-100001', signerAddress: seller.address, signature: sign(seller, c), timestamp: ts, nonce } + } + const [a, b] = await Promise.all([runConfirmPayment(db, mk('cp-conc0001')), runConfirmPayment(db, mk('cp-conc0002'))]) + const oks = [a, b].filter(x => x.status === 200) + assertEquals(oks.length, 1) // exactly one succeeds + assertEquals(db.releaseCalls.length, 1) // escrow moved exactly once +}) + +// --------------------------------------------------------------------------- +// process-withdraw auth-gate mirror (server derives user_id; client value ignored) +// --------------------------------------------------------------------------- +async function runWithdrawAuth(db: any, body: any): Promise<{ status: number; userId?: string }> { + const { identityId, signerAddress, signature, timestamp, nonce, token, amount, walletAddress } = body + if (!identityId || !signerAddress || !signature || !nonce || typeof timestamp !== 'number') return { status: 401 } + if (!isFreshTimestamp(timestamp)) return { status: 401 } + const challenge = buildWithdrawChallenge({ identityId, token: String(token ?? ''), amount: Number(amount ?? 0), destination: String(walletAddress ?? ''), signerAddress, timestamp, nonce }) + if (!(await verifyWalletSignature(challenge, signature, signerAddress))) return { status: 401 } + const ownership = await assertIdentityOwnedByWallet(db, identityId, signerAddress, getPeopleApiStub) + if (!ownership.ok) return { status: 403 } + if (!(await consumeNonce(db, nonce, 'withdraw'))) return { status: 401 } + const userId = await identityToUUID(identityId) // server-derived; body.user_id ignored + return { status: 200, userId } +} + +Deno.test('process-withdraw: signer acting on ANOTHER identity is rejected (ownership) ', async () => { + const db = makeDb({ visa: [{ visa_number: 'V-100001', wallet_address: seller.address, status: 'active' }] }) + const ts = Date.now(), nonce = 'wd-otheruser' + // Attacker owns nothing; claims victim identity V-100001. + const challenge = buildWithdrawChallenge({ identityId: 'V-100001', token: 'HEZ', amount: 5, destination: '5EVIL', signerAddress: attacker.address, timestamp: ts, nonce }) + const r = await runWithdrawAuth(db, { identityId: 'V-100001', token: 'HEZ', amount: 5, walletAddress: '5EVIL', signerAddress: attacker.address, signature: u8aToHex(attacker.sign(u8aWrapBytes(challenge))), timestamp: ts, nonce }) + assertEquals(r.status, 403) +}) + +Deno.test('process-withdraw: valid owner passes and user_id is derived from identity (client user_id irrelevant)', async () => { + const db = makeDb({ visa: [{ visa_number: 'V-100001', wallet_address: seller.address, status: 'active' }] }) + const ts = Date.now(), nonce = 'wd-gooduser' + const challenge = buildWithdrawChallenge({ identityId: 'V-100001', token: 'HEZ', amount: 5, destination: '5DEST', signerAddress: seller.address, timestamp: ts, nonce }) + const r = await runWithdrawAuth(db, { identityId: 'V-100001', token: 'HEZ', amount: 5, walletAddress: '5DEST', signerAddress: seller.address, signature: u8aToHex(seller.sign(u8aWrapBytes(challenge))), timestamp: ts, nonce, user_id: 'client-supplied-garbage' }) + assertEquals(r.status, 200) + assertEquals(r.userId, await identityToUUID('V-100001')) +}) + +Deno.test('process-withdraw: signature bound to amount+destination — tampering either invalidates it', async () => { + const db = makeDb({ visa: [{ visa_number: 'V-100001', wallet_address: seller.address, status: 'active' }] }) + const ts = Date.now() + const signed = buildWithdrawChallenge({ identityId: 'V-100001', token: 'HEZ', amount: 5, destination: '5DEST', signerAddress: seller.address, timestamp: ts, nonce: 'wd-tamper01' }) + const sig = u8aToHex(seller.sign(u8aWrapBytes(signed))) + // Attacker keeps the signature but bumps amount + redirects destination. + const r = await runWithdrawAuth(db, { identityId: 'V-100001', token: 'HEZ', amount: 999999, walletAddress: '5EVIL', signerAddress: seller.address, signature: sig, timestamp: ts, nonce: 'wd-tamper01' }) + assertEquals(r.status, 401) +}) diff --git a/web/supabase/functions/_shared/__tests__/identity-auth.test.ts b/web/supabase/functions/_shared/__tests__/identity-auth.test.ts new file mode 100644 index 00000000..ffab3d97 --- /dev/null +++ b/web/supabase/functions/_shared/__tests__/identity-auth.test.ts @@ -0,0 +1,298 @@ +// Unit tests for the fund-custody authorization boundary (_shared/identity-auth.ts). +// +// Run: deno test -A web/supabase/functions/_shared/__tests__/identity-auth.test.ts +// +// These exercise the REAL module (real sr25519 sign/verify via @pezkuwi/*), with +// the Supabase client and People-Chain API mocked. They prove the gates that the +// edge functions compose: signature verification, canonical money-bound +// challenges, identity-ownership, single-use nonces and timestamp freshness. + +import { + assertEquals, + assert, + assertNotEquals, +} from 'https://deno.land/std@0.224.0/assert/mod.ts' +import { Keyring } from 'npm:@pezkuwi/keyring@14.0.25' +import { cryptoWaitReady, encodeAddress } from 'npm:@pezkuwi/util-crypto@14.0.25' +import { stringToU8a, u8aToHex, u8aWrapBytes } from 'npm:@pezkuwi/util@14.0.25' + +import { + verifyWalletSignature, + buildWithdrawChallenge, + buildConfirmPaymentChallenge, + buildLockEscrowChallenge, + buildDepositChallenge, + buildAdminChallenge, + assertIdentityOwnedByWallet, + consumeNonce, + isFreshTimestamp, + generateCitizenNumber, + identityToUUID, + CHALLENGE_MAX_AGE_MS, +} from '../identity-auth.ts' + +// --------------------------------------------------------------------------- +// Test key material +// --------------------------------------------------------------------------- +await cryptoWaitReady() +const kr = new Keyring({ type: 'sr25519' }) +const alice = kr.addFromUri('//Alice') +const bob = kr.addFromUri('//Bob') + +// --------------------------------------------------------------------------- +// verifyWalletSignature +// --------------------------------------------------------------------------- +Deno.test('verifyWalletSignature: valid raw signature passes', async () => { + const msg = 'Pezkuwi P2P Withdrawal\ntoken:HEZ' + const sig = u8aToHex(alice.sign(stringToU8a(msg))) + assert(await verifyWalletSignature(msg, sig, alice.address)) +}) + +Deno.test('verifyWalletSignature: -wrapped signature passes (extension signRaw form)', async () => { + const msg = 'Pezkuwi P2P Confirm Payment\ntrade:abc' + const sig = u8aToHex(alice.sign(u8aWrapBytes(msg))) + assert(await verifyWalletSignature(msg, sig, alice.address)) +}) + +Deno.test('verifyWalletSignature: wrong signer is rejected', async () => { + const msg = 'authorize me' + const sig = u8aToHex(alice.sign(stringToU8a(msg))) // signed by Alice + assertEquals(await verifyWalletSignature(msg, sig, bob.address), false) +}) + +Deno.test('verifyWalletSignature: signature over a DIFFERENT message is rejected (no malleability/reuse)', async () => { + const sig = u8aToHex(alice.sign(stringToU8a('message-A'))) + assertEquals(await verifyWalletSignature('message-B', sig, alice.address), false) +}) + +Deno.test('verifyWalletSignature: empty / malformed signatures return false (no throw)', async () => { + assertEquals(await verifyWalletSignature('m', '', alice.address), false) + assertEquals(await verifyWalletSignature('m', '0x', alice.address), false) + assertEquals(await verifyWalletSignature('m', 'not-hex', alice.address), false) + assertEquals(await verifyWalletSignature('m', '0xdeadbeef', alice.address), false) +}) + +// --------------------------------------------------------------------------- +// Canonical challenge builders — money-param binding +// --------------------------------------------------------------------------- +Deno.test('buildWithdrawChallenge: canonical shape + binds token/amount/destination/nonce', () => { + const base = { identityId: 'V-100001', token: 'HEZ', amount: 5, destination: '5DEST', signerAddress: '5SIGN', timestamp: 111, nonce: 'n-1234abcd' } + const c = buildWithdrawChallenge(base) + assertEquals(c.split('\n')[0], 'Pezkuwi P2P Withdrawal') + assert(c.includes('token:HEZ')) + assert(c.includes('amount:5')) + assert(c.includes('destination:5DEST')) + // Any change to a money field changes the signed bytes: + assertNotEquals(c, buildWithdrawChallenge({ ...base, token: 'PEZ' })) + assertNotEquals(c, buildWithdrawChallenge({ ...base, amount: 6 })) + assertNotEquals(c, buildWithdrawChallenge({ ...base, destination: '5EVIL' })) + assertNotEquals(c, buildWithdrawChallenge({ ...base, nonce: 'n-otherabcd' })) +}) + +Deno.test('buildConfirmPaymentChallenge: binds tradeId + seller identity', () => { + const base = { tradeId: 'T1', sellerIdentity: 'V-1', signerAddress: '5S', timestamp: 1, nonce: 'nnnnnnnn' } + const c = buildConfirmPaymentChallenge(base) + assertEquals(c.split('\n')[0], 'Pezkuwi P2P Confirm Payment') + assertNotEquals(c, buildConfirmPaymentChallenge({ ...base, tradeId: 'T2' })) + assertNotEquals(c, buildConfirmPaymentChallenge({ ...base, sellerIdentity: 'V-2' })) +}) + +Deno.test('buildLockEscrowChallenge: binds token + amount', () => { + const base = { identityId: 'V-1', token: 'HEZ', amount: 10, signerAddress: '5S', timestamp: 1, nonce: 'nnnnnnnn' } + const c = buildLockEscrowChallenge(base) + assertEquals(c.split('\n')[0], 'Pezkuwi P2P Lock Escrow') + assertNotEquals(c, buildLockEscrowChallenge({ ...base, amount: 11 })) + assertNotEquals(c, buildLockEscrowChallenge({ ...base, token: 'PEZ' })) +}) + +Deno.test('challenge prefixes are distinct — a signature for one action cannot satisfy another', () => { + const t = 1, n = 'nnnnnnnn', s = '5S' + const prefixes = new Set([ + buildWithdrawChallenge({ identityId: 'i', token: 'HEZ', amount: 1, destination: 'd', signerAddress: s, timestamp: t, nonce: n }).split('\n')[0], + buildConfirmPaymentChallenge({ tradeId: 'x', sellerIdentity: 'i', signerAddress: s, timestamp: t, nonce: n }).split('\n')[0], + buildLockEscrowChallenge({ identityId: 'i', token: 'HEZ', amount: 1, signerAddress: s, timestamp: t, nonce: n }).split('\n')[0], + buildDepositChallenge({ identityId: 'i', token: 'HEZ', txHash: '0x', signerAddress: s, timestamp: t, nonce: n }).split('\n')[0], + buildAdminChallenge({ action: 'resolve', disputeId: 'd', tradeId: 'x', decision: 'split', adminAddress: s, timestamp: t, nonce: n }).split('\n')[0], + ]) + assertEquals(prefixes.size, 5) // all unique +}) + +Deno.test('end-to-end: a withdraw signature does NOT verify against a tampered amount challenge', async () => { + const p = { identityId: 'V-1', token: 'HEZ', amount: 5, destination: '5DEST', signerAddress: alice.address, timestamp: Date.now(), nonce: 'wd-abcdefgh' } + const sig = u8aToHex(alice.sign(u8aWrapBytes(buildWithdrawChallenge(p)))) + // Attacker replays the signature but bumps the amount -> server rebuilds a + // different challenge -> verification fails. + const tampered = buildWithdrawChallenge({ ...p, amount: 5000 }) + assertEquals(await verifyWalletSignature(tampered, sig, alice.address), false) + // sanity: the untampered challenge does verify + assert(await verifyWalletSignature(buildWithdrawChallenge(p), sig, alice.address)) +}) + +// --------------------------------------------------------------------------- +// identityToUUID / generateCitizenNumber +// --------------------------------------------------------------------------- +Deno.test('identityToUUID: deterministic + distinct per identity', async () => { + const a = await identityToUUID('V-100001') + const b = await identityToUUID('V-100001') + const c = await identityToUUID('V-100002') + assertEquals(a, b) + assertNotEquals(a, c) + assert(/^[0-9a-f]{8}-[0-9a-f]{4}-5[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/.test(a), 'valid v5 uuid shape') +}) + +Deno.test('generateCitizenNumber: deterministic 6-digit tied to (address, collection, item)', () => { + const n = generateCitizenNumber(alice.address, 42, 7) + assertEquals(n, generateCitizenNumber(alice.address, 42, 7)) + assertEquals(n.length, 6) + // Different owner or item generally yields a different number. + assertNotEquals(generateCitizenNumber(alice.address, 42, 7), generateCitizenNumber(bob.address, 42, 7)) +}) + +// --------------------------------------------------------------------------- +// Mock Supabase client +// --------------------------------------------------------------------------- +type VisaRow = { visa_number: string; wallet_address: string; status: string } + +function makeSupabaseMock(opts: { visaRows?: VisaRow[]; nonceStore?: Set } = {}) { + const visaRows = opts.visaRows ?? [] + const nonceStore = opts.nonceStore ?? new Set() + return { + nonceStore, + from(table: string) { + if (table === 'p2p_visa') { + const filters: Record = {} + const builder: any = { + select() { return builder }, + eq(col: string, val: string) { filters[col] = val; return builder }, + async maybeSingle() { + const row = visaRows.find(r => + r.visa_number === filters['visa_number'] && + r.wallet_address === filters['wallet_address'] && + r.status === filters['status']) + return { data: row ?? null, error: null } + }, + } + return builder + } + if (table === 'p2p_challenge_nonces') { + return { + async insert(row: { nonce: string; purpose: string }) { + if (nonceStore.has(row.nonce)) { + return { error: { code: '23505', message: 'duplicate key value violates unique constraint' } } + } + nonceStore.add(row.nonce) + return { error: null } + }, + } + } + throw new Error(`unexpected table ${table}`) + }, + } as any +} + +// People-chain API mock. `ownedItem === null` => wallet owns no citizen NFT. +function makePeopleApi(ownedItem: number | null) { + const res = ownedItem === null + ? { isEmpty: true } + : { isEmpty: false, isSome: true, unwrap: () => ({ toNumber: () => ownedItem }) } + return async () => ({ query: { tiki: { citizenNft: async (_addr: string) => res } } }) as any +} + +// --------------------------------------------------------------------------- +// assertIdentityOwnedByWallet — visa +// --------------------------------------------------------------------------- +Deno.test('ownership(visa): active visa bound to signer -> ok', async () => { + const sb = makeSupabaseMock({ visaRows: [{ visa_number: 'V-100001', wallet_address: alice.address, status: 'active' }] }) + const r = await assertIdentityOwnedByWallet(sb, 'V-100001', alice.address, makePeopleApi(null)) + assertEquals(r.ok, true) + assertEquals(r.kind, 'visa') +}) + +Deno.test('ownership(visa): signer B cannot act on identity A (visa bound to A) -> rejected', async () => { + const sb = makeSupabaseMock({ visaRows: [{ visa_number: 'V-100001', wallet_address: alice.address, status: 'active' }] }) + const r = await assertIdentityOwnedByWallet(sb, 'V-100001', bob.address, makePeopleApi(null)) + assertEquals(r.ok, false) +}) + +Deno.test('ownership(visa): inactive visa -> rejected', async () => { + const sb = makeSupabaseMock({ visaRows: [{ visa_number: 'V-100001', wallet_address: alice.address, status: 'revoked' }] }) + const r = await assertIdentityOwnedByWallet(sb, 'V-100001', alice.address, makePeopleApi(null)) + assertEquals(r.ok, false) +}) + +// --------------------------------------------------------------------------- +// assertIdentityOwnedByWallet — citizen +// --------------------------------------------------------------------------- +Deno.test('ownership(citizen): chain owns item + derivation matches -> ok', async () => { + const item = 7 + const six = generateCitizenNumber(alice.address, 42, item) + const sb = makeSupabaseMock() + const r = await assertIdentityOwnedByWallet(sb, `#42-${item}-${six}`, alice.address, makePeopleApi(item)) + assertEquals(r.ok, true) + assertEquals(r.kind, 'citizen') +}) + +Deno.test('ownership(citizen): claimed item != chain-owned item -> rejected (no NFT theft)', async () => { + const claimedItem = 7 + const six = generateCitizenNumber(alice.address, 42, claimedItem) + const sb = makeSupabaseMock() + // Chain says alice actually owns item 9, not the claimed 7. + const r = await assertIdentityOwnedByWallet(sb, `#42-${claimedItem}-${six}`, alice.address, makePeopleApi(9)) + assertEquals(r.ok, false) +}) + +Deno.test('ownership(citizen): correct item but forged 6-digit -> rejected', async () => { + const item = 7 + const sb = makeSupabaseMock() + const r = await assertIdentityOwnedByWallet(sb, `#42-${item}-000000`, alice.address, makePeopleApi(item)) + assertEquals(r.ok, false) +}) + +Deno.test('ownership(citizen): wallet owns no NFT -> rejected', async () => { + const item = 7 + const six = generateCitizenNumber(alice.address, 42, item) + const sb = makeSupabaseMock() + const r = await assertIdentityOwnedByWallet(sb, `#42-${item}-${six}`, alice.address, makePeopleApi(null)) + assertEquals(r.ok, false) +}) + +Deno.test('ownership: malformed identity / wrong collection -> rejected', async () => { + const sb = makeSupabaseMock() + assertEquals((await assertIdentityOwnedByWallet(sb, 'garbage', alice.address, makePeopleApi(1))).ok, false) + assertEquals((await assertIdentityOwnedByWallet(sb, '#1-2-345678', alice.address, makePeopleApi(2))).ok, false) // collection != 42 + assertEquals((await assertIdentityOwnedByWallet(sb, '', alice.address, makePeopleApi(1))).ok, false) +}) + +// --------------------------------------------------------------------------- +// consumeNonce — single use / replay +// --------------------------------------------------------------------------- +Deno.test('consumeNonce: first use ok, second use (replay) rejected', async () => { + const sb = makeSupabaseMock() + assertEquals(await consumeNonce(sb, 'wd-11112222', 'withdraw'), true) + assertEquals(await consumeNonce(sb, 'wd-11112222', 'withdraw'), false) // replay +}) + +Deno.test('consumeNonce: concurrent double-spend of one nonce yields exactly one success', async () => { + const sb = makeSupabaseMock() + const [a, b] = await Promise.all([ + consumeNonce(sb, 'wd-race0001', 'withdraw'), + consumeNonce(sb, 'wd-race0001', 'withdraw'), + ]) + assertEquals([a, b].filter(Boolean).length, 1) +}) + +Deno.test('consumeNonce: too-short nonce rejected', async () => { + const sb = makeSupabaseMock() + assertEquals(await consumeNonce(sb, 'short', 'withdraw'), false) +}) + +// --------------------------------------------------------------------------- +// isFreshTimestamp +// --------------------------------------------------------------------------- +Deno.test('isFreshTimestamp: now is fresh; expired and non-finite rejected', () => { + assert(isFreshTimestamp(Date.now())) + assertEquals(isFreshTimestamp(Date.now() - CHALLENGE_MAX_AGE_MS - 5000), false) + assertEquals(isFreshTimestamp(Date.now() + 120_000), false) // beyond 60s skew + assertEquals(isFreshTimestamp(NaN), false) + assertEquals(isFreshTimestamp(Infinity), false) +}) diff --git a/web/supabase/functions/confirm-payment/index.ts b/web/supabase/functions/confirm-payment/index.ts index 05c76616..edc7c3e6 100644 --- a/web/supabase/functions/confirm-payment/index.ts +++ b/web/supabase/functions/confirm-payment/index.ts @@ -116,21 +116,13 @@ serve(async (req) => { return json(401, { success: false, error: 'Authorization already used (replay detected)' }) } - // 6) Release escrow (service role) + mark trade completed - const { data: releaseRes, error: releaseErr } = await serviceClient.rpc('release_escrow_internal', { - p_from_user_id: trade.seller_id, - p_to_user_id: trade.buyer_id, - p_token: offer.token, - p_amount: trade.crypto_amount, - p_reference_type: 'trade', - p_reference_id: tradeId, - }) - if (releaseErr) return json(500, { success: false, error: releaseErr.message || 'Release failed' }) - const parsed = typeof releaseRes === 'string' ? JSON.parse(releaseRes) : releaseRes - if (!parsed?.success) return json(400, { success: false, error: parsed?.error || 'Release failed' }) - + // 6) Atomically CLAIM the payment_sent -> completed transition BEFORE moving + // any funds. This is the concurrency lock: only one caller can flip the + // row out of 'payment_sent', so two concurrent (differently-nonced) release + // requests for the same trade cannot both reach release_escrow_internal and + // double-spend escrow that was locked for other trades on the same offer. const nowIso = new Date().toISOString() - const { error: updErr } = await serviceClient + const { data: claimed, error: claimErr } = await serviceClient .from('p2p_fiat_trades') .update({ seller_confirmed_at: nowIso, @@ -139,9 +131,34 @@ serve(async (req) => { completed_at: nowIso, }) .eq('id', tradeId) - if (updErr) { - // Funds released but status update failed — log for reconciliation. - console.error('Trade status update failed after release:', updErr) + .eq('status', 'payment_sent') // compare-and-swap guard + .select('id') + if (claimErr) return json(500, { success: false, error: 'Failed to claim trade for release' }) + if (!claimed || claimed.length === 0) { + // Someone (or a concurrent request) already moved it out of payment_sent. + return json(409, { success: false, error: 'Trade is no longer awaiting confirmation (already released?)' }) + } + + // 7) Release escrow (service role). If it fails, revert the claim so the trade + // can be retried / disputed rather than being stuck "completed" with no move. + const { data: releaseRes, error: releaseErr } = await serviceClient.rpc('release_escrow_internal', { + p_from_user_id: trade.seller_id, + p_to_user_id: trade.buyer_id, + p_token: offer.token, + p_amount: trade.crypto_amount, + p_reference_type: 'trade', + p_reference_id: tradeId, + }) + const parsed = typeof releaseRes === 'string' ? JSON.parse(releaseRes) : releaseRes + if (releaseErr || !parsed?.success) { + await serviceClient + .from('p2p_fiat_trades') + .update({ status: 'payment_sent', seller_confirmed_at: null, escrow_released_at: null, completed_at: null }) + .eq('id', tradeId) + return json(releaseErr ? 500 : 400, { + success: false, + error: releaseErr?.message || parsed?.error || 'Release failed', + }) } return json(200, { diff --git a/web/supabase/migrations/20260725040000_freeze_trade_financial_columns.sql b/web/supabase/migrations/20260725040000_freeze_trade_financial_columns.sql new file mode 100644 index 00000000..3802755a --- /dev/null +++ b/web/supabase/migrations/20260725040000_freeze_trade_financial_columns.sql @@ -0,0 +1,109 @@ +-- ===================================================== +-- Migration: Freeze fund-routing columns on trades/offers +-- Date: 2026-07-25 +-- ===================================================== +-- +-- Fix for a CONFIRMED custody-critical escrow-redirection bypass. +-- +-- p2p_fiat_trades still carries a world-open UPDATE policy +-- (trades_anon_update USING(true), migration 20260223160000) because the client +-- legitimately flips status / uploads payment proof with the anon key, and there +-- is no server session to bind row ownership to. The 20260725010000 hardening +-- migration knowingly left this, on the assumption that "marking a trade status +-- does not itself move funds". +-- +-- That assumption is INCOMPLETE. The seller-authorized release path +-- (confirm-payment edge function) and admin_resolve_dispute() read the fund +-- DESTINATION and AMOUNT from the trade row at release time: +-- release_escrow_internal(trade.seller_id, trade.buyer_id, token, trade.crypto_amount) +-- Because buyer_id / crypto_amount / offer_id are attacker-writable via the anon +-- key, an external attacker can do +-- UPDATE p2p_fiat_trades SET buyer_id = WHERE id = +-- and, when the honest seller signs the (correctly authenticated) release, the +-- escrow is paid to the attacker instead of the buyer who sent fiat. The +-- signature gate is intact but authorizes a row whose routing was tampered. +-- +-- Minimal fix (no app-breaking change, no signed-session refactor required): +-- make the fund-routing / amount columns IMMUTABLE after insert via a BEFORE +-- UPDATE trigger. No legitimate code path ever mutates these columns after the +-- trade/offer is created (accept_p2p_offer INSERTs them; every UPDATE — status, +-- proof, timestamps, dispute fields, remaining_amount, price edits — leaves them +-- untouched). The service role is still allowed to change them so future +-- backend tooling / manual reconciliation is not boxed in. + +-- ----------------------------------------------------- +-- 1. p2p_fiat_trades: freeze routing + amount columns +-- ----------------------------------------------------- +CREATE OR REPLACE FUNCTION public.enforce_trade_financial_immutability() +RETURNS TRIGGER AS $$ +BEGIN + -- Allow the backend service role to change anything (reconciliation). + IF current_setting('role', true) = 'service_role' + OR current_setting('request.jwt.claim.role', true) = 'service_role' THEN + RETURN NEW; + END IF; + + IF NEW.offer_id IS DISTINCT FROM OLD.offer_id + OR NEW.seller_id IS DISTINCT FROM OLD.seller_id + OR NEW.buyer_id IS DISTINCT FROM OLD.buyer_id + OR NEW.crypto_amount IS DISTINCT FROM OLD.crypto_amount + OR NEW.fiat_amount IS DISTINCT FROM OLD.fiat_amount + OR NEW.price_per_unit IS DISTINCT FROM OLD.price_per_unit + OR NEW.escrow_locked_amount IS DISTINCT FROM OLD.escrow_locked_amount THEN + RAISE EXCEPTION 'Trade fund-routing columns are immutable (offer_id, seller_id, buyer_id, crypto_amount, fiat_amount, price_per_unit, escrow_locked_amount)'; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS trg_freeze_trade_financials ON public.p2p_fiat_trades; +CREATE TRIGGER trg_freeze_trade_financials + BEFORE UPDATE ON public.p2p_fiat_trades + FOR EACH ROW EXECUTE FUNCTION public.enforce_trade_financial_immutability(); + +-- ----------------------------------------------------- +-- 2. p2p_fiat_offers: freeze token / seller / original amount +-- (token drives WHICH balance release_escrow_internal moves; seller_id is the +-- escrow owner). Status, remaining_amount, price and fiat fields stay editable +-- so the existing offer edit / pause / partial-fill flows keep working. +-- ----------------------------------------------------- +CREATE OR REPLACE FUNCTION public.enforce_offer_financial_immutability() +RETURNS TRIGGER AS $$ +BEGIN + IF current_setting('role', true) = 'service_role' + OR current_setting('request.jwt.claim.role', true) = 'service_role' THEN + RETURN NEW; + END IF; + + IF NEW.seller_id IS DISTINCT FROM OLD.seller_id + OR NEW.token IS DISTINCT FROM OLD.token + OR NEW.amount_crypto IS DISTINCT FROM OLD.amount_crypto THEN + RAISE EXCEPTION 'Offer fund-routing columns are immutable (seller_id, token, amount_crypto)'; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS trg_freeze_offer_financials ON public.p2p_fiat_offers; +CREATE TRIGGER trg_freeze_offer_financials + BEFORE UPDATE ON public.p2p_fiat_offers + FOR EACH ROW EXECUTE FUNCTION public.enforce_offer_financial_immutability(); + +COMMENT ON FUNCTION public.enforce_trade_financial_immutability IS +'Blocks anon/authenticated UPDATEs from changing a trade''s fund-routing/amount +columns (escrow redirection defense). Service role is exempt.'; +COMMENT ON FUNCTION public.enforce_offer_financial_immutability IS +'Blocks anon/authenticated UPDATEs from changing an offer''s seller/token/amount +(escrow token mis-routing defense). Service role is exempt.'; + +-- --------------------------------------------------------------------------- +-- 3. Remove the dead legacy escrow-mover from the anon attack surface. +-- resolve_p2p_dispute() is SECURITY DEFINER and moves escrow, but its +-- auth.uid() admin gate is NULL under the public anon key, so it already +-- fails closed for anon. Revoking PUBLIC/anon/authenticated EXECUTE removes +-- it as an attack surface entirely (dispute resolution now flows through the +-- wallet-signed resolve-dispute edge function + admin_resolve_dispute). +-- --------------------------------------------------------------------------- +REVOKE EXECUTE ON FUNCTION resolve_p2p_dispute FROM PUBLIC, anon, authenticated;