security(p2p): fix withdrawal BOLA, financial RLS exposure, admin dispute escrow

Critical/high audit remediation on the custodial P2P ledger. Auth model is wallet-based; identity (citizen/visa) is cryptographically bound to a wallet (People Chain tiki.citizenNft / active p2p_visa), which enables a correct fix. - Withdrawal BOLA (CRITICAL): process-withdraw now requires a wallet-SIGNED challenge; server verifies signature (raw + <Bytes> forms), asserts the signer OWNS the identity, consumes a single-use nonce (replay), and derives user_id server-side — the client-supplied user_id is ignored. process-withdrawal (batch) now requires the service-role key (was: any bearer, incl. public anon key). request_withdraw is REVOKEd from anon/authenticated + service-role guarded. - Financial RLS (HIGH): drop the blanket USING(true) SELECT on user_internal_balances, p2p_balance_transactions, p2p_deposit_withdraw_requests; lock p2p_user_payment_methods (IBAN PII) + p2p_fiat_disputes UPDATE to service_role; legitimate reads move behind scoped SECURITY DEFINER RPCs. - Deposit integrity: verify-deposit now binds the on-chain sender to identity ownership before crediting. - Admin dispute (CRITICAL fund-logic): DisputeResolutionPanel relabeled trades without moving escrow. New admin-signed resolve-dispute function + admin_resolve_dispute RPC moves escrow (release/refund/split) atomically with correct accounting (avoids the double-count in the legacy resolve_p2p_dispute). Client isAdmin documented as cosmetic. DEPLOY RUNBOOK (gated; owner runs): 1) apply migrations 20260225/20260725* in order; 2) deploy edge functions process-withdraw, process-withdrawal, verify-deposit, resolve-dispute; 3) set edge secrets PEOPLE_RPC_ENDPOINT (+ optional ADMIN_WALLETS); 4) ship frontend. Migrations + functions + frontend must go together or the app breaks. KNOWN RESIDUAL (Round 2 — as severe as the withdrawal BOLA): release_/lock_/refund_ escrow_internal still have PUBLIC EXECUTE and the client calls release_escrow_internal directly with the anon key from confirmPaymentReceived -> an anon caller can drain any victim's LOCKED balance. Fix = a wallet-signed confirm-payment edge function (same pattern as withdrawals) before revoking PUBLIC execute. Not yet fixed.
2026-07-27 01:55:46 +00:00 · 2026-07-25 00:09:03 -07:00
parent 27b4057bd4
commit a8f41cd47f
13 changed files with 1178 additions and 96 deletions
@@ -0,0 +1,118 @@
+-- =====================================================
+-- Migration: Withdrawal authorization hardening
+-- Date: 2026-07-25
+-- =====================================================
+--
+-- Part of the fix for the CRITICAL broken-object-level-auth finding on the
+-- withdrawal path. Two defenses:
+--
+--   1. Replay protection for the signed withdrawal challenge (nonce store).
+--   2. Defense-in-depth: request_withdraw() may ONLY be executed by the backend
+--      service role. Previously it was executable by anon/authenticated, so an
+--      attacker with the public anon key could call it directly with a victim's
+--      user_id to create a pending withdrawal to an attacker-controlled wallet,
+--      which the batch processor would then pay out.
+--
+
+-- =====================================================
+-- 1. CHALLENGE NONCE STORE (single-use signed challenges)
+-- =====================================================
+CREATE TABLE IF NOT EXISTS public.p2p_challenge_nonces (
+  nonce      TEXT PRIMARY KEY,
+  purpose    TEXT NOT NULL,
+  created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
+);
+
+CREATE INDEX IF NOT EXISTS idx_p2p_challenge_nonces_created
+  ON public.p2p_challenge_nonces(created_at);
+
+ALTER TABLE public.p2p_challenge_nonces ENABLE ROW LEVEL SECURITY;
+
+-- Only the service role (edge functions) may read/write nonces.
+DROP POLICY IF EXISTS "challenge_nonces_service_only" ON public.p2p_challenge_nonces;
+CREATE POLICY "challenge_nonces_service_only" ON public.p2p_challenge_nonces
+  FOR ALL USING (auth.role() = 'service_role') WITH CHECK (auth.role() = 'service_role');
+
+REVOKE ALL ON public.p2p_challenge_nonces FROM anon, authenticated, PUBLIC;
+
+-- =====================================================
+-- 2. LOCK request_withdraw() TO SERVICE ROLE
+-- =====================================================
+-- Recreate with an explicit service-role guard (mirrors process_deposit()).
+CREATE OR REPLACE FUNCTION request_withdraw(
+  p_user_id UUID,
+  p_token TEXT,
+  p_amount DECIMAL(20, 12),
+  p_wallet_address TEXT
+) RETURNS JSON AS $$
+DECLARE
+  v_balance RECORD;
+  v_request_id UUID;
+BEGIN
+  -- SECURITY: backend service role only. Object-level authorization (proving the
+  -- caller owns p_user_id) is enforced by the process-withdraw edge function via
+  -- a verified wallet signature before this is ever invoked.
+  IF current_setting('role', true) <> 'service_role'
+     AND current_setting('request.jwt.claim.role', true) <> 'service_role' THEN
+    RETURN json_build_object(
+      'success', false,
+      'error', 'UNAUTHORIZED: withdrawals must go through the backend service'
+    );
+  END IF;
+
+  -- Lock user's balance
+  SELECT * INTO v_balance
+  FROM user_internal_balances
+  WHERE user_id = p_user_id AND token = p_token
+  FOR UPDATE;
+
+  -- Check sufficient available balance
+  IF v_balance IS NULL OR v_balance.available_balance < p_amount THEN
+    RETURN json_build_object(
+      'success', false,
+      'error', 'Insufficient available balance. Available: ' || COALESCE(v_balance.available_balance, 0)
+    );
+  END IF;
+
+  -- Lock the amount (move to locked_balance)
+  UPDATE user_internal_balances
+  SET
+    available_balance = available_balance - p_amount,
+    locked_balance = locked_balance + p_amount,
+    updated_at = NOW()
+  WHERE user_id = p_user_id AND token = p_token;
+
+  -- Create withdrawal request
+  INSERT INTO p2p_deposit_withdraw_requests (
+    user_id, request_type, token, amount, wallet_address, status
+  ) VALUES (
+    p_user_id, 'withdraw', p_token, p_amount, p_wallet_address, 'pending'
+  ) RETURNING id INTO v_request_id;
+
+  -- Log the transaction
+  INSERT INTO p2p_balance_transactions (
+    user_id, token, transaction_type, amount,
+    balance_before, balance_after, reference_type, reference_id,
+    description
+  ) VALUES (
+    p_user_id, p_token, 'withdraw', -p_amount,
+    v_balance.available_balance, v_balance.available_balance - p_amount, 'withdraw_request', v_request_id,
+    'Withdrawal request to ' || p_wallet_address
+  );
+
+  RETURN json_build_object(
+    'success', true,
+    'request_id', v_request_id,
+    'amount', p_amount,
+    'wallet_address', p_wallet_address,
+    'status', 'pending'
+  );
+END;
+$$ LANGUAGE plpgsql SECURITY DEFINER;
+
+REVOKE EXECUTE ON FUNCTION request_withdraw(UUID, TEXT, DECIMAL, TEXT) FROM PUBLIC, anon, authenticated;
+
+COMMENT ON FUNCTION request_withdraw IS
+'Create withdrawal request and lock balance. SERVICE ROLE ONLY — called by the
+process-withdraw edge function after it verifies the caller owns the identity via
+a wallet signature. Never call directly from the client.';
@@ -0,0 +1,122 @@
+-- =====================================================
+-- Migration: Harden RLS on financial + PII tables
+-- Date: 2026-07-25
+-- =====================================================
+--
+-- Forward migration (does NOT rewrite history) that removes the blanket
+-- `USING (true)` read policies added in 20260223160000_fix_rls_for_wallet_auth
+-- on the crown-jewel financial/PII tables, so the public anon key can no longer
+-- enumerate every user's balances, ledger history, deposit/withdraw requests, or
+-- payment-method PII.
+--
+-- Reads the app legitimately needs are moved behind SECURITY DEFINER RPCs that
+-- return only the requested user's rows (instead of the whole table). Writes to
+-- these tables already go through service_role edge functions / SECURITY DEFINER
+-- RPCs, so no client write path is affected.
+--
+-- NOTE (residual, tracked): user_id is a UUID v5 derived from a citizen/visa
+-- number, which is not a secret. These RPCs remove full-table exfiltration but do
+-- not yet cryptographically bind the reader to the row owner. Fully closing that
+-- requires a signed-session read path (same mechanism as withdrawals) applied to
+-- these reads — see the security report. The high-severity leak (dump ALL rows)
+-- is closed here.
+
+-- =====================================================
+-- 1. user_internal_balances — no direct client reads (uses
+--    get_user_internal_balance RPC which is SECURITY DEFINER and bypasses RLS)
+-- =====================================================
+DROP POLICY IF EXISTS "balances_anon_select" ON public.user_internal_balances;
+-- No anon policy remains -> anon/authenticated cannot read the table directly.
+-- SECURITY DEFINER RPCs (get_user_internal_balance) continue to work.
+
+-- =====================================================
+-- 2. p2p_balance_transactions — replace open SELECT with a scoped RPC
+-- =====================================================
+DROP POLICY IF EXISTS "balance_tx_anon_select" ON public.p2p_balance_transactions;
+
+CREATE OR REPLACE FUNCTION public.get_user_balance_transactions(
+  p_user_id UUID,
+  p_limit INT DEFAULT 50
+) RETURNS SETOF public.p2p_balance_transactions AS $$
+  SELECT *
+  FROM public.p2p_balance_transactions
+  WHERE user_id = p_user_id
+  ORDER BY created_at DESC
+  LIMIT LEAST(GREATEST(COALESCE(p_limit, 50), 1), 200);
+$$ LANGUAGE sql STABLE SECURITY DEFINER;
+
+GRANT EXECUTE ON FUNCTION public.get_user_balance_transactions(UUID, INT) TO anon, authenticated;
+
+COMMENT ON FUNCTION public.get_user_balance_transactions IS
+'Returns balance-transaction history for a single user_id only (no full-table
+dump). Replaces the removed USING(true) SELECT policy.';
+
+-- =====================================================
+-- 3. p2p_deposit_withdraw_requests — replace open SELECT with a scoped RPC
+--    (service_role write policy from 20260223160000 is retained)
+-- =====================================================
+DROP POLICY IF EXISTS "deposit_requests_anon_select" ON public.p2p_deposit_withdraw_requests;
+
+CREATE OR REPLACE FUNCTION public.get_user_deposit_withdraw_requests(
+  p_user_id UUID,
+  p_limit INT DEFAULT 50
+) RETURNS SETOF public.p2p_deposit_withdraw_requests AS $$
+  SELECT *
+  FROM public.p2p_deposit_withdraw_requests
+  WHERE user_id = p_user_id
+  ORDER BY created_at DESC
+  LIMIT LEAST(GREATEST(COALESCE(p_limit, 50), 1), 200);
+$$ LANGUAGE sql STABLE SECURITY DEFINER;
+
+GRANT EXECUTE ON FUNCTION public.get_user_deposit_withdraw_requests(UUID, INT) TO anon, authenticated;
+
+COMMENT ON FUNCTION public.get_user_deposit_withdraw_requests IS
+'Returns deposit/withdraw requests for a single user_id only. Replaces the
+removed USING(true) SELECT policy.';
+
+-- =====================================================
+-- 4. p2p_user_payment_methods (IBAN / payment PII) — lock to service role
+--    (no direct client access exists; offers embed encrypted payment details).
+-- =====================================================
+DO $$
+BEGIN
+  IF EXISTS (SELECT 1 FROM information_schema.tables
+             WHERE table_schema = 'public' AND table_name = 'p2p_user_payment_methods') THEN
+    EXECUTE 'ALTER TABLE public.p2p_user_payment_methods ENABLE ROW LEVEL SECURITY';
+    EXECUTE 'DROP POLICY IF EXISTS "user_pm_anon_select" ON public.p2p_user_payment_methods';
+    EXECUTE 'DROP POLICY IF EXISTS "user_pm_anon_insert" ON public.p2p_user_payment_methods';
+    EXECUTE 'DROP POLICY IF EXISTS "user_pm_anon_update" ON public.p2p_user_payment_methods';
+    EXECUTE 'DROP POLICY IF EXISTS "user_pm_service_only" ON public.p2p_user_payment_methods';
+    EXECUTE 'CREATE POLICY "user_pm_service_only" ON public.p2p_user_payment_methods
+               FOR ALL USING (auth.role() = ''service_role'') WITH CHECK (auth.role() = ''service_role'')';
+    EXECUTE 'REVOKE ALL ON public.p2p_user_payment_methods FROM anon, authenticated, PUBLIC';
+  END IF;
+END $$;
+
+-- =====================================================
+-- 5. p2p_fiat_disputes — remove world-open UPDATE.
+--    Dispute state transitions (claim/resolve) now go through the service-role
+--    `resolve-dispute` edge function (which verifies the admin wallet signature).
+--    Regular users still INSERT disputes (disputes_anon_insert retained).
+-- =====================================================
+DROP POLICY IF EXISTS "disputes_anon_update" ON public.p2p_fiat_disputes;
+DROP POLICY IF EXISTS "disputes_service_update" ON public.p2p_fiat_disputes;
+CREATE POLICY "disputes_service_update" ON public.p2p_fiat_disputes
+  FOR UPDATE USING (auth.role() = 'service_role') WITH CHECK (auth.role() = 'service_role');
+
+-- =====================================================
+-- Done. Balances, ledger, deposit/withdraw requests and payment PII are no longer
+-- world-readable via the anon key; dispute resolution is no longer world-writable.
+--
+-- RESIDUAL (documented in the security report, NOT closed here to avoid breaking
+-- the live app without the signed-session refactor):
+--   * p2p_fiat_offers / p2p_fiat_trades / p2p_messages still carry USING(true)
+--     policies because the client reads/writes them directly with the anon key
+--     and there is no server session to bind to. Marking a trade status does not
+--     itself move funds.
+--   * The internal-ledger escrow RPCs (lock_/release_/refund_escrow_internal)
+--     are still EXECUTE-able by anon (confirmPaymentReceived / createFiatOffer
+--     call them directly). This is a SEPARATE critical hole that needs the same
+--     signed-challenge mechanism applied to trade actions (an authenticated
+--     confirm-payment / create-offer edge function). See report.
+-- =====================================================
@@ -0,0 +1,158 @@
+-- =====================================================
+-- Migration: Atomic admin dispute resolution (moves escrow)
+-- Date: 2026-07-25
+-- =====================================================
+--
+-- Fix for the CRITICAL fund-logic finding: the admin Dispute Resolution panel
+-- only relabelled p2p_fiat_trades.status and NEVER moved escrow, so "buyer wins"
+-- paid nothing, "refund seller" left funds locked, and "split" was unhandled.
+--
+-- This SECURITY DEFINER function performs the escrow movement AND all status
+-- changes in a single transaction. It is service-role only; caller identity
+-- (admin) is verified by the resolve-dispute edge function via a wallet
+-- signature before this is invoked.
+--
+-- ESCROW ACCOUNTING (verified against createFiatOffer + accept_p2p_offer):
+--   * createFiatOffer locks the FULL offer amount (available -> locked).
+--   * accept_p2p_offer does NOT lock again; it only decrements
+--     offer.remaining_amount. So: seller.locked == remaining_amount + SUM(active
+--     trade amounts) for the offer.
+--   Therefore, per trade of amount X:
+--     - release_to_buyer : locked(seller) -= X, available(buyer) += X.  (offer untouched)
+--     - refund_to_seller : locked(seller) -= X, available(seller) += X. (offer untouched)
+--     - split            : release X/2 to buyer + refund X/2 to seller. (offer untouched)
+--   remaining_amount is intentionally NOT restored (that would double-count the
+--   funds — a latent bug in the older resolve_p2p_dispute()).
+
+CREATE OR REPLACE FUNCTION public.admin_resolve_dispute(
+  p_dispute_id UUID,
+  p_trade_id UUID,
+  p_decision TEXT,            -- 'release_to_buyer' | 'refund_to_seller' | 'split' | 'escalate'
+  p_reasoning TEXT,
+  p_admin_ref TEXT            -- admin wallet address (for audit only)
+) RETURNS JSON AS $$
+DECLARE
+  v_trade RECORD;
+  v_token TEXT;
+  v_half DECIMAL(20, 12);
+  v_res JSON;
+  v_trade_status TEXT;
+BEGIN
+  -- SECURITY: backend service role only. Admin identity is verified upstream by
+  -- the resolve-dispute edge function (wallet signature vs. admin wallet set).
+  IF current_setting('role', true) <> 'service_role'
+     AND current_setting('request.jwt.claim.role', true) <> 'service_role' THEN
+    RETURN json_build_object('success', false, 'error', 'UNAUTHORIZED: service role required');
+  END IF;
+
+  IF p_reasoning IS NULL OR length(trim(p_reasoning)) = 0 THEN
+    RETURN json_build_object('success', false, 'error', 'Reasoning is required');
+  END IF;
+
+  IF p_decision NOT IN ('release_to_buyer', 'refund_to_seller', 'split', 'escalate') THEN
+    RETURN json_build_object('success', false, 'error', 'Invalid decision');
+  END IF;
+
+  -- Lock the trade row
+  SELECT * INTO v_trade FROM public.p2p_fiat_trades WHERE id = p_trade_id FOR UPDATE;
+  IF NOT FOUND THEN
+    RETURN json_build_object('success', false, 'error', 'Trade not found');
+  END IF;
+
+  -- ----- ESCALATE: no fund movement, just mark dispute escalated -----
+  IF p_decision = 'escalate' THEN
+    UPDATE public.p2p_fiat_disputes
+    SET status = 'escalated', decision = 'escalate', decision_reasoning = p_reasoning, updated_at = NOW()
+    WHERE id = p_dispute_id;
+
+    INSERT INTO public.p2p_audit_log (user_id, action, entity_type, entity_id, details)
+    VALUES (NULL, 'dispute_escalated', 'trade', p_trade_id,
+      jsonb_build_object('dispute_id', p_dispute_id, 'admin_ref', p_admin_ref, 'reasoning', p_reasoning));
+
+    RETURN json_build_object('success', true, 'decision', 'escalate');
+  END IF;
+
+  -- For fund-moving decisions the trade must be in dispute.
+  IF v_trade.status <> 'disputed' THEN
+    RETURN json_build_object('success', false, 'error', 'Trade is not in disputed status (current: ' || v_trade.status || ')');
+  END IF;
+
+  -- Resolve token from the offer
+  SELECT token INTO v_token FROM public.p2p_fiat_offers WHERE id = v_trade.offer_id;
+  IF v_token IS NULL THEN
+    RETURN json_build_object('success', false, 'error', 'Offer/token not found for trade');
+  END IF;
+
+  IF p_decision = 'release_to_buyer' THEN
+    v_res := public.release_escrow_internal(
+      v_trade.seller_id, v_trade.buyer_id, v_token, v_trade.crypto_amount, 'dispute_resolution', p_trade_id);
+    IF (v_res->>'success')::boolean IS NOT TRUE THEN
+      RAISE EXCEPTION 'release_escrow_internal failed: %', COALESCE(v_res->>'error', 'unknown');
+    END IF;
+    v_trade_status := 'completed';
+
+  ELSIF p_decision = 'refund_to_seller' THEN
+    v_res := public.refund_escrow_internal(
+      v_trade.seller_id, v_token, v_trade.crypto_amount, 'dispute_resolution', p_trade_id);
+    IF (v_res->>'success')::boolean IS NOT TRUE THEN
+      RAISE EXCEPTION 'refund_escrow_internal failed: %', COALESCE(v_res->>'error', 'unknown');
+    END IF;
+    v_trade_status := 'refunded';
+
+  ELSIF p_decision = 'split' THEN
+    v_half := ROUND(v_trade.crypto_amount / 2, 12);
+    -- Half to buyer
+    v_res := public.release_escrow_internal(
+      v_trade.seller_id, v_trade.buyer_id, v_token, v_half, 'dispute_resolution_split', p_trade_id);
+    IF (v_res->>'success')::boolean IS NOT TRUE THEN
+      RAISE EXCEPTION 'split release failed: %', COALESCE(v_res->>'error', 'unknown');
+    END IF;
+    -- Remainder back to seller (handles odd cents deterministically)
+    v_res := public.refund_escrow_internal(
+      v_trade.seller_id, v_token, v_trade.crypto_amount - v_half, 'dispute_resolution_split', p_trade_id);
+    IF (v_res->>'success')::boolean IS NOT TRUE THEN
+      RAISE EXCEPTION 'split refund failed: %', COALESCE(v_res->>'error', 'unknown');
+    END IF;
+    v_trade_status := 'completed';
+  END IF;
+
+  -- Update trade
+  UPDATE public.p2p_fiat_trades
+  SET status = v_trade_status,
+      completed_at = CASE WHEN v_trade_status = 'completed' THEN NOW() ELSE completed_at END,
+      escrow_released_at = NOW(),
+      dispute_resolved_at = NOW(),
+      dispute_resolution = p_decision || ': ' || COALESCE(p_reasoning, ''),
+      updated_at = NOW()
+  WHERE id = p_trade_id;
+
+  -- Update dispute
+  UPDATE public.p2p_fiat_disputes
+  SET status = 'resolved', decision = p_decision, decision_reasoning = p_reasoning,
+      resolved_at = NOW(), updated_at = NOW()
+  WHERE id = p_dispute_id;
+
+  -- Notify both parties (best-effort, same txn)
+  INSERT INTO public.p2p_notifications (user_id, type, title, message, reference_type, reference_id)
+  VALUES
+    (v_trade.seller_id, 'dispute_resolved', 'Dispute Resolved', 'Your dispute was resolved: ' || p_decision, 'dispute', p_dispute_id),
+    (v_trade.buyer_id,  'dispute_resolved', 'Dispute Resolved', 'Your dispute was resolved: ' || p_decision, 'dispute', p_dispute_id);
+
+  -- Audit
+  INSERT INTO public.p2p_audit_log (user_id, action, entity_type, entity_id, details)
+  VALUES (NULL, 'dispute_resolved', 'trade', p_trade_id,
+    jsonb_build_object(
+      'dispute_id', p_dispute_id, 'decision', p_decision, 'admin_ref', p_admin_ref,
+      'reasoning', p_reasoning, 'seller_id', v_trade.seller_id, 'buyer_id', v_trade.buyer_id,
+      'amount', v_trade.crypto_amount, 'token', v_token));
+
+  RETURN json_build_object('success', true, 'decision', p_decision, 'trade_id', p_trade_id, 'token', v_token, 'amount', v_trade.crypto_amount);
+END;
+$$ LANGUAGE plpgsql SECURITY DEFINER;
+
+REVOKE EXECUTE ON FUNCTION public.admin_resolve_dispute(UUID, UUID, TEXT, TEXT, TEXT) FROM PUBLIC, anon, authenticated;
+
+COMMENT ON FUNCTION public.admin_resolve_dispute IS
+'Atomically resolves a P2P dispute: moves escrow (release/refund/split) AND
+updates trade + dispute status + notifications in one transaction. SERVICE ROLE
+ONLY — admin identity is verified by the resolve-dispute edge function.';