-
Notifications
You must be signed in to change notification settings - Fork 6
Fix: Facilitator: Missing validation for facilitatorFee <= value causes failed transactions #201
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
Changes from 1 commit
050b06f
f4a76db
73de1a9
8831510
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 |
|---|---|---|
|
|
@@ -602,9 +602,10 @@ export async function settleWithRouter( | |
| } | ||
| } | ||
|
|
||
| // 8. Defensive balance check (verify stage should have already caught this) | ||
| // 8. Defensive balance check and fee validation (applies to ALL networks) | ||
| if (balanceChecker) { | ||
| try { | ||
| // 8.1 Check user balance | ||
| const balanceCheck = await balanceChecker.checkBalance( | ||
| signer as any, // Signer has readContract method needed for balance checks | ||
| authorization.from as `0x${string}`, | ||
|
|
@@ -632,29 +633,78 @@ export async function settleWithRouter( | |
| network: paymentRequirements.network, | ||
| payer: authorization.from, | ||
| }; | ||
| } else { | ||
| logger.debug( | ||
| } | ||
|
|
||
| // 8.2 NEW: Validate facilitatorFee <= value (critical for ALL networks) | ||
| const fee = BigInt(settlementParams.facilitatorFee); | ||
| const paymentValue = BigInt(authorization.value); | ||
|
|
||
| if (fee > paymentValue) { | ||
| logger.error( | ||
| { | ||
| payer: authorization.from, | ||
| network, | ||
| balance: balanceCheck.balance, | ||
| required: balanceCheck.required, | ||
| cached: balanceCheck.cached, | ||
| facilitatorFee: settlementParams.facilitatorFee, | ||
| value: authorization.value, | ||
| ratio: (Number(fee) / Number(paymentValue)).toFixed(2), | ||
| }, | ||
| "Facilitator fee exceeds payment amount - rejecting transaction (applies to all networks)", | ||
| ); | ||
|
|
||
| return { | ||
| success: false, | ||
| errorReason: "FACILITATOR_FEE_EXCEEDS_VALUE" as any, | ||
| transaction: "", | ||
| network: paymentRequirements.network, | ||
| payer: authorization.from, | ||
| }; | ||
| } | ||
|
||
|
|
||
| // 8.3 NEW: Warn if fee ratio is suspicious (> 50% for any network) | ||
| const feeRatio = paymentValue > 0n ? (Number(fee) / Number(paymentValue)) * 100 : 0; | ||
|
||
| if (feeRatio > 50) { | ||
| logger.warn( | ||
| { | ||
| payer: authorization.from, | ||
| network, | ||
| facilitatorFee: settlementParams.facilitatorFee, | ||
| value: authorization.value, | ||
| feeRatio: `${feeRatio.toFixed(1)}%`, | ||
| }, | ||
| "Balance check passed during settlement (defensive check)", | ||
| "High facilitator fee ratio detected - possible gas price issue (network-agnostic check)", | ||
| ); | ||
| } | ||
|
|
||
| logger.debug( | ||
| { | ||
| payer: authorization.from, | ||
| network, | ||
| balance: balanceCheck.balance, | ||
| required: balanceCheck.required, | ||
| facilitatorFee: settlementParams.facilitatorFee, | ||
| feeRatio: `${feeRatio.toFixed(2)}%`, | ||
| cached: balanceCheck.cached, | ||
| }, | ||
| "Balance check and fee validation passed", | ||
| ); | ||
| } catch (error) { | ||
| // FIXED: Don't proceed with transaction if validation fails (affects ALL networks) | ||
| logger.error( | ||
| { | ||
| error, | ||
| payer: authorization.from, | ||
| network, | ||
| }, | ||
| "Balance check failed during settlement, proceeding with transaction", | ||
| "Balance or fee validation failed - rejecting transaction (network-agnostic)", | ||
| ); | ||
| // If balance check fails, we continue with the transaction | ||
| // This ensures settlement can still work even if balance check has issues | ||
|
|
||
| return { | ||
| success: false, | ||
| errorReason: "VALIDATION_FAILED" as any, | ||
| transaction: "", | ||
| network: paymentRequirements.network, | ||
| payer: authorization.from, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,11 +22,23 @@ vi.mock("viem", async () => { | |||||||||||||||||||||||||
| const actual = await vi.importActual("viem"); | ||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||
| ...actual, | ||||||||||||||||||||||||||
| parseErc6492Signature: vi.fn((sig: string) => ({ | ||||||||||||||||||||||||||
| signature: sig, | ||||||||||||||||||||||||||
| address: "0x0000000000000000000000000000000000000000", | ||||||||||||||||||||||||||
| data: "0x", | ||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||
| parseErc6492Signature: vi.fn((sig: string | any) => { | ||||||||||||||||||||||||||
| // Handle both hex string and old {v, r, s} format | ||||||||||||||||||||||||||
| if (typeof sig === "string") { | ||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||
| signature: sig, | ||||||||||||||||||||||||||
| address: "0x0000000000000000000000000000000000000000", | ||||||||||||||||||||||||||
| data: "0x", | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| // Old format {v, r, s} | ||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||
| signature: `0x${sig.r.slice(2)}${sig.s.slice(2)}${sig.v.toString(16).padStart(2, "0")}`, | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| return { | |
| signature: `0x${sig.r.slice(2)}${sig.s.slice(2)}${sig.v.toString(16).padStart(2, "0")}`, | |
| const vValue = | |
| typeof sig.v === "string" | |
| ? parseInt(sig.v, 16) | |
| : Number(sig.v); | |
| if (Number.isNaN(vValue)) { | |
| throw new Error("Invalid v value in mock parseErc6492Signature"); | |
| } | |
| const vHex = vValue.toString(16).padStart(2, "0"); | |
| return { | |
| signature: `0x${sig.r.slice(2)}${sig.s.slice(2)}${vHex}`, |
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.
✅ FIXED: Fixed the parseErc6492Signature mock to handle both numeric and string hex values for the v component. Added proper validation and parseInt for string values, with error handling for invalid v values.
Action taken: Added proper type checking and validation for sig.v parameter to handle both string and number formats
Outdated
Copilot
AI
Jan 10, 2026
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.
The variable name "paymentWithHighFee" is misleading in this test case because the test is verifying that fee equals value (100% fee edge case), not testing a normal payment scenario. Consider renaming to something more descriptive like "paymentWithEqualFee" or "paymentWithMaxFee" to better reflect the test scenario.
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.
✅ FIXED: Renamed misleading variable name 'paymentWithHighFee' to 'paymentWithEqualFee' in the test case where fee equals value (100% fee edge case). This makes the test intent clearer.
Action taken: Renamed variable from 'paymentWithHighFee' to 'paymentWithEqualFee' for fee == value test case
Outdated
Copilot
AI
Jan 10, 2026
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.
The variable name "requirementsWithHighFee" is misleading in this test case because the test is verifying that fee equals value (100% fee edge case), not a high fee scenario specifically. Consider renaming to something more descriptive like "requirementsWithEqualFee" or "requirementsWithMaxFee" to better reflect the test scenario.
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.
✅ FIXED: Renamed misleading variable name 'requirementsWithHighFee' to 'requirementsWithEqualFee' in the test case where fee equals value (100% fee edge case). This makes the test intent clearer.
Action taken: Renamed variable from 'requirementsWithHighFee' to 'requirementsWithEqualFee' for fee == value test case
Outdated
Copilot
AI
Jan 10, 2026
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.
The variable name "paymentWithHighFee" is misleading in this test case because the test is verifying normal operation where fee is less than value (1% fee), not a high fee scenario. Consider renaming to something more descriptive like "paymentWithNormalFee" or "paymentPayload" to better reflect the test scenario.
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.
✅ FIXED: Renamed misleading variable name 'paymentWithHighFee' to 'paymentWithNormalFee' in the test case where fee is less than value (1% fee, normal case). This makes the test intent clearer.
Action taken: Renamed variable from 'paymentWithHighFee' to 'paymentWithNormalFee' for fee < value test case
Outdated
Copilot
AI
Jan 10, 2026
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.
The variable name "requirementsWithHighFee" is misleading in this test case because the test is verifying normal operation where fee is less than value (1% fee), not a high fee scenario. Consider renaming to something more descriptive like "requirementsWithNormalFee" or "paymentRequirements" to better reflect the test scenario.
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.
✅ FIXED: Renamed misleading variable name 'requirementsWithHighFee' to 'requirementsWithNormalFee' in the test case where fee is less than value (1% fee, normal case). This makes the test intent clearer.
Action taken: Renamed variable from 'requirementsWithHighFee' to 'requirementsWithNormalFee' for fee < value test case
Copilot
AI
Jan 10, 2026
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.
This test verifies that zero-value payments with zero fees don't trigger the fee validation error, but zero-value payments may be problematic for other reasons (what is being settled if nothing has value?). Consider whether zero-value payments should be rejected earlier in the flow, or if this is an intentional valid case. If zero-value is invalid, this test should expect an error.
| // Should not fail with fee validation error | |
| expect(result.errorReason).not.toBe("FACILITATOR_FEE_EXCEEDS_VALUE"); | |
| // Zero-value payments should be rejected by validation | |
| expect(result.success).toBe(false); | |
| expect(result.errorReason).toBe("VALIDATION_FAILED"); |
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.
Action taken: No change - zero-value payments are intentionally valid per x402 protocol specification
Copilot
AI
Jan 10, 2026
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.
The test suite is missing a critical test case: when payment value is 0 but facilitatorFee > 0. This scenario should be rejected by the fee validation (fee > value), and a test should verify this behavior to ensure the edge case is properly handled.
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.
✅ FIXED: Added test case for zero value payment with non-zero facilitator fee at facilitator/test/unit/settlement.test.ts:519-552. The test verifies that payments with value=0 and fee>0 are correctly rejected with FACILITATOR_FEE_EXCEEDS_VALUE error.
Action taken: Added new test case 'should reject transaction when value is 0 and facilitatorFee > 0' in the fee validation test suite
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.
Converting very large BigInt values to Number using
Number(fee)andNumber(paymentValue)can lose precision for values larger than Number.MAX_SAFE_INTEGER (2^53 - 1). For tokens with high decimals or large amounts, this could result in incorrect ratio calculations.Consider using BigInt arithmetic instead:
(fee * 100n) / paymentValueto get the ratio as a percentageThere 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.
✅ FIXED: Fixed BigInt precision loss by using BigInt arithmetic for ratio calculations. Changed from Number(fee)/Number(paymentValue) to (fee * 10000n) / paymentValue to calculate basis points, then only converting to Number for final display. This prevents precision loss for large token values.
Action taken: Replaced Number conversion with BigInt arithmetic: (fee * 10000n) / paymentValue for basis points calculation