fix: prevent relayer wallet from depositing own ETH into escrow (#595)#616
Open
ionfwsrijan wants to merge 3 commits into
Open
fix: prevent relayer wallet from depositing own ETH into escrow (#595)#616ionfwsrijan wants to merge 3 commits into
ionfwsrijan wants to merge 3 commits into
Conversation
Contributor
|
🎉 Thank you for your contribution! Your pull request has been received and will be reviewed shortly. If you enjoy the project, please consider giving the repository a ⭐. You can also follow my GitHub profile to stay updated on future open-source projects. Thanks for being part of the community! 🚀 |
github-actions
Bot
added
backend
📝 WalkthroughWalkthroughThe PR fixes a financial architecture flaw where the relayer wallet was paying escrow deposits instead of the customer. The Escrow contract now enforces ChangesEscrow contract and API: customer-signed deposits
Mobile app authentication and marketplace updates
Sequence Diagram(s)sequenceDiagram
participant Customer as Customer Client
participant API as Order API
participant EscrowSvc as escrow.js
participant RPC as Blockchain RPC
participant Contract as Escrow.sol
rect rgba(99, 149, 255, 0.5)
note over Customer,Contract: Step 1 — Bid Accept (server builds unsigned tx)
Customer->>API: POST /:id/bids/:bidId/accept
API->>EscrowSvc: buildDepositTx(orderDisplayId, customerWallet, driverWallet, amountWei)
EscrowSvc->>RPC: getFeeData() + getBlock('latest')
RPC-->>EscrowSvc: feeData, block
EscrowSvc-->>API: { txData: unsignedEIP1559Tx, bookingId }
API->>API: DB update escrow_booking_id + escrow_status='funding'
API-->>Customer: 200 { txData, message: "awaiting customer deposit signature" }
end
rect rgba(99, 200, 120, 0.5)
note over Customer,Contract: Step 2 — Customer signs, submits, then confirms
Customer->>Contract: sign + broadcast txData (msg.sender == customer)
Contract->>Contract: require(msg.sender == customer) ✓
Contract-->>Customer: txHash
Customer->>API: POST /:id/confirm-deposit { txHash }
API->>EscrowSvc: recordDepositTx(bookingId, txHash)
EscrowSvc->>RPC: waitForTransaction(txHash, 1)
RPC-->>EscrowSvc: receipt (status OK)
EscrowSvc-->>API: { txHash, bookingId }
API->>API: DB update escrow_status='funded', deposit_tx_hash, escrow_deposited_at
API-->>Customer: 200 { txHash }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 990-996: The order fetch in the supabase query does not verify
that the authenticated customer owns the order, creating a security
vulnerability where any customer could confirm deposits for orders belonging to
other customers. Obtain the authenticated customer's ID from the request object
(likely from req.user or similar authentication context), and add an additional
equality check to the supabase query using .eq('customer_id', customerId)
alongside the existing .eq('id', orderId) clause. Alternatively, after fetching
the order, verify that the order's customer_id matches the authenticated
customer's ID in the conditional check before proceeding with the operation.
- Around line 1006-1010: The supabase update call for the orders table that sets
escrow_status to 'funded' and updates deposit_tx_hash and escrow_deposited_at
lacks error handling, which can lead to state inconsistency if the database
update fails after the on-chain deposit is confirmed. Wrap the await
supabase.from('orders').update() call in proper error handling using try-catch
or check the response object for errors, and implement appropriate recovery
logic such as logging the failure, attempting a rollback of the blockchain
transaction if possible, or retrying the database operation to maintain
consistency between the blockchain and backend database state.
- Around line 983-985: The confirm-deposit route handler uses z.object() and
z.string() in the validateBody call but the Zod library is not imported at the
top of the file, which will cause a ReferenceError at runtime. Add an import
statement for z from the 'zod' package at the top of the file alongside your
other imports, or alternatively move the inline schema object to
requestSchemas.js where other request validation schemas are defined and import
that schema instead.
- Around line 664-677: The buildDepositTx function returns an object containing
txData and bookingId, but the API response only sends a generic message. When
buildDepositTx succeeds and returns txData, you must include the txData and
bookingId in the res.json response so the customer receives the unsigned
transaction data needed to sign and complete the deposit flow. Update the
destructuring of the buildDepositTx result to capture bookingId as well, then
pass both txData and bookingId to the res.json response along with the message.
In `@backend/api/src/services/escrow.js`:
- Around line 132-147: The recordDepositTx function accepts any successful
transaction hash without verifying that it actually deposited funds for the
expected bookingId, creating a security vulnerability where malicious users can
confirm deposits for the wrong booking. After retrieving the transaction
receipt, parse the receipt's logs to look for a Deposited event and verify that
this event was emitted with the expected bookingId parameter. Only return
success if the receipt contains a valid Deposited event matching the provided
bookingId; otherwise return an appropriate error indicating the transaction does
not correspond to the expected booking.
- Around line 104-117: The block.chainId property does not exist in ethers v6,
resulting in an undefined chainId in the txData object. Instead of accessing
chainId from the block returned by getBlock('latest'), retrieve the chainId from
the network object using escrowContract.runner.provider.getNetwork(). Add an
await call to get the network and extract the chainId property from the returned
network object, then assign it to the chainId field in txData instead of using
block.chainId.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cca102ac-ecd5-4fe2-8b54-94588e74e627
📒 Files selected for processing (4)
backend/api/src/routes/orderRoutes.jsbackend/api/src/services/escrow.jsblockchain/contracts/Escrow.solblockchain/test/escrow.test.cjs
ionfwsrijan
force-pushed
the
fix/issue-595-escrow-deposit-flow
branch
from
19de4ad to
c5b3456
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blockchain/contracts/Escrow.sol (1)
60-67:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLine 65 auth check is incomplete for first-write ownership of a booking.
require(msg.sender == customer)prevents third-party funding, butcustomer/driverare still caller-supplied on initial creation. A caller can self-assign ascustomerfor a knownbookingId, fund once, and block the real customer (Escrow exists). Please bind first deposit to pre-authorized booking parties (e.g., relayer/owner pre-registration or signed authorization overbookingId,customer,driver,amount,deadline).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blockchain/contracts/Escrow.sol` around lines 60 - 67, The deposit function accepts customer and driver as caller-supplied parameters, allowing an attacker to self-assign as customer for any bookingId and block the real customer from depositing due to the "Escrow exists" check. To fix this, implement pre-authorization for booking parties by either adding a prior registration/approval mechanism (e.g., a createBooking function that records authorized customer/driver pairs for each bookingId before deposit can be called) or add signature-based authorization that verifies the caller has signed the specific bookingId, customer, driver, and amount parameters. Validate these pre-authorized parties early in the deposit function before accepting the funds, ensuring only authorized participants can create new escrows.
♻️ Duplicate comments (3)
backend/api/src/routes/orderRoutes.js (3)
1010-1012:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
zis not imported — server will crash withReferenceError.The inline Zod schema uses
z.object()andz.string()butzis not imported. This causes test failures as confirmed by the pipeline.🐛 Proposed fix — add Zod import
Add to imports at the top of the file:
import express from 'express'; import crypto from 'crypto'; import { ethers } from 'ethers'; +import { z } from 'zod'; import { bidLimiter } from '../middleware/rateLimiter.js';Alternatively, define the schema in
requestSchemas.jsalongside the other schemas and import it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` around lines 1010 - 1012, The Zod validation library is not imported but the code uses z.object() and z.string() in the validateBody call for the confirm-deposit route handler, which will cause a ReferenceError. Add the missing import statement for Zod at the top of the file, or alternatively move the inline schema object containing the txHash validation to a separate requestSchemas.js file and import it instead of defining it inline.Source: Linters/SAST tools
1033-1038:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing error handling on order update — state could become inconsistent.
The update to
escrow_status: 'funded'does not check for errors. If the database update fails after confirming the on-chain deposit, the backend state would be inconsistent with blockchain state.🛡️ Proposed fix
- await supabase.from('orders').update({ + const { error: updateErr } = await supabase.from('orders').update({ escrow_status: 'funded', deposit_tx_hash: result.txHash, escrow_deposited_at: new Date().toISOString(), }).eq('id', orderId); + if (updateErr) { + logger.error('[confirm-deposit] Failed to update order:', updateErr.message); + return res.status(500).json({ + error: 'Deposit confirmed on-chain but failed to update order record', + txHash: result.txHash + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` around lines 1033 - 1038, The supabase.from('orders').update() call that sets escrow_status to 'funded' lacks error handling, which could leave the database state inconsistent with blockchain state if the update fails. Add error handling around this update operation by either wrapping it in a try-catch block or checking the response object returned by the update operation. If the update fails, ensure you log the error appropriately and either throw an error to prevent further execution or implement a recovery mechanism to maintain consistency between the on-chain deposit and the backend order record.
1016-1026:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing order ownership check — any customer can confirm deposits for any order.
The endpoint fetches the order by
idwithout verifyingcustomer_id. WhilerequireRole(['customer'])ensures the caller is a customer, it doesn't ensure they own the order.🛡️ Proposed fix — add ownership check
const { data: order, error: fetchErr } = await supabase .from('orders') .select('id, order_display_id, escrow_booking_id, escrow_status') .eq('id', orderId) + .eq('customer_id', req.user.id) .maybeSingle(); - if (fetchErr || !order) return res.status(404).json({ error: 'Order not found' }); + if (fetchErr || !order) { + return res.status(404).json({ error: 'Order not found or access denied' }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` around lines 1016 - 1026, The order fetch query in the supabase select statement is missing a customer_id field check, allowing any authenticated customer to access and confirm deposits for orders they don't own. Add customer_id to the select fields in the supabase query, then verify that the fetched order's customer_id matches the authenticated user's customer_id before proceeding with the escrow_status check. This ownership verification should happen after the order is fetched but before returning success, ensuring only the order owner can confirm deposits.
🧹 Nitpick comments (1)
backend/api/src/routes/orderRoutes.js (1)
1040-1043: 💤 Low valueInconsistent logging — use
logger.errorfor consistency.This catch block uses
console.errorwhile the rest of the file uses the importedloggermodule.} catch (err) { - console.error('[confirm-deposit] Exception:', err.message); + logger.error('[confirm-deposit] Exception:', err.message); res.status(500).json({ error: 'Internal Server Error' }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` around lines 1040 - 1043, Replace the `console.error` call in the catch block with `logger.error` to maintain consistency with the logging pattern used throughout the rest of the orderRoutes.js file. The catch block that handles the '[confirm-deposit]' exception should use the imported logger module instead of console for error logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 669-704: The code continues to accept the bid via the
accept_bid_tx RPC call even when buildDepositTx returns null, leaving the escrow
flow broken and the order in an inconsistent state. After calling buildDepositTx
when driverWallet and customerWallet are present, add an early validation check:
if txData is null or undefined, return an error response with appropriate status
and message before proceeding to the accept_bid_tx RPC call. This ensures the
bid is only accepted when the escrow deposit transaction has been successfully
prepared.
---
Outside diff comments:
In `@blockchain/contracts/Escrow.sol`:
- Around line 60-67: The deposit function accepts customer and driver as
caller-supplied parameters, allowing an attacker to self-assign as customer for
any bookingId and block the real customer from depositing due to the "Escrow
exists" check. To fix this, implement pre-authorization for booking parties by
either adding a prior registration/approval mechanism (e.g., a createBooking
function that records authorized customer/driver pairs for each bookingId before
deposit can be called) or add signature-based authorization that verifies the
caller has signed the specific bookingId, customer, driver, and amount
parameters. Validate these pre-authorized parties early in the deposit function
before accepting the funds, ensuring only authorized participants can create new
escrows.
---
Duplicate comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 1010-1012: The Zod validation library is not imported but the code
uses z.object() and z.string() in the validateBody call for the confirm-deposit
route handler, which will cause a ReferenceError. Add the missing import
statement for Zod at the top of the file, or alternatively move the inline
schema object containing the txHash validation to a separate requestSchemas.js
file and import it instead of defining it inline.
- Around line 1033-1038: The supabase.from('orders').update() call that sets
escrow_status to 'funded' lacks error handling, which could leave the database
state inconsistent with blockchain state if the update fails. Add error handling
around this update operation by either wrapping it in a try-catch block or
checking the response object returned by the update operation. If the update
fails, ensure you log the error appropriately and either throw an error to
prevent further execution or implement a recovery mechanism to maintain
consistency between the on-chain deposit and the backend order record.
- Around line 1016-1026: The order fetch query in the supabase select statement
is missing a customer_id field check, allowing any authenticated customer to
access and confirm deposits for orders they don't own. Add customer_id to the
select fields in the supabase query, then verify that the fetched order's
customer_id matches the authenticated user's customer_id before proceeding with
the escrow_status check. This ownership verification should happen after the
order is fetched but before returning success, ensuring only the order owner can
confirm deposits.
---
Nitpick comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Around line 1040-1043: Replace the `console.error` call in the catch block
with `logger.error` to maintain consistency with the logging pattern used
throughout the rest of the orderRoutes.js file. The catch block that handles the
'[confirm-deposit]' exception should use the imported logger module instead of
console for error logging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fed26cee-8172-4b60-a228-ef18891ba8f0
📒 Files selected for processing (4)
backend/api/src/routes/orderRoutes.jsbackend/api/src/services/escrow.jsblockchain/contracts/Escrow.solblockchain/test/escrow.test.cjs
🚧 Files skipped from review as they are similar to previous changes (2)
- blockchain/test/escrow.test.cjs
- backend/api/src/services/escrow.js
- Add import { z } from 'zod' to orderRoutes.js for confirm-deposit validation
- Add wallet addresses + escrow mock to two bid-acceptance RPC error tests
(upstream wallet validation gate returns 422 when wallets missing)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/test/integration/bids.test.js`:
- Around line 626-631: Update the test fixtures to align with the new deposit
contract API. First, change the mockEscrowDeposit.mockResolvedValue at line 631
and the duplicate section around line 672 to return { txData } instead of {
txHash }, since bid-accept now consumes buildDepositTx() with a txData property.
Second, replace the invalid EVM address placeholders (0xCustomerWallet and
0xDriverWallet) in the polygon_wallet_address fields at lines 627, 630, 668, and
671 with valid 42-character hexadecimal EVM addresses (e.g., 0x followed by 40
hex characters) so the tests actually exercise the intended funding path with
proper wallet addresses.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77a837c0-7508-4204-a4a3-ac45a87d6880
📒 Files selected for processing (2)
backend/api/src/routes/orderRoutes.jsbackend/api/test/integration/bids.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/api/src/routes/orderRoutes.js
- Add missing zod import (z) to orderRoutes.js for confirm-deposit validation - Rewrite bid test mock to use buildDepositTx/recordDepositTx instead of deprecated escrowDeposit (route was rewritten for issue KanishJebaMathewM#595) - Rewrite escrow.test.js to test buildDepositTx/recordDepositTx instead of removed escrowDeposit function - Fix Flutter analysis: replace undefined _client/_httpClient in ProfileService.logout() with SupabaseService.client and _apiClient - Fix Flutter analysis: remove duplicate http import in profile_screen.dart - Fix Flutter analysis: remove unnecessary null check in marketplace_repository
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/customer/lib/services/profile_service.dart`:
- Around line 45-52: The logout API call in the profile service is missing a
timeout mechanism that was present in the previous implementation. Add a timeout
to the _apiClient.post call to the '/api/auth/logout' endpoint by wrapping the
await statement with .timeout(Duration(seconds: 5)) to prevent the logout
operation from hanging indefinitely if the network is unresponsive or the server
is unreachable. Ensure you handle TimeoutException if needed for proper error
handling during logout failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b92ca46-5c8f-463f-92c7-f1bb960c9253
📒 Files selected for processing (5)
apps/customer/lib/services/profile_service.dartapps/driver/lib/screens/profile_screen.dartapps/driver/lib/services/marketplace_repository.dartbackend/api/test/integration/bids.test.jsbackend/api/test/integration/escrow.test.js
💤 Files with no reviewable changes (1)
- apps/driver/lib/screens/profile_screen.dart
Contributor
Author
|
@KanishJebaMathewM Please review this and add labels |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Escrow contract's
deposit()had no access control — any wallet (including the backend's relayer) could call it. The relayer funded escrows with its own MATIC and never recouped the cost, creating a financial drain. The backend held sole responsibility for deposits, making it a single point of failure.Solution
require(msg.sender == customer, "Only customer can deposit")to prevent non-customer wallets from depositingescrowDeposit()withbuildDepositTx()(returns unsigned tx data for customer to sign) andrecordDepositTx()(verifies on-chain deposit after customer submits)POST /orders/:id/confirm-depositfor customer to confirm after their wallet signsFiles changed
blockchain/contracts/Escrow.sol— requiremsg.sender == customerin depositblockchain/test/escrow.test.cjs— added non-customer deposit rejection testbackend/api/src/services/escrow.js— newbuildDepositTx/recordDepositTxAPIbackend/api/src/routes/orderRoutes.js— updated bid acceptance, added confirm-deposit routeCloses #595
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Tests