-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Implement balance checks for L2PS transactions to ensure suffic… #680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: testnet
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,9 +20,12 @@ | |
| import { getSharedState } from "src/utilities/sharedState" | ||
| import log from "src/utilities/logger" | ||
| import { Operation, ValidityData } from "@kynesyslabs/demosdk/types" | ||
| import type { INativePayload } from "@kynesyslabs/demosdk/types" | ||
| import { forgeToHex } from "src/libs/crypto/forgeUtils" | ||
| import _ from "lodash" | ||
| import { ucrypto, uint8ArrayToHex } from "@kynesyslabs/demosdk/encryption" | ||
| import ParallelNetworks from "@/libs/l2ps/parallelNetworks" | ||
| import L2PSTransactionExecutor, { L2PS_TX_FEE } from "@/libs/l2ps/L2PSTransactionExecutor" | ||
|
|
||
| // INFO Cryptographically validate a transaction and calculate gas | ||
| // REVIEW is it overkill to write an interface for the return value? | ||
|
|
@@ -99,6 +102,37 @@ | |
| return validityData | ||
| } | ||
|
|
||
| log.debug("[TX] confirmTransaction - Transaction validity verified, compiling ValidityData") | ||
|
|
||
| // Check sender balance covers the transfer amount | ||
| 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 | ||
| } | ||
| } | ||
|
|
||
| // For L2PS encrypted transactions, decrypt inner tx and check balance | ||
| if (tx.content.type === "l2psEncryptedTx") { | ||
| const l2psBalanceError = await checkL2PSBalance(tx) | ||
| if (l2psBalanceError) { | ||
| validityData.data.message = l2psBalanceError | ||
| validityData.data.valid = false | ||
| validityData = await signValidityData(validityData) | ||
| return validityData | ||
| } | ||
| } | ||
|
|
||
| log.debug( | ||
| "[TX] confirmTransaction - Transaction validity verified, compiling ValidityData", | ||
| ) | ||
|
|
@@ -109,6 +143,49 @@ | |
| return validityData | ||
| } | ||
|
|
||
| /** | ||
| * Decrypt L2PS encrypted tx and check inner tx balance before mempool. | ||
| * Returns error message string if insufficient, null if OK. | ||
| */ | ||
| 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) return null // Can't check without UID, let handleL2PS catch it | ||
|
|
||
| const parallelNetworks = ParallelNetworks.getInstance() | ||
| let l2psInstance = await parallelNetworks.getL2PS(l2psUid) | ||
| if (!l2psInstance) { | ||
| l2psInstance = await parallelNetworks.loadL2PS(l2psUid) | ||
| } | ||
| if (!l2psInstance) return null // No L2PS config, let handleL2PS catch it | ||
|
|
||
| const decryptedTx = await l2psInstance.decryptTx(tx as any) | ||
| if (!decryptedTx?.content?.from) return null | ||
|
|
||
| const sender = decryptedTx.content.from as string | ||
|
|
||
| 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 | ||
| const balance = await L2PSTransactionExecutor.getBalance(sender) | ||
| if (balance < BigInt(totalRequired)) { | ||
| return `[Tx Validation] [BALANCE ERROR] Insufficient balance: need ${totalRequired} but have ${balance}\n` | ||
|
Comment on lines
+168
to
+180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apply the L2PS fee only to inner Line 177 charges Suggested fix- 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
+ if (decryptedTx.content.type !== "native" || !Array.isArray(decryptedTx.content.data)) {
+ return null
+ }
+ const nativePayload = decryptedTx.content.data[1] as INativePayload
+ if (nativePayload?.nativeOperation !== "send") {
+ return null
+ }
+ const [, sendAmount] = nativePayload.args as [string, number]
+ if (!Number.isInteger(sendAmount) || sendAmount <= 0) {
+ return "[Tx Validation] [BALANCE ERROR] Invalid amount: must be a positive integer\n"
+ }
+
+ const totalRequired = BigInt(sendAmount) + BigInt(L2PS_TX_FEE)
const balance = await L2PSTransactionExecutor.getBalance(sender)
- if (balance < BigInt(totalRequired)) {
+ if (balance < totalRequired) {
return `[Tx Validation] [BALANCE ERROR] Insufficient balance: need ${totalRequired} but have ${balance}\n`
}🤖 Prompt for AI Agents |
||
| } | ||
| } catch (error) { | ||
| log.error(`[confirmTransaction] L2PS balance pre-check error: ${error instanceof Error ? error.message : error}`) | ||
|
Check warning on line 183 in src/libs/blockchain/routines/validateTransaction.ts
|
||
| // Don't block on decryption errors — let handleL2PS deal with it | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| async function signValidityData(data: ValidityData): Promise<ValidityData> { | ||
| const hash = Hashing.sha256(JSON.stringify(data.data)) | ||
| // return data | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| import _ from "lodash" | ||
| import log from "src/utilities/logger" | ||
|
|
||
| export async function manageExecution( | ||
|
Check failure on line 11 in src/libs/network/manageExecution.ts
|
||
| content: BundleContent, | ||
| sender: string, | ||
| ): Promise<RPCResponse> { | ||
|
|
@@ -79,6 +79,9 @@ | |
| returnValue.response = result.response | ||
| returnValue.require_reply = result.require_reply | ||
| returnValue.extra = result.extra | ||
| if (!result.success) { | ||
| log.error(`[SERVER] broadcastTx FAILED — returning to client: result=${returnValue.result}, extra=${JSON.stringify(returnValue.extra)}, response.extra=${result.response?.extra}`) | ||
| } | ||
|
Comment on lines
+82
to
+84
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Logging can throw on failure 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
Comment on lines
+82
to
+84
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: wc -l src/libs/network/manageExecution.tsRepository: kynesyslabs/node Length of output: 99 🏁 Script executed: cat -n src/libs/network/manageExecution.ts | sed -n '70,100p'Repository: kynesyslabs/node Length of output: 1830 🏁 Script executed: cat -n src/libs/network/manageExecution.ts | sed -n '1,85p'Repository: kynesyslabs/node Length of output: 4616 🏁 Script executed: cat -n src/libs/network/manageExecution.ts | head -121Repository: kynesyslabs/node Length of output: 6196 🏁 Script executed: rg "handleExecuteTransaction" -A 5 --type tsRepository: kynesyslabs/node Length of output: 9676 🏁 Script executed: fd -name "endpointHandlers*" -o -name "*types*"Repository: kynesyslabs/node Length of output: 290 🏁 Script executed: rg "type ExecutionResult|interface ExecutionResult" -A 10 --type tsRepository: kynesyslabs/node Length of output: 42 🏁 Script executed: rg "result\.extra\s*=" src/libs/network/endpointHandlers.ts | head -20Repository: kynesyslabs/node Length of output: 530 🏁 Script executed: rg "result\.extra\s*=" src/libs/network/endpointHandlers.ts -A 2 -B 1Repository: kynesyslabs/node Length of output: 1751 🏁 Script executed: rg "type|interface" src/libs/network/endpointHandlers.ts | grep -i executionRepository: kynesyslabs/node Length of output: 42 🏁 Script executed: rg "ExecutionResult" src/libs/network/endpointHandlers.ts -B 5 -A 5 | head -50Repository: kynesyslabs/node Length of output: 1129 🏁 Script executed: find . -name "*.ts" -type f | xargs rg "type ExecutionResult|interface ExecutionResult" 2>/dev/nullRepository: kynesyslabs/node Length of output: 44 🏁 Script executed: rg "ExecutionResult" --type ts | grep "import\|type\|interface"Repository: kynesyslabs/node Length of output: 139 🏁 Script executed: cat -n src/libs/network/endpointHandlers.ts | sed -n '1,20p'Repository: kynesyslabs/node Length of output: 936 🏁 Script executed: rg "from.*demosdk" src/libs/network/endpointHandlers.tsRepository: kynesyslabs/node Length of output: 510 🏁 Script executed: find . -path ./node_modules -prune -o -name "types.ts" -o -name "types.d.ts" | grep -v node_modulesRepository: kynesyslabs/node Length of output: 245 🏁 Script executed: rg "ExecutionResult" src/libs/network/endpointHandlers.ts -B 10 | head -30Repository: kynesyslabs/node Length of output: 1372 🏁 Script executed: cat -n src/libs/network/endpointHandlers.ts | sed -n '1,50p'Repository: kynesyslabs/node Length of output: 2466 🏁 Script executed: rg "BigInt" src/libs/network --type ts | head -20Repository: kynesyslabs/node Length of output: 490 🏁 Script executed: rg "BigInt" . --type ts | grep -v node_modules | head -20Repository: kynesyslabs/node Length of output: 2452 🏁 Script executed: rg "JSON\.stringify.*extra" src/libs/network --type tsRepository: kynesyslabs/node Length of output: 294 🏁 Script executed: rg "toJSON|replacer" src/libs/network --type ts | head -20Repository: kynesyslabs/node Length of output: 152 Serialize Line 83 calls 🤖 Prompt for AI Agents |
||
| break | ||
| } catch (error) { | ||
| const errorMessage = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| import type { BlockContent, L2PSTransaction, RPCResponse } from "@kynesyslabs/demosdk/types" | ||
| import type { BlockContent, L2PSTransaction, RPCResponse, INativePayload } from "@kynesyslabs/demosdk/types" | ||
| import Chain from "src/libs/blockchain/chain" | ||
| import Transaction from "src/libs/blockchain/transaction" | ||
| import { emptyResponse } from "../../server_rpc" | ||
|
|
||
| import { L2PS, L2PSEncryptedPayload } from "@kynesyslabs/demosdk/l2ps" | ||
| import ParallelNetworks from "@/libs/l2ps/parallelNetworks" | ||
| import L2PSMempool from "@/libs/blockchain/l2ps_mempool" | ||
| import L2PSTransactionExecutor from "@/libs/l2ps/L2PSTransactionExecutor" | ||
| import L2PSTransactionExecutor, { L2PS_TX_FEE } from "@/libs/l2ps/L2PSTransactionExecutor" | ||
| import log from "@/utilities/logger" | ||
|
|
||
| /** | ||
|
|
@@ -72,6 +72,38 @@ async function decryptAndValidate( | |
| } | ||
|
|
||
|
|
||
|
|
||
| /** | ||
| * Check sender balance before mempool insertion. | ||
| * Returns an error message if balance is insufficient, null if OK. | ||
| */ | ||
| 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"}` | ||
| } | ||
|
Comment on lines
+80
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. L2ps fee/balance check inconsistent 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
|
||
|
|
||
| return null | ||
| } | ||
|
Comment on lines
+80
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only charge this pre-check on decrypted Line 94 adds Suggested fix 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
+ if (decryptedTx.content.type !== "native" || !Array.isArray(decryptedTx.content.data)) {
+ return null
+ }
+
+ const nativePayload = decryptedTx.content.data[1] as INativePayload
+ if (nativePayload?.nativeOperation !== "send") {
+ return null
+ }
+
+ const [, sendAmount] = nativePayload.args as [string, number]
+ if (!Number.isInteger(sendAmount) || sendAmount <= 0) {
+ return "Invalid amount: must be a positive integer"
+ }
+
+ const totalRequired = BigInt(sendAmount) + BigInt(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}`
+ if (balance < totalRequired) {
+ return `Insufficient balance: need ${totalRequired} (${sendAmount} + ${L2PS_TX_FEE} fee) but have ${balance}`
}🤖 Prompt for AI Agents |
||
|
|
||
| export default async function handleL2PS( | ||
| l2psTx: L2PSTransaction, | ||
| ): Promise<RPCResponse> { | ||
|
|
@@ -111,6 +143,13 @@ export default async function handleL2PS( | |
| return createErrorResponse(response, 400, `Decrypted transaction hash mismatch: expected ${originalHash}, got ${decryptedTx.hash}`) | ||
| } | ||
|
|
||
| // Pre-check sender balance BEFORE mempool insertion | ||
| const balanceError = await checkSenderBalance(decryptedTx) | ||
| if (balanceError) { | ||
| log.error(`[handleL2PS] Balance pre-check failed: ${balanceError}`) | ||
| return createErrorResponse(response, 400, balanceError) | ||
| } | ||
|
Comment on lines
+146
to
+151
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
mempool_file="$(fd 'l2ps_mempool\.ts$' | head -n1)"
test -n "$mempool_file"
echo "== $mempool_file =="
sed -n '1,260p' "$mempool_file"
echo
echo "== reservation-related matches =="
rg -n -C4 '\b(addTransaction|existsByOriginalHash|updateGCREdits|updateStatus)\s*\(' "$mempool_file"
rg -n -C4 '\b(balance|pending|sender|from_address|original_hash)\b' "$mempool_file"Repository: kynesyslabs/node Length of output: 15354 🏁 Script executed: # Check if there are any sender/balance-related fields or methods in l2ps_mempool.ts beyond line 260
rg -n 'sender|balance|pending|debit|reserve|from_address' src/libs/blockchain/l2ps_mempool.tsRepository: kynesyslabs/node Length of output: 494 🏁 Script executed: # Find and examine checkSenderBalance function
rg -n 'checkSenderBalance' --type ts -A 15 src/Repository: kynesyslabs/node Length of output: 3097 🏁 Script executed: # Find processValidL2PSTransaction and L2PSTransactionExecutor
fd '.*\.(ts|tsx)$' src/ | xargs rg -l 'processValidL2PSTransaction|L2PSTransactionExecutor' | head -5Repository: kynesyslabs/node Length of output: 256 🏁 Script executed: # Get full checkSenderBalance implementation
sed -n '80,120p' src/libs/network/routines/transactions/handleL2PS.tsRepository: kynesyslabs/node Length of output: 1573 🏁 Script executed: # Get processValidL2PSTransaction implementation
sed -n '160,220p' src/libs/network/routines/transactions/handleL2PS.tsRepository: kynesyslabs/node Length of output: 2400 🏁 Script executed: # Check L2PSTransactionExecutor for balance handling
rg -n 'class L2PSTransactionExecutor|execute.*function|balance|sender|debit' src/libs/l2ps/L2PSTransactionExecutor.ts -A 3 | head -80Repository: kynesyslabs/node Length of output: 3571 Concurrent sends from same sender can bypass the balance guard due to TOCTOU vulnerability. The balance check at line 147 reads the current L1 state once, but between this check and actual balance deduction in Implement sender-scoped pending debit tracking: either reserve amounts in the mempool as transactions are added, or introduce sender-level locking to serialize balance checks and execution for the same sender. 🤖 Prompt for AI Agents |
||
|
|
||
| // Process Valid Transaction | ||
| return await processValidL2PSTransaction(response, l2psUid, l2psTx, decryptedTx, originalHash) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't fail open when the L2PS balance cannot be verified.
Lines 154-164 and 182-186 return
nullwhenever the inner tx cannot be inspected (!l2psUid, missing/unloaded L2PS, missingfrom, or any thrown error).confirmTransaction()interpretsnullas “no balance error”, so this guard silently disappears under operational failures and can still produce signedValidityDatawithvalid = true.Also applies to: 182-186
🤖 Prompt for AI Agents