Skip to content

enforce server-side, on-chain-verified credit issuance (unlimited-cre… - #56

Open
mertcano wants to merge 1 commit into
circlefin:masterfrom
mertcano:mertcano-patch-1
Open

enforce server-side, on-chain-verified credit issuance (unlimited-cre…#56
mertcano wants to merge 1 commit into
circlefin:masterfrom
mertcano:mertcano-patch-1

Conversation

@mertcano

Copy link
Copy Markdown

A critical vulnerability in the credit top-up path where the server previously trusted client-supplied credit and USDC amounts without on-chain verification.

** Vulnerabilities & Anti-Patterns Remediated:**

  • Business Logic / Broken Access Control — Unlimited Credit Minting (CRITICAL):
    Previously, POST /api/transactions stored credit_amount straight from the request body. Combined with the Circle webhook granting exactly transaction.credit_amount upon confirmation, an authenticated user could mint an unlimited number of credits for the cost of a single $0.01 USDC transfer.
    Fix: The endpoint now ignores client-supplied credits and usdcAmount. Issuance is calculated strictly server-side using the hardcoded EXCHANGE_RATE_USDC_PER_CREDIT.

  • Missing On-Chain Settlement Verification (CRITICAL):
    The server recorded a payment purely based on the client's payload.
    Fix: Integrated viem to fetch the transaction receipt by txHash via a server RPC. The transaction is verified to ensure it represents a successful USDC transfer to the correct admin wallet from the authenticated user's wallet before any database insertion occurs.

  • Sensitive Error Disclosure (MEDIUM):
    Raw database and Row-Level-Security (RLS) errors (e.g., insertError.message, insertError.code, RLS_BLOCK) were previously passed directly to the client via 500 responses.
    Fix: Detailed error logs are now isolated to the server console. The client receives generic, opaque error codes (e.g., "Insert failed" or "Server error").

Key Code Changes:

  • app/api/transactions/route.ts: Completely refactored the POST handler to perform on-chain receipt validation, decode USDC Transfer event logs, and strictly derive credit allocations on the backend.

…dit mint)

A critical vulnerability in the credit top-up path where the server previously trusted client-supplied credit and USDC amounts without on-chain verification. 

**🚨 Vulnerabilities & Anti-Patterns Remediated:**

*   **Business Logic / Broken Access Control — Unlimited Credit Minting (CRITICAL):**
    Previously, `POST /api/transactions` stored `credit_amount` straight from the request body. Combined with the Circle webhook granting exactly `transaction.credit_amount` upon confirmation, an authenticated user could mint an unlimited number of credits for the cost of a single $0.01 USDC transfer. 
    **Fix:** The endpoint now ignores client-supplied `credits` and `usdcAmount`. Issuance is calculated strictly server-side using the hardcoded `EXCHANGE_RATE_USDC_PER_CREDIT`.

*   **Missing On-Chain Settlement Verification (CRITICAL):**
    The server recorded a payment purely based on the client's payload. 
    **Fix:** Integrated `viem` to fetch the transaction receipt by `txHash` via a server RPC. The transaction is verified to ensure it represents a successful USDC transfer to the correct admin wallet from the authenticated user's wallet before any database insertion occurs.

*   **Sensitive Error Disclosure (MEDIUM):**
    Raw database and Row-Level-Security (RLS) errors (e.g., `insertError.message`, `insertError.code`, `RLS_BLOCK`) were previously passed directly to the client via 500 responses.
    **Fix:** Detailed error logs are now isolated to the server console. The client receives generic, opaque error codes (e.g., "Insert failed" or "Server error").

**Key Code Changes:**
*   `app/api/transactions/route.ts`: Completely refactored the `POST` handler to perform on-chain receipt validation, decode USDC `Transfer` event logs, and strictly derive credit allocations on the backend.
@kutluhaneth46

Copy link
Copy Markdown

Direction is good (server-derived credits + receipt check), but as written the verification is still attacker-controlled in two places that defeat the CRITICAL claim.

1. Client-supplied destinationAddress becomes the expected admin

const adminStr = destinationAddress || EXPECTED_ADMIN_WALLET;
// ...
getAddress(e.args.to as string) === admin

The purchase UI already learns the real admin from GET /api/destination-wallet (DB admin_wallets). Here the client can override that with any to address. An attacker can set destinationAddress to themselves, transfer USDC to themselves, pass the "matching transfer" check, and mint credits.

The expected recipient must be resolved server-side (same query as /api/destination-wallet, or a pinned ADMIN_WALLET_ADDRESS env with no client override). Drop destinationAddress from the trusted path entirely.

2. Credits attach to the session user, not to a verified payer

user_id: user.id comes from the cookie session, while from is whatever walletAddress the client sends. Any authenticated user who observes a legitimate Transfer(victim → admin) can POST that txHash with walletAddress = victim and receive the credits on their account. Idempotency on chainId:txHash then permanently burns the victim's top-up.

Need at least one of:

  • bind walletAddress to the authenticated user (stored wallet / prior linking), or
  • require the tx sender to match a wallet the user has proven control of in-session,

before inserting credits.

3. Arc Testnet (the demo default) is absent

DEFAULT_CHAIN in this repo is Arc Testnet 5042002 with USDC 0x3600…0000, but RPC_BY_CHAIN / USDC_BY_CHAIN only list Ethereum/Polygon/Base (+ sepolias). A real purchase on the primary demo network 400s as Invalid payload via !RPC_BY_CHAIN[chainId].

4. Smaller nits

  • Number(transfer.args.value) / 1e6 reintroduces float for a monetary value; prefer integer micro-USDC → credits with bigint math.
  • Public fallback RPCs are fine for a sample app, but should stay env-overridable only for anything beyond local demos.

Happy to re-review once recipient + payer binding are server-authoritative — the receipt/Transfer decode approach is the right skeleton.

@kutluhaneth46

Copy link
Copy Markdown

Opened a follow-up that addresses the holes above: #60 — server-side admin recipient, claim signature bound to the paying wallet, Arc Testnet RPC, and bigint-derived credits. Happy to align if you prefer iterating on this PR instead.

Made with Cursor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants