round: enforce fee cap against realised quote economics (#379) - #452
round: enforce fee cap against realised quote economics (#379)#452ellemouton wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements realized-fee cap enforcement to prevent operators from bypassing fee limits by manipulating output amounts. It introduces the realisedQuoteFee function to calculate the actual economic fee and validates it against the fee cap and the operator's declared fee. Extensive tests were added to cover various attack vectors, such as change underpayment. Feedback was provided to refine the fee calculation by including zero-value outputs and explicitly rejecting negative output amounts to ensure robustness against manipulation.
| for i := range quote.VTXOQuotes { | ||
| amt := quote.VTXOQuotes[i].AmountSat | ||
| if amt > 0 { | ||
| outputsSat += amt | ||
| } | ||
| } | ||
| for i := range quote.LeaveQuotes { | ||
| amt := quote.LeaveQuotes[i].AmountSat | ||
| if amt > 0 { | ||
| outputsSat += amt | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of summing output amounts incorrectly handles zero-value and negative-value outputs.
- Zero-value outputs are valid in Bitcoin (e.g., for
OP_RETURN) and should be included in the total output sum. By ignoring them withif amt > 0, the calculatedrealisedfee will be higher than it should be, which could lead to incorrect rejections of valid quotes. - Negative-value outputs are invalid in a transaction, but a malicious operator could include them in a quote to manipulate the
realisedfee calculation. The current code ignores them. A better approach is to explicitly reject quotes with negative output amounts.
I suggest modifying the loops to include zero-value outputs in the sum and to return an error for negative-value outputs. This will make the fee calculation more robust and secure.
| for i := range quote.VTXOQuotes { | |
| amt := quote.VTXOQuotes[i].AmountSat | |
| if amt > 0 { | |
| outputsSat += amt | |
| } | |
| } | |
| for i := range quote.LeaveQuotes { | |
| amt := quote.LeaveQuotes[i].AmountSat | |
| if amt > 0 { | |
| outputsSat += amt | |
| } | |
| } | |
| for i := range quote.VTXOQuotes { | |
| amt := quote.VTXOQuotes[i].AmountSat | |
| if amt < 0 { | |
| return 0, fmt.Errorf("vtxo quote %d has negative amount %d", i, amt) | |
| } | |
| outputsSat += amt | |
| } | |
| for i := range quote.LeaveQuotes { | |
| amt := quote.LeaveQuotes[i].AmountSat | |
| if amt < 0 { | |
| return 0, fmt.Errorf("leave quote %d has negative amount %d", i, amt) | |
| } | |
| outputsSat += amt | |
| } |
There was a problem hiding this comment.
Applied in 053dae4.
Verdict: applied in full. realisedQuoteFee no longer filters amt > 0; instead it sums all amounts and explicitly rejects negatives on both sides (boarding inputs and VTXO/leave outputs). The forfeit-input branch was already authoritative (sourced from VTXOStore / fallback hint) and doesn't take this code path.
On the Ark protocol question: zero-value VTXO/leave outputs are not a legitimate honest shape — VTXOs and leaves both flow through the dust/output checks elsewhere. So the previous filter was effectively dead defensive code for honest cases. Mathematically a zero contributes nothing to the sum either way; the real correctness fix the comment exposes is that the same filter was silently absorbing negatives, which would shift the realised fee in the operator's favor. The new code fails closed on negatives with a diagnostic instead.
New tests:
TestRealisedFeeIncludesZeroValueOutputs— quote with a zero-value leave output is summed into Σoutputs and the realised fee matches the declared fee.TestRealisedFeeRejectsNegativeOutput— negativeAmountSatis rejected byrealisedQuoteFeeand surfaced viaQuoteRejectedwith a "negative" diagnostic.
Lint: 0 issues (make lint-native). Round suite: green (go test ./round/... -count=1).
The quote.OperatorFeeSat field is operator-attested; a malicious operator could quote a small fee within env.MaxOperatorFee while shaving the IsChange=true output by a much larger delta. The echo validator intentionally permits change-output amount deviation, so without a second gate the client would accept and sign a round whose actual economic fee exceeds the cap. Recompute the realised fee at evaluateQuote time as Σ(authoritative inputs) − Σ(quoted outputs), with inputs sourced from the client's own intent composition (boarding ChainInfo amounts and VTXOStore forfeit values) and outputs from the quote's positional slices. Reject when the realised value exceeds the cap, is negative, or disagrees with the declared OperatorFeeSat (operator dishonesty that would otherwise drift downstream fee accounting). Closes #379.
a3496ee to
053dae4
Compare
|
Superseded by consolidated PR #459. Closing to reduce CI load. |
[codex] Add arktest faucet command
1 participant
Closes #379.
Summary
round.evaluateQuoteonly checked the operator-attestedquote.OperatorFeeSatagainstenv.MaxOperatorFee. The echo validator intentionally permits amount deviation on theIsChange=trueoutput, so a malicious operator could quote a small fee within the cap while shaving change by a much larger delta. The client would then sign a round whose realised economic fee exceeded the cap.Fix
evaluateQuotenow enforces the cap against the realised quote economics:Three additional rejections:
realisedQuoteFee > feeCap(the actual bypass)realisedQuoteFee < 0(defensive; would only over-reject)realisedQuoteFee != quote.OperatorFeeSat(closes drift between on-chain realised fee and operator-declared fee used for confirmation-time accounting)The declared-field check stays as belt-and-braces.
ctxis plumbed throughevaluateQuoteand its three callers (initial quote, post-accept reseal, in-state reseal). TransientVTXOStore.GetVTXOfailure surfaces asQuoteRejectedwith a diagnostic (fail-closed; the alternative re-opens the bypass).Test plan
TestEvaluateQuoteRejectsChangeUnderpaymentBypass— declared fee 1k sat (under 10k cap), but change shaved 60k→40k yields realised 25k; without fix accepts, with fix rejectsTestEvaluateQuoteRejectsSingleOutputUnderpayment— no-change single-output caseTestEvaluateQuoteRealisedFeeUsesForfeitStore— refresh-round (forfeit-only) inputsTestEvaluateQuoteRejectsRealisedFeeNegative— defensive caseTestEvaluateQuoteRejectsChangeUnderpaymentBelowCap— below-cap-but-dishonest (realised != declared)TestEvaluateQuoteAcceptsExactlyAtCap— boundarymake lint-native— 0 issuesgo test ./round/... -count=1— passRelationship to #378
Complementary, not conflicting. #378 (
single-output quote echo can underpay fixed recipients) tightens the echo validator on the single-output implicit-change path. #379 (this PR) adds a global realised-fee gate that covers single- and multi-output cases. The two fixes reinforce each other; no legitimate-quote regression introduced by their combination.