fix(payments): prevent on-chain txId replay across winner submissions#1357
fix(payments): prevent on-chain txId replay across winner submissions#1357mitgajera wants to merge 1 commit into
Conversation
|
@mitgajera is attempting to deploy a commit to the Superteam Team on Vercel. A member of the Team first needs to authorize it. |
mitgajera
changed the title
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR extends sponsor and user tracking across the homepage banner component by adding a Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API Request
participant API as verify-external-payment API
participant DB as Database
Client->>API: POST with txIds to verify
activate API
Note over API: Extract submitted txIds
API->>DB: Query submissions with paymentDetails<br/>containing any submitted txIds
activate DB
DB-->>API: Return matching submissions<br/>(paid + unpaid)
deactivate DB
Note over API: Aggregate txIds from results<br/>Deduplicate
alt txIds already used
API-->>Client: 400 Bad Request
else txIds are new
API->>DB: Process payment
activate DB
DB-->>API: Success
deactivate DB
API-->>Client: 200 OK
end
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/api/sponsor-dashboard/listings/verify-external-payment.ts (2)
108-115: Query approach is correct; consider indexing for scale.The
paymentDetailsfield is a JSON type storing an array of payment objects. Thestring_containsfilter correctly searches across all submissions but cannot use database indexes efficiently.For production scale, if this endpoint becomes slow, consider storing
txIdvalues in a dedicated indexed column or a separatepayment_transactionstable. This is not blocking for the current fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/api/sponsor-dashboard/listings/verify-external-payment.ts` around lines 108 - 115, The current prisma.submission.findMany query scans the JSON paymentDetails using string_contains (paymentDetails, txIds) which won’t scale; add a dedicated indexed field or relation to store transaction IDs (e.g., a txIds string[] column on Submission or a payment_transactions table with submissionId and txId) and migrate data, then update the lookup in the verify-external-payment handler to query that indexed column/relation (e.g., filter by txIds has/hasSome or join on payment_transactions) instead of using paymentDetails string_contains; keep prisma model changes consistent with existing functions that read/write payments.
102-127: Solid fix for the txId replay vulnerability.The global query approach correctly addresses the security issue by checking all submissions rather than just request-scoped unpaid ones. The filtering on line 125 properly handles potential
string_containssubstring false positives by intersecting with the inputtxIds.One minor edge case: if
txIdsis empty (all payment links lack a txId), the query runs withOR: []which is wasteful. Consider adding an early guard:🛡️ Suggested guard for empty txIds
const txIds = paymentLinks.map((link) => link.txId).filter(Boolean); + + // Skip global txId check if no txIds to verify + if (txIds.length === 0) { + // Proceed directly to validation loop - individual links will fail with "Invalid URL" + } + const duplicateTxIds = txIds.filter( (txId, index) => txIds.indexOf(txId) !== index, );Or wrap lines 102-133 in an
if (txIds.length > 0)block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/api/sponsor-dashboard/listings/verify-external-payment.ts` around lines 102 - 127, Add an early guard to skip the global DB query when there are no txIds: if txIds.length === 0 return/skip the block that runs prisma.submission.findMany and the subsequent computation of submissionsUsingTxIds and alreadyUsedTxIds; this avoids executing prisma.submission.findMany({ where: { OR: txIds.map(...) } }) with an empty OR and prevents wasted work when all payment links lack a txId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/earn/index.tsx`:
- Line 168: The calculation for totalUsers can go negative when userCount < 289;
clamp the adjusted count before rounding by replacing the expression that
computes totalUsers (the Math.ceil((userCount - 289) / 10) * 10 line) with a
version that first computes adjusted = Math.max(userCount - 289, 0) (or
equivalent), then rounds up in tens: Math.ceil(adjusted / 10) * 10, ensuring
totalUsers never becomes negative; update references to totalUsers if needed.
- Around line 163-173: The server-side code is blocking SSR by calling
prisma.user.count() and prisma.sponsors.count() inside getServerSideProps;
remove those calls and stop returning totalUsers/totalSponsors from
getServerSideProps (keep returning potentialSession/cookieExists), and instead
fetch the stats client-side from the cached endpoints /api/homepage/user-count
and /api/homepage/sponsor-count (e.g., in the page component using useEffect or
SWR) and populate UI state with their JSON responses and graceful fallbacks;
make sure to reference and replace the existing
prisma.user.count()/prisma.sponsors.count() usage and the returned props object
so the page no longer waits for DB counts on the server.
---
Nitpick comments:
In `@src/pages/api/sponsor-dashboard/listings/verify-external-payment.ts`:
- Around line 108-115: The current prisma.submission.findMany query scans the
JSON paymentDetails using string_contains (paymentDetails, txIds) which won’t
scale; add a dedicated indexed field or relation to store transaction IDs (e.g.,
a txIds string[] column on Submission or a payment_transactions table with
submissionId and txId) and migrate data, then update the lookup in the
verify-external-payment handler to query that indexed column/relation (e.g.,
filter by txIds has/hasSome or join on payment_transactions) instead of using
paymentDetails string_contains; keep prisma model changes consistent with
existing functions that read/write payments.
- Around line 102-127: Add an early guard to skip the global DB query when there
are no txIds: if txIds.length === 0 return/skip the block that runs
prisma.submission.findMany and the subsequent computation of
submissionsUsingTxIds and alreadyUsedTxIds; this avoids executing
prisma.submission.findMany({ where: { OR: txIds.map(...) } }) with an empty OR
and prevents wasted work when all payment links lack a txId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1ad9a75-faca-4967-86d3-abb49157e01c
📒 Files selected for processing (5)
src/features/home/components/Banner/SponsorBanner.tsxsrc/features/home/components/Banner/index.tsxsrc/pages/api/sponsor-dashboard/listings/verify-external-payment.tssrc/pages/earn/index.tsxsrc/utils/json-ld.ts
| const [userCount, sponsorCount] = await Promise.all([ | ||
| prisma.user.count(), | ||
| prisma.sponsors.count(), | ||
| ]); | ||
|
|
||
| const totalUsers = Math.ceil((userCount - 289) / 10) * 10; | ||
| const totalSponsors = Math.ceil(sponsorCount / 10) * 10; | ||
|
|
||
| return { | ||
| props: { potentialSession: cookieExists, totalUsers, totalSponsors }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file structure and read the relevant section
wc -l src/pages/earn/index.tsxRepository: SuperteamDAO/earn
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the getServerSideProps function to see full context
sed -n '140,180p' src/pages/earn/index.tsxRepository: SuperteamDAO/earn
Length of output: 936
🏁 Script executed:
#!/bin/bash
# Check for any caching mechanisms in the codebase
rg -i "cache|redis|memoize" src/pages/earn/ -A 2 -B 2Repository: SuperteamDAO/earn
Length of output: 3276
🏁 Script executed:
#!/bin/bash
# Check if there's error handling around the count queries
rg "prisma.user.count|prisma.sponsors.count" -B 5 -A 5Repository: SuperteamDAO/earn
Length of output: 5872
🏁 Script executed:
#!/bin/bash
# Check the import statements to understand the structure
head -20 src/pages/earn/index.tsxRepository: SuperteamDAO/earn
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Check the full implementation of the API routes
cat src/app/api/homepage/user-count/route.tsRepository: SuperteamDAO/earn
Length of output: 1067
🏁 Script executed:
#!/bin/bash
# Check the sponsor count route
cat src/app/api/homepage/sponsor-count/route.tsRepository: SuperteamDAO/earn
Length of output: 1057
Use the existing cached API routes instead of blocking on database counts in getServerSideProps.
Lines 163-166 make every page request wait on synchronous prisma.user.count() and prisma.sponsors.count() calls with no error handling. The identical logic already exists in dedicated API routes (/api/homepage/user-count and /api/homepage/sponsor-count) with proper caching (1 day max-age), error handling, and graceful fallbacks. Fetch these stats client-side from those endpoints instead of blocking the SSR path on live database queries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/earn/index.tsx` around lines 163 - 173, The server-side code is
blocking SSR by calling prisma.user.count() and prisma.sponsors.count() inside
getServerSideProps; remove those calls and stop returning
totalUsers/totalSponsors from getServerSideProps (keep returning
potentialSession/cookieExists), and instead fetch the stats client-side from the
cached endpoints /api/homepage/user-count and /api/homepage/sponsor-count (e.g.,
in the page component using useEffect or SWR) and populate UI state with their
JSON responses and graceful fallbacks; make sure to reference and replace the
existing prisma.user.count()/prisma.sponsors.count() usage and the returned
props object so the page no longer waits for DB counts on the server.
| prisma.sponsors.count(), | ||
| ]); | ||
|
|
||
| const totalUsers = Math.ceil((userCount - 289) / 10) * 10; |
There was a problem hiding this comment.
Clamp the adjusted user count before rounding.
Line 168 goes negative whenever userCount < 289, which can leak a negative reach number into the banner on fresh or staging databases.
Suggested fix
- const totalUsers = Math.ceil((userCount - 289) / 10) * 10;
+ const totalUsers = Math.ceil(Math.max(0, userCount - 289) / 10) * 10;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalUsers = Math.ceil((userCount - 289) / 10) * 10; | |
| const totalUsers = Math.ceil(Math.max(0, userCount - 289) / 10) * 10; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/earn/index.tsx` at line 168, The calculation for totalUsers can go
negative when userCount < 289; clamp the adjusted count before rounding by
replacing the expression that computes totalUsers (the Math.ceil((userCount -
289) / 10) * 10 line) with a version that first computes adjusted =
Math.max(userCount - 289, 0) (or equivalent), then rounds up in tens:
Math.ceil(adjusted / 10) * 10, ensuring totalUsers never becomes negative;
update references to totalUsers if needed.
The verify-external-payment endpoint checked txIds only against submissions fetched with `isPaid: false` in the current request. A txId from an already-paid submission was invisible to the guard, allowing the same on-chain transaction to be reused to mark a second winner as paid. Fix: replace the local check with a global prisma.submission.findMany query that scans ALL submissions (paid and unpaid) for any matching txId before entering the payment verification loop. Also adds an early return when no valid txIds are present to avoid a wasted query.
mitgajera
force-pushed
the
fix/payment-txid-replay
branch
from
e173f33 to
de20fac
Compare
1 participant
What does this PR do?
Fixes a vulnerability in the
verify-external-paymentendpoint where the same on-chain transaction ID could be used to mark multiple winners as paid.The duplicate txId check was only scanning submissions fetched with
isPaid: falsein the current request. A txId from an already-paid submission wasinvisible to this check, so a sponsor could reuse the same real on-chain transaction to "verify" payment for additional winners.
Fix: replaced the local check with a global
prisma.submission.findManyquery that scans all submissions (paid and unpaid) for any matching txIdbefore processing the payment loop.
Where should the reviewer start?
src/pages/api/sponsor-dashboard/listings/verify-external-payment.ts— lines 102–131.The old code (removed):
How should this be manually tested?
Any background context you want to provide?
The root cause is that the submissions variable used for the duplicate check is pre-filtered to isPaid: false, which excludes paid submissions from the scope
of the check. The fix queries globally with no payment-status filter.
What are the relevant issues?
N/A (security find, no prior issue)
Summary by CodeRabbit