feat: Implement balance checks for L2PS transactions to ensure suffic…#680
Closed
Shitikyan wants to merge 3 commits into
Closed
feat: Implement balance checks for L2PS transactions to ensure suffic…#680Shitikyan wants to merge 3 commits into
Shitikyan wants to merge 3 commits into
Conversation
…ient funds before processing
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoImplement balance checks for L2PS and regular transactions WalkthroughsDescription• Add balance validation for L2PS encrypted transactions before mempool insertion • Implement sender balance checks for regular transactions to prevent insufficient funds • Export L2PS_TX_FEE constant for reuse across modules • Add error logging for failed broadcast transactions File Changes1. src/libs/blockchain/routines/validateTransaction.ts
|
|
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Code Review by Qodo
1. Logging can throw on failure
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+82
to
+84
| if (!result.success) { | ||
| log.error(`[SERVER] broadcastTx FAILED — returning to client: result=${returnValue.result}, extra=${JSON.stringify(returnValue.extra)}, response.extra=${result.response?.extra}`) | ||
| } |
Contributor
There was a problem hiding this comment.
1. Logging can throw on failure 🐞 Bug ⛯ Reliability
In broadcastTx failure handling, manageExecution JSON.stringifies returnValue.extra; if extra is a non-serializable object (e.g., contains circular refs or BigInt), the log statement can throw and break the error-handling path (client may not receive the intended error response).
Agent Prompt
### Issue description
`manageExecution` logs `broadcastTx` failures using `JSON.stringify(returnValue.extra)`. Because `extra` can be an object (and potentially non-JSON-serializable), the logging line can throw and break the error path.
### Issue Context
`returnValue.extra` is populated from `result.extra`, and `handleExecuteTransaction` assigns objects to `result.extra` in multiple branches.
### Fix Focus Areas
- src/libs/network/manageExecution.ts[69-85]
### Suggested fix
- Wrap the stringify in a try/catch and fall back to a safe representation.
- Optionally implement a small `safeStringify` helper that:
- converts `bigint` to string
- handles circular references (or falls back to `util.inspect`)
- truncates overly large outputs
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+102
| async function checkSenderBalance(decryptedTx: Transaction): Promise<string | null> { | ||
| const sender = decryptedTx.content.from as string | ||
| if (!sender) return "Missing sender address in decrypted transaction" | ||
|
|
||
| // Extract amount from native payload | ||
| let amount = 0 | ||
| if (decryptedTx.content.type === "native" && Array.isArray(decryptedTx.content.data)) { | ||
| const nativePayload = decryptedTx.content.data[1] as INativePayload | ||
| if (nativePayload?.nativeOperation === "send") { | ||
| const [, sendAmount] = nativePayload.args as [string, number] | ||
| amount = sendAmount || 0 | ||
| } | ||
| } | ||
|
|
||
| const totalRequired = amount + L2PS_TX_FEE | ||
| try { | ||
| const balance = await L2PSTransactionExecutor.getBalance(sender) | ||
| if (balance < BigInt(totalRequired)) { | ||
| return `Insufficient balance: need ${totalRequired} (${amount} + ${L2PS_TX_FEE} fee) but have ${balance}` | ||
| } | ||
| } catch (error) { | ||
| return `Balance check failed: ${error instanceof Error ? error.message : "Unknown error"}` | ||
| } |
Contributor
There was a problem hiding this comment.
2. L2ps fee/balance check inconsistent 🐞 Bug ✓ Correctness
The new L2PS balance pre-checks always add L2PS_TX_FEE (even when the executor does not charge any fee for that tx type), and they don’t validate sendAmount like the executor does. This can incorrectly reject L2PS transactions that would otherwise execute successfully (e.g., non-send native ops, or non-native txs with only gcr_edits) and can also produce misleading errors/behavior when the amount is malformed.
Agent Prompt
### Issue description
`checkSenderBalance` / `checkL2PSBalance` currently require `L2PS_TX_FEE` for all decrypted L2PS transactions, but the executor only checks/burns the fee for `nativeOperation === "send"` and may accept other tx types without fees. This mismatch can cause false rejections. Additionally, the pre-check doesn’t validate the extracted `sendAmount` like the executor does.
### Issue Context
- Executor fee+amount checks are implemented in `handleNativeTransaction` for `nativeOperation === "send"`.
- Other tx types (non-native with `gcr_edits`, or unknown native operations) do not burn/check a fee.
### Fix Focus Areas
- src/libs/network/routines/transactions/handleL2PS.ts[80-102]
- src/libs/blockchain/routines/validateTransaction.ts[145-176]
- src/libs/l2ps/L2PSTransactionExecutor.ts[158-220]
### Suggested fix
1. Decide on policy:
- **Option A (match current executor):** Only add `L2PS_TX_FEE` to `totalRequired` for txs where the executor actually burns/charges the fee (currently native `send`).
- **Option B (enforce fee for all L2PS txs):** Update `L2PSTransactionExecutor.generateGCREdits`/handlers to also burn/check the fee for other tx categories, then keep pre-check consistent.
2. Validate amount exactly as executor does before BigInt conversion:
- `typeof sendAmount === 'number' && Number.isFinite(sendAmount) && Number.isInteger(sendAmount) && sendAmount > 0`
3. Compute required balance using BigInt arithmetic:
- `const required = BigInt(sendAmount) + BigInt(L2PS_TX_FEE)`
- Avoid `BigInt(amount + fee)`.
4. Consider extracting a shared helper (e.g., in `L2PSTransactionExecutor`) to compute required balance so pre-check and executor cannot drift.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…x-l2ps-balance-check
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe changes add sender balance validation checks at transaction processing layers. A constant is exported for use across modules, balance verification helpers are introduced in both blockchain and network layers, and error logging is enhanced for broadcast failures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ValidateTransaction
participant GCR
participant L2PS as ParallelNetworks<br/>& L2PSExecutor
participant Mempool
rect rgba(100, 150, 200, 0.5)
Note over ValidateTransaction,GCR: Regular Transaction Validation
Client->>ValidateTransaction: confirmTransaction(tx)
ValidateTransaction->>GCR: getGCRNativeBalance(sender)
GCR-->>ValidateTransaction: balance
alt balance < amount
ValidateTransaction-->>Client: InvalidTransaction (BALANCE ERROR)
else sufficient balance
ValidateTransaction-->>Client: ValidTransaction
end
end
rect rgba(200, 150, 100, 0.5)
Note over ValidateTransaction,L2PS: L2PS Encrypted Transaction Validation
Client->>ValidateTransaction: confirmTransaction(L2PSEncryptedTx)
ValidateTransaction->>L2PS: checkL2PSBalance(tx)
L2PS->>L2PS: decrypt inner transaction
L2PS->>L2PS: extract nativeAmount from decrypted payload
L2PS->>L2PSExecutor: getBalance(sender)
L2PSExecutor-->>L2PS: L2PS balance
alt (nativeAmount + L2PS_TX_FEE) > L2PS balance
L2PS-->>ValidateTransaction: null
ValidateTransaction-->>Client: InvalidTransaction (BALANCE ERROR)
else sufficient L2PS balance
L2PS-->>ValidateTransaction: success
ValidateTransaction-->>Client: ValidTransaction
end
end
rect rgba(150, 200, 100, 0.5)
Note over Client,Mempool: L2PS Network Handler Pre-Check
Client->>Mempool: submitL2PSTx(decryptedTx)
Mempool->>Mempool: checkSenderBalance(decryptedTx)
alt balance check fails
Mempool-->>Client: HTTP 400 Error
else balance sufficient
Mempool->>Mempool: processValidL2PSTransaction()
Mempool-->>Client: Accepted
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/libs/blockchain/routines/validateTransaction.ts`:
- Around line 154-164: The current guards in validateTransaction (checks around
l2psUid, ParallelNetworks.getInstance().getL2PS/loadL2PS, l2psInstance.decryptTx
and decryptedTx?.content?.from) return null on operational failures which
confirmTransaction treats as "no balance error" and allows validity to pass;
change these paths to surface failures instead of failing open: replace the null
returns with thrown errors or a distinct failure result (e.g., an explicit
BalanceVerificationFailure/throw) so the caller (confirmTransaction) can treat
inability to inspect as a verification failure; update both the block around
getL2PS/loadL2PS/decryptTx and the similar logic at lines 182-186 to propagate
errors (via throw or explicit error return) rather than returning null, and
ensure confirmTransaction handles that signal as a balance verification error.
- Around line 168-180: The balance check is charging L2PS_TX_FEE even for
non-native/send payloads because amount defaults to 0; change the logic to only
add L2PS_TX_FEE when the decryptedTx is a native send. Concretely, detect native
send (use the existing decryptedTx.content.type === "native" and
nativePayload?.nativeOperation === "send" check), set amount from
nativePayload.args only in that branch, compute totalRequired = amount +
(isNativeSend ? L2PS_TX_FEE : 0), then call
L2PSTransactionExecutor.getBalance(sender) and compare balance <
BigInt(totalRequired) as before so non-send payloads are not charged the fee.
In `@src/libs/network/manageExecution.ts`:
- Around line 82-84: The log line in manageExecution.ts that logs on broadcastTx
failure uses JSON.stringify on returnValue.extra and can throw for BigInt or
circular structures; change the logging to first produce a safe serializedExtra
by performing defensive serialization (e.g., JSON.stringify wrapped in a
try/catch or a serializer with a replacer that converts BigInt to string and
handles circular refs via a seen Set) and then use that serializedExtra in
log.error; update the failure branch around variables result, returnValue and
the log.error call so any serialization errors are caught and do not mask the
original failure.
In `@src/libs/network/routines/transactions/handleL2PS.ts`:
- Around line 80-105: checkSenderBalance currently always adds L2PS_TX_FEE and
queries getBalance even for non-native-send transactions; restrict the pre-check
so it only runs for decrypted native send payloads: detect
decryptedTx.content.type === "native" and nativePayload?.nativeOperation ===
"send" (as you already parse) and if not a native send, return null immediately;
otherwise compute amount, include L2PS_TX_FEE in totalRequired, call
L2PSTransactionExecutor.getBalance(sender) and compare as before. Ensure
references to L2PS_TX_FEE, checkSenderBalance, decryptedTx.content.type/data,
nativePayload.nativeOperation, and L2PSTransactionExecutor.getBalance are used
to locate and update the logic.
- Around line 146-151: The pre-check in handleL2PS uses checkSenderBalance but
is vulnerable to TOCTOU because concurrent sends can both pass before
L2PSTransactionExecutor.execute applies debits; fix by implementing
sender-scoped pending-debit tracking or locking: when handleL2PS accepts a tx,
reserve the sender's amount in the mempool (add per-sender pendingDebit field)
or acquire a sender-level lock (e.g., by sender ID) before running
checkSenderBalance and before calling L2PSTransactionExecutor.execute to
serialize processing; ensure L2PSTransactionExecutor.execute consults and
updates the same reservation (or requires the lock) and mempool updates remove
or adjust the pendingDebit on commit/rollback so concurrent requests correctly
see reserved amounts.
🪄 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: fbb2f0b1-36aa-45cf-a92c-1479c5785343
📒 Files selected for processing (4)
src/libs/blockchain/routines/validateTransaction.tssrc/libs/l2ps/L2PSTransactionExecutor.tssrc/libs/network/manageExecution.tssrc/libs/network/routines/transactions/handleL2PS.ts
Correctness — fee-bearing scope
- handleL2PS.ts (checkSenderBalance) and validateTransaction.ts
(checkL2PSBalance) both unconditionally added `L2PS_TX_FEE` to the
required-balance check, but the executor only burns that fee inside
`L2PSTransactionExecutor.handleNativeTransaction()` for
`nativeOperation === "send"`. The pre-checks therefore rejected
every non-`native/send` L2PS payload (web2, crosschain, gcr_edits-
only) with a false-negative balance error. Now the fee is added
only when the inner tx is genuinely fee-bearing, and
`sendAmount` is shape-validated up front instead of being silently
coerced to 0 on a malformed payload.
Correctness — fail-closed validate path
- validateTransaction.ts previously returned `null` on every "cannot
verify" outcome (no l2ps_uid, L2PS not loaded, decryption error).
`confirmTransaction` reads `null` as "no balance error", which means
the node signed and broadcast `ValidityData { valid: true }` for txs
it never actually verified — a fail-open hole that turned every
operational failure into a stamp of approval. Every previously-`null`
failure path now returns a precise `[BALANCE ERROR]` string so
`confirmTransaction` surfaces it instead of vouching for the tx.
Correctness — TOCTOU between balance check and executor debit
- handleL2PS.ts now funnels every `(check + insert + execute)`
sequence through a per-sender in-process lock (`withSenderLock`).
Without it, two concurrent broadcasts from the same wallet both
read the same pre-debit balance and both pass the gate; the
executor then debits twice from a wallet that had funds for one.
In-process serialisation is sufficient because every L2PS tx for a
given sender arrives through one node entry point; cross-node
ordering is handled downstream by the mempool + consensus pipeline.
The lock map drops entries once a sender's queue drains, so
long-lived senders don't leak entries.
Reliability — safe failure-path logging
- manageExecution.ts:84 previously called `JSON.stringify(returnValue.extra)`
in the broadcastTx failure-log branch. If `extra` carried a circular
reference or a `BigInt`, stringify itself would throw, abort the
error-handling path, and leave the client without any response.
Replaced with a `safeStringifyExtra()` helper that serialises BigInts
as `"123n"` and degrades to `<unserialisable: ...>` instead of
throwing.
Net `tsc --noEmit --skipLibCheck` delta on this branch:
baseline `origin/testnet`: 36 pre-existing errors
this branch: 36 — 0 net new
Co-Authored-By: Claude Opus 4.7 <[email protected]>
5 tasks
Contributor
Author
|
Superseded by #937 — rebased onto current |
Greptile SummaryThis PR adds balance pre-checks for both native and L2PS encrypted transactions before they are admitted to the mempool, and exports
Confidence Score: 2/5The new balance checks improve correctness but ship with a Map leak in the sender lock and an unguarded destructure that can throw outside the error handler — both on the hot path for every L2PS transaction. The lock cleanup condition compares a freshly-created Promise against one stored in the Map; they will never be the same object, so the Map grows without bound. The args destructure in checkSenderBalance runs before the try/catch, meaning a malformed native/send tx throws an unhandled rejection through the lock. In validateTransaction.ts, getBalance silently creates new L1 accounts as a side effect during a read-only validation pass, and there is no sender lock, leaving a TOCTOU window open on that path. Both handleL2PS.ts and validateTransaction.ts need attention — the lock implementation and unguarded destructure in the former, and the missing lock plus account-creation side effect in the latter. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant handleL2PS
participant withSenderLock
participant checkSenderBalance
participant L2PSTransactionExecutor
participant processValidL2PSTransaction
Client->>handleL2PS: L2PSTransaction
handleL2PS->>handleL2PS: validateL2PSStructure()
handleL2PS->>handleL2PS: getL2PSInstance()
handleL2PS->>handleL2PS: decryptAndValidate()
handleL2PS->>withSenderLock: acquire per-sender lock
withSenderLock->>checkSenderBalance: checkSenderBalance(decryptedTx)
checkSenderBalance->>L2PSTransactionExecutor: getBalance(sender)
L2PSTransactionExecutor-->>checkSenderBalance: BigInt balance
alt Insufficient balance
checkSenderBalance-->>withSenderLock: error string
withSenderLock-->>handleL2PS: createErrorResponse(400)
handleL2PS-->>Client: 400 error
else Balance OK
checkSenderBalance-->>withSenderLock: null
withSenderLock->>processValidL2PSTransaction: process tx
processValidL2PSTransaction->>L2PSTransactionExecutor: execute()
L2PSTransactionExecutor-->>processValidL2PSTransaction: L2PSExecutionResult
processValidL2PSTransaction-->>withSenderLock: RPCResponse 200
withSenderLock-->>handleL2PS: release lock
handleL2PS-->>Client: 200 success
end
Reviews (1): Last reviewed commit: "review: address bot findings on PR #680" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+89
to
+111
| const senderLocks = new Map<string, Promise<void>>() | ||
| async function withSenderLock<T>( | ||
| sender: string, | ||
| fn: () => Promise<T>, | ||
| ): Promise<T> { | ||
| const previous = senderLocks.get(sender) ?? Promise.resolve() | ||
| let release: () => void = () => undefined | ||
| const next = new Promise<void>(res => { | ||
| release = res | ||
| }) | ||
| senderLocks.set(sender, previous.then(() => next)) | ||
| try { | ||
| await previous | ||
| return await fn() | ||
| } finally { | ||
| release() | ||
| // Drop the map entry once the queue has drained so long-lived | ||
| // senders don't leak entries here. | ||
| if (senderLocks.get(sender) === previous.then(() => next)) { | ||
| senderLocks.delete(sender) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
false because Promise.prototype.then() returns a new Promise object on every invocation. The reference stored in senderLocks (call it Promise A) will never === the new Promise B created inline in the finally block. As a result senderLocks.delete(sender) is never called and the Map grows without bound — every sender address ever seen leaks an entry for the lifetime of the process.
Suggested change
| const senderLocks = new Map<string, Promise<void>>() | |
| async function withSenderLock<T>( | |
| sender: string, | |
| fn: () => Promise<T>, | |
| ): Promise<T> { | |
| const previous = senderLocks.get(sender) ?? Promise.resolve() | |
| let release: () => void = () => undefined | |
| const next = new Promise<void>(res => { | |
| release = res | |
| }) | |
| senderLocks.set(sender, previous.then(() => next)) | |
| try { | |
| await previous | |
| return await fn() | |
| } finally { | |
| release() | |
| // Drop the map entry once the queue has drained so long-lived | |
| // senders don't leak entries here. | |
| if (senderLocks.get(sender) === previous.then(() => next)) { | |
| senderLocks.delete(sender) | |
| } | |
| } | |
| } | |
| const senderLocks = new Map<string, Promise<void>>() | |
| async function withSenderLock<T>( | |
| sender: string, | |
| fn: () => Promise<T>, | |
| ): Promise<T> { | |
| const previous = senderLocks.get(sender) ?? Promise.resolve() | |
| let release: () => void = () => undefined | |
| const next = new Promise<void>(res => { | |
| release = res | |
| }) | |
| const thisLock = previous.then(() => next) | |
| senderLocks.set(sender, thisLock) | |
| try { | |
| await previous | |
| return await fn() | |
| } finally { | |
| release() | |
| // Drop the map entry once the queue has drained so long-lived | |
| // senders don't leak entries here. | |
| if (senderLocks.get(sender) === thisLock) { | |
| senderLocks.delete(sender) | |
| } | |
| } | |
| } |
Comment on lines
+143
to
+150
| if (feeBearing) { | ||
| const [, sendAmount] = (decryptedTx.content.data as any[])[1] | ||
| .args as [string, number] | ||
| if (typeof sendAmount !== "number" || !Number.isFinite(sendAmount) || sendAmount < 0) { | ||
| return `Invalid native send amount: ${String(sendAmount)}` | ||
| } | ||
| amount = sendAmount | ||
| } |
There was a problem hiding this comment.
.args destructuring runs before the try/catch that only wraps getBalance. If a native/send tx arrives with args as undefined or a non-iterable, this line throws a TypeError that escapes checkSenderBalance and withSenderLock as an unhandled rejection. Wrapping the entire function body in a try/catch (matching the pattern already used in checkL2PSBalance in validateTransaction.ts) closes this gap.
Comment on lines
+150
to
+219
| async function checkL2PSBalance(tx: Transaction): Promise<string | null> { | ||
| try { | ||
| const l2psPayload = (tx.content?.data as any)?.[1] | ||
| const l2psUid = l2psPayload?.l2ps_uid as string | undefined | ||
| if (!l2psUid) { | ||
| // Fail closed — `confirmTransaction` treats null as | ||
| // "balance OK" and would sign ValidityData claiming the tx | ||
| // is valid even though we never verified it. | ||
| return `[Tx Validation] [BALANCE ERROR] L2PS transaction missing l2ps_uid — cannot verify sender balance\n` | ||
| } | ||
|
|
||
| const parallelNetworks = ParallelNetworks.getInstance() | ||
| let l2psInstance = await parallelNetworks.getL2PS(l2psUid) | ||
| if (!l2psInstance) { | ||
| l2psInstance = await parallelNetworks.loadL2PS(l2psUid) | ||
| } | ||
| if (!l2psInstance) { | ||
| return `[Tx Validation] [BALANCE ERROR] L2PS network ${l2psUid} is not loaded on this node — cannot verify sender balance\n` | ||
| } | ||
|
|
||
| const decryptedTx = await l2psInstance.decryptTx(tx as any) | ||
| if (!decryptedTx?.content?.from) { | ||
| return `[Tx Validation] [BALANCE ERROR] L2PS payload decryption produced no sender — cannot verify balance\n` | ||
| } | ||
|
|
||
| const sender = decryptedTx.content.from as string | ||
|
|
||
| // L2PS_TX_FEE is only burned by the executor on `native` / | ||
| // `send` — see `L2PSTransactionExecutor.handleNativeTransaction()`. | ||
| // Charging the fee for other tx types here would reject valid | ||
| // non-send L2PS payloads (web2, crosschain, gcr_edits-only). | ||
| let amount = 0 | ||
| let feeBearing = false | ||
| if ( | ||
| decryptedTx.content.type === "native" && | ||
| Array.isArray(decryptedTx.content.data) | ||
| ) { | ||
| const nativePayload = decryptedTx.content.data[1] as INativePayload | ||
| if (nativePayload?.nativeOperation === "send") { | ||
| feeBearing = true | ||
| const [, sendAmount] = nativePayload.args as [string, number] | ||
| if ( | ||
| typeof sendAmount !== "number" || | ||
| !Number.isFinite(sendAmount) || | ||
| sendAmount < 0 | ||
| ) { | ||
| return `[Tx Validation] [BALANCE ERROR] Invalid native send amount: ${String(sendAmount)}\n` | ||
| } | ||
| amount = sendAmount | ||
| } | ||
| } | ||
|
|
||
| const fee = feeBearing ? L2PS_TX_FEE : 0 | ||
| const totalRequired = amount + fee | ||
| if (totalRequired === 0) return null | ||
|
|
||
| const balance = await L2PSTransactionExecutor.getBalance(sender) | ||
| if (balance < BigInt(totalRequired)) { | ||
| return `[Tx Validation] [BALANCE ERROR] Insufficient balance: need ${totalRequired} but have ${balance}\n` | ||
| } | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error) | ||
| log.error(`[confirmTransaction] L2PS balance pre-check error: ${message}`) | ||
| // Fail closed — a decryption / load throw means we cannot vouch | ||
| // for the tx; signing ValidityData claiming balance-OK here was | ||
| // the original fail-open hole. | ||
| return `[Tx Validation] [BALANCE ERROR] L2PS balance pre-check failed: ${message}\n` | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
checkL2PSBalance is missing a sender lock and getBalance creates accounts as a side effect. Unlike handleL2PS, this code path has no per-sender mutex, so two concurrent confirmTransaction calls for the same L2PS sender can both pass the balance check concurrently. Additionally, L2PSTransactionExecutor.getBalance calls getOrCreateL1Account internally, which creates a new L1 account if one does not exist — running this during a read-only validation phase is an unintended write that may pollute the GCR with empty accounts from invalid or spam transactions.
Comment on lines
+105
to
+108
| log.debug("[TX] confirmTransaction - Transaction validity verified, compiling ValidityData") | ||
|
|
||
| // Check sender balance covers the transfer amount | ||
| if (tx.content.amount > 0) { |
There was a problem hiding this comment.
confirmTransaction: once immediately after the signature check (line 105) and again after the balance checks (lines 136–138), where it was already present before this PR. The second occurrence is the original, so the first should be removed.
Suggested change
| log.debug("[TX] confirmTransaction - Transaction validity verified, compiling ValidityData") | |
| // Check sender balance covers the transfer amount | |
| if (tx.content.amount > 0) { | |
| // Check sender balance covers the transfer amount | |
| if (tx.content.amount > 0) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+108
to
+123
| if (tx.content.amount > 0) { | ||
| const from = typeof tx.content.from === "string" ? tx.content.from : forgeToHex(tx.content.from) | ||
| let fromBalance = 0 | ||
| try { | ||
| fromBalance = await GCR.getGCRNativeBalance(from) | ||
| } catch { | ||
| // Address not in GCR — balance is 0 | ||
| } | ||
| if (fromBalance < tx.content.amount) { | ||
| validityData.data.message = | ||
| `[Tx Validation] [BALANCE ERROR] Insufficient balance: need ${tx.content.amount} but have ${fromBalance}\n` | ||
| validityData.data.valid = false | ||
| validityData = await signValidityData(validityData) | ||
| return validityData | ||
| } | ||
| } |
There was a problem hiding this comment.
tx.content.amount > 0, only the transfer amount is compared against fromBalance — no fee or gas cost is included. If the sender has exactly tx.content.amount tokens but execution also charges gas (handled by defineGas), this check will sign the transaction as valid even though it will revert at execution time.
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.
…ient funds before processing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes