round: tighten single-output quote echo amount validation (#378) - #453
round: tighten single-output quote echo amount validation (#378)#453ellemouton 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 addresses a security vulnerability (issue #378) where single-output intents, treated as implicit change, bypassed amount validation. The validateQuoteEchoes function in round/transitions.go has been updated to strictly enforce that the echoed amount for single-output intents must equal the intent target minus the operator fee. The PR also includes comprehensive regression tests and updates existing tests to reflect this requirement. Review feedback suggests using valid P2WPKH scripts in test data, replacing magic numbers with existing variables for better maintainability, and refactoring the validation logic to use if/else structures instead of continue statements for improved readability.
| // buildSingleLeaveIntents returns a deterministic intent carrying a | ||
| // single non-change LeaveRequest. Mirrors a single-VTXO offboard. | ||
| func buildSingleLeaveIntents() Intents { | ||
| leavePkScript := []byte{0x00, 0x14, 0xCA, 0xFE, 0xBA, 0xBE} |
There was a problem hiding this comment.
The leavePkScript is not a valid P2WPKH script. A P2WPKH script should be 22 bytes long (OP_0 <20-byte-hash>), but this is only 6 bytes. While the test may pass because the same invalid script is used for comparison, it's better to use valid data in tests to avoid confusion and ensure correctness.
| leavePkScript := []byte{0x00, 0x14, 0xCA, 0xFE, 0xBA, 0xBE} | |
| leavePkScript := append([]byte{0x00, 0x14}, make([]byte, 20)...) |
There was a problem hiding this comment.
Fixed in f96b570 — replaced the 6-byte stub with a valid 22-byte P2WPKH script (append([]byte{0x00, 0x14}, bytes.Repeat([]byte{0xab}, 20)...)).
|
|
||
| intents := buildSingleLeaveIntents() | ||
| quote := quoteFromIntents(t, intents, 2_500) | ||
| quote.LeaveQuotes[0].AmountSat -= 2_500 |
There was a problem hiding this comment.
To avoid using a magic number and improve maintainability, you can use the OperatorFeeSat from the quote object, which is already set to the correct value.
| quote.LeaveQuotes[0].AmountSat -= 2_500 | |
| quote.LeaveQuotes[0].AmountSat -= quote.OperatorFeeSat |
There was a problem hiding this comment.
Fixed in f96b570 — replaced the 2_500 magic number with quote.OperatorFeeSat.
| // Multi-output intent: only the explicit IsChange=true | ||
| // slot may deviate from its intent target. | ||
| if !implicitChange { | ||
| if vtxoReq.IsChange { | ||
| continue | ||
| } | ||
| if entry.AmountSat != int64(vtxoReq.Amount) { | ||
| return fmt.Sprintf("vtxo[%d] "+ | ||
| "non-change amount %d != "+ | ||
| "intent target %d", i, | ||
| entry.AmountSat, | ||
| int64(vtxoReq.Amount)), false | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| // Single-output implicit-change intent: the only honest | ||
| // deviation is (Amount − OperatorFeeSat). Anything else | ||
| // is a fee-shave attack on the lone output. | ||
| expected := int64(vtxoReq.Amount) - quote.OperatorFeeSat | ||
| if entry.AmountSat != expected { | ||
| return fmt.Sprintf("vtxo[%d] implicit-change "+ | ||
| "amount %d != intent target %d - "+ | ||
| "operator fee %d (= %d)", i, | ||
| entry.AmountSat, int64(vtxoReq.Amount), | ||
| quote.OperatorFeeSat, expected), false | ||
| } |
There was a problem hiding this comment.
The logic for validating VTXO amounts can be simplified for better readability and maintainability by using an if/else structure to handle the implicitChange case directly. This avoids the nested ifs and continue statements.
if implicitChange {
// Single-output implicit-change intent: the only honest
// deviation is (Amount − OperatorFeeSat). Anything else
// is a fee-shave attack on the lone output.
expected := int64(vtxoReq.Amount) - quote.OperatorFeeSat
if entry.AmountSat != expected {
return fmt.Sprintf("vtxo[%d] implicit-change "+
"amount %d != intent target %d - "+
"operator fee %d (= %d)", i,
entry.AmountSat, int64(vtxoReq.Amount),
quote.OperatorFeeSat, expected), false
}
} else {
// Multi-output intent: only the explicit IsChange=true
// slot may deviate from its intent target.
if !vtxoReq.IsChange && entry.AmountSat != int64(vtxoReq.Amount) {
return fmt.Sprintf("vtxo[%d] "+
"non-change amount %d != "+
"intent target %d", i,
entry.AmountSat,
int64(vtxoReq.Amount)), false
}
}There was a problem hiding this comment.
Refactored in f96b570 — the VTXO validator now uses if implicitChange { ... } else if !vtxoReq.IsChange { ... }, removing the nested ifs and continue statements. Verified semantically equivalent.
| // Multi-output intent: only the explicit IsChange=true | ||
| // slot may deviate from its intent target. | ||
| if !implicitChange { | ||
| if leaveReq.IsChange { | ||
| continue | ||
| } | ||
| if entry.AmountSat != leaveReq.Output.Value { | ||
| return fmt.Sprintf("leave[%d] "+ | ||
| "non-change amount %d != "+ | ||
| "intent target %d", i, | ||
| entry.AmountSat, | ||
| leaveReq.Output.Value), false | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| // Single-output implicit-change intent: the only honest | ||
| // deviation is (Amount − OperatorFeeSat). | ||
| expected := leaveReq.Output.Value - quote.OperatorFeeSat | ||
| if entry.AmountSat != expected { | ||
| return fmt.Sprintf("leave[%d] implicit-change "+ | ||
| "amount %d != intent target %d - "+ | ||
| "operator fee %d (= %d)", i, | ||
| entry.AmountSat, leaveReq.Output.Value, | ||
| quote.OperatorFeeSat, expected), false | ||
| } |
There was a problem hiding this comment.
Similar to the VTXO validation, this logic for validating leave amounts can be simplified for better readability and maintainability by using an if/else structure to handle the implicitChange case directly.
if implicitChange {
// Single-output implicit-change intent: the only honest
// deviation is (Amount − OperatorFeeSat).
expected := leaveReq.Output.Value - quote.OperatorFeeSat
if entry.AmountSat != expected {
return fmt.Sprintf("leave[%d] implicit-change "+
"amount %d != intent target %d - "+
"operator fee %d (= %d)", i,
entry.AmountSat, leaveReq.Output.Value,
quote.OperatorFeeSat, expected), false
}
} else {
// Multi-output intent: only the explicit IsChange=true
// slot may deviate from its intent target.
if !leaveReq.IsChange && entry.AmountSat != leaveReq.Output.Value {
return fmt.Sprintf("leave[%d] "+
"non-change amount %d != "+
"intent target %d", i,
entry.AmountSat,
leaveReq.Output.Value), false
}
}There was a problem hiding this comment.
Refactored in f96b570 — applied the same if implicitChange { ... } else if !leaveReq.IsChange { ... } restructure to the leave-amount validator. Verified semantically equivalent.
validateQuoteEchoes previously skipped the non-change amount-equality check entirely whenever the combined VTXORequests + LeaveRequests count was one. The shortcut was added to mirror the server's implicit-change relaxation (the lone slot absorbs the residual) but was too broad: it admitted any echoed amount on that slot, including zero. A malicious or compromised operator endpoint could shave arbitrary value from any single-output flow -- the worst case being a single-recipient directed send with coin-selection-exact change=0, where the lone slot is a third-party recipient marked IsChange=false and downstream commitment validation treats the quote's AmountSat as authoritative. The server's residual on the implicit-change slot is exactly (intent target - OperatorFeeSat). Enforce that equality on the lone slot for both VTXO and leave channels. The OperatorFeeSat itself is already bounded by env.MaxOperatorFee at line 840, so the only honest deviation reduces to a capped fee deduction -- not unbounded shaving. Closes #378.
5efcb94 to
f96b570
Compare
|
Superseded by consolidated PR #459. Closing to reduce CI load. |
…s-2026-05-15 multi: consolidated security fixes (May 15)
1 participant
Closes #378.
Summary
round.validateQuoteEchoessetimplicitChange := totalOutputs == 1and skipped the non-change amount echo check for both VTXO and leave entries on any single-output intent. The wallet flow only blockschange == 0 && len(recipients) > 1; a single-recipient directed send with exact coin selection ships oneVTXORequest{IsChange: false, Amount: r.Amount}which slips through. Downstream callersquoteVTXOAmount/quoteLeaveAmountstreat the quote amount as authoritative, so a malicious echo propagates into commitment validation — the operator could underpay a fixed recipient.Fix
Replaced the unconditional implicit-change skip with the precise rule
Applied symmetrically to VTXO and leave channels.
OperatorFeeSatis already capped byenv.MaxOperatorFee(negative rejected, zero/unset fails closed) so the only honest deviation is bounded by the fee cap rather than unbounded shaving.Test plan
TestEvaluateQuoteEchoRejectsSingleVTXOUnderpaymentTestEvaluateQuoteEchoRejectsSingleLeaveUnderpaymentTestEvaluateQuoteEchoRejectsSingleVTXOOverpaymentTestEvaluateQuoteEchoRejectsSingleVTXOMissingFeeDeductionTestEvaluateQuoteEchoAcceptsSingleVTXOImplicitChangeFee(honest-path)TestEvaluateQuoteEchoAcceptsSingleLeaveImplicitChangeFee(honest-path)TestActorBuffersEarlyQuoteupdated — old fixture sentAmountSat: int64(v.Amount)(no fee deduction), which only passed because the old code skipped the check; updated fixture aligns with honest server behaviormake lint-native— 0 issuesgo test ./round/... ./wallet/... -count=1— passRelationship to #379
Complementary. #379 (
fee cap can be bypassed via change underpayment) adds a global realised-fee gate; #378 (this PR) tightens the per-output echo validator on the single-output implicit-change path. The two compose: #378 rejects underpayment at the echo step; #379 catches change-shaving the echo validator still permits.