diff --git a/round/actor_test.go b/round/actor_test.go index 26f89b98e..f239fd5ba 100644 --- a/round/actor_test.go +++ b/round/actor_test.go @@ -1174,14 +1174,26 @@ func TestActorBuffersEarlyQuote(t *testing.T) { roundID := testRoundID("early-quote") // Send the quote BEFORE RoundJoined. + // + // We have a single boarding input of 50_000 sat and a single + // matching VTXO output. The lone output is implicit change + // under the #270 protocol, so the server may quote a residual + // below the intent's target. Quote it down by 1_000 sat so + // the realised fee (Σinputs−Σoutputs) agrees with + // OperatorFeeSat — see #379. vtxoQuotes := make([]VTXOQuoteEntry, len(vtxos)) for i, v := range vtxos { script, err := v.EffectivePkScript() require.NoError(t, err) + amount := int64(v.Amount) + if i == 0 { + amount -= 1_000 + } + vtxoQuotes[i] = VTXOQuoteEntry{ PkScript: script, - AmountSat: int64(v.Amount), + AmountSat: amount, RecipientKey: v.SigningKey.PubKey.SerializeCompressed(), } } diff --git a/round/quote_echo_test.go b/round/quote_echo_test.go index cf58a8fbc..b7a024fa5 100644 --- a/round/quote_echo_test.go +++ b/round/quote_echo_test.go @@ -2,6 +2,7 @@ package round import ( "context" + "strings" "testing" "time" @@ -20,6 +21,12 @@ import ( // highlight the rejection path. The returned operatorPub drives // EffectivePkScript derivation so the quote's echoed PkScript can // be computed identically from the intent. +// +// A single boarding input of 130_000 sat is included so the +// realised-fee check in evaluateQuote (#379) sees Σinputs − +// Σoutputs == 5_000, which matches the default OperatorFeeSat +// passed by quoteFromIntents. Tests that mutate fee or output +// amounts may need to compensate. func buildEchoTestIntents(t *testing.T) (Intents, *btcec.PublicKey) { t.Helper() @@ -44,7 +51,21 @@ func buildEchoTestIntents(t *testing.T) (Intents, *btcec.PublicKey) { IsChange: false, } + // Boarding input value covers Σ(quoted outputs) + the default + // 5_000 sat fee that quoteFromIntents stamps. The chain info + // amount is what realisedQuoteFee sums into Σinputs. + boarding := BoardingIntent{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 130_000, + }, + }, + } + return Intents{ + Boarding: []BoardingIntent{ + boarding, + }, VTXOs: []types.VTXORequest{ reqA.req, reqB.req, @@ -107,14 +128,17 @@ func TestEvaluateQuoteEchoAcceptsFaithfulQuote(t *testing.T) { quote := quoteFromIntents(t, intents, 5_000) env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok, "faithful echo should accept") } // TestEvaluateQuoteEchoAcceptsChangeDeviation verifies that amount // deviation is permitted for the single IsChange=true VTXO output — -// the residual sink is server-decided by design. +// the residual sink is server-decided by design — as long as the +// resulting realised fee stays within env.MaxOperatorFee (#379). func TestEvaluateQuoteEchoAcceptsChangeDeviation(t *testing.T) { t.Parallel() @@ -122,11 +146,18 @@ func TestEvaluateQuoteEchoAcceptsChangeDeviation(t *testing.T) { quote := quoteFromIntents(t, intents, 5_000) // Change entry is intents.VTXOs[1]; server chooses a - // different residual. Must still accept. - quote.VTXOQuotes[1].AmountSat = 42_000 + // different residual. Σinputs=130_000, other outputs sum to + // 40_000+25_000=65_000, so the new change=57_000 pushes the + // realised fee to 8_000 — still under the 10_000 cap. + // quote.OperatorFeeSat must agree with the realised value so + // the dishonesty check passes. + quote.VTXOQuotes[1].AmountSat = 57_000 + quote.OperatorFeeSat = 8_000 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok, "change-output deviation must be permitted") } @@ -143,7 +174,9 @@ func TestEvaluateQuoteEchoRejectsVTXOLengthMismatch(t *testing.T) { quote.VTXOQuotes = quote.VTXOQuotes[:1] env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "vtxo entries") @@ -159,7 +192,9 @@ func TestEvaluateQuoteEchoRejectsLeaveLengthMismatch(t *testing.T) { quote.LeaveQuotes = nil env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "leave entries") @@ -179,7 +214,9 @@ func TestEvaluateQuoteEchoRejectsVTXOPkScriptMismatch(t *testing.T) { quote.VTXOQuotes[0].PkScript[0] ^= 0xFF env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "pkScript echo mismatch") @@ -199,7 +236,9 @@ func TestEvaluateQuoteEchoRejectsRecipientKeyMismatch(t *testing.T) { quote.VTXOQuotes[0].RecipientKey[1] ^= 0xFF env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "recipient key echo mismatch") @@ -221,7 +260,9 @@ func TestEvaluateQuoteEchoRejectsNonChangeVTXOAmountDrift(t *testing.T) { quote.VTXOQuotes[1].AmountSat = 65_000 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "non-change amount") @@ -238,7 +279,9 @@ func TestEvaluateQuoteEchoRejectsNonChangeLeaveAmountDrift(t *testing.T) { quote.LeaveQuotes[0].AmountSat = 24_999 env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "leave") @@ -312,7 +355,8 @@ func TestEvaluateQuoteRendersActionableRejectReason(t *testing.T) { env := quoteReceivedTestEnv(10_000) decision := evaluateQuote( - env, RoundID{}, intents, quote, + context.Background(), env, RoundID{}, intents, + quote, ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) @@ -358,7 +402,9 @@ func TestEvaluateQuoteRejectsExpiredQuote(t *testing.T) { return expiry.Add(5 * time.Second) } - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok, "expired quote must reject") require.Contains(t, rej.Reason, "expired") @@ -379,7 +425,9 @@ func TestEvaluateQuoteAcceptsFreshQuoteBeforeExpiry(t *testing.T) { env := quoteReceivedTestEnv(10_000) env.Now = func() time.Time { return now } - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) _, ok := decision.(*QuoteAccepted) require.True(t, ok) } @@ -409,10 +457,14 @@ func TestQuoteReceivedReplacesOnReseal(t *testing.T) { env := quoteReceivedTestEnv(10_000) // Second pass: higher seal_pass with a different change - // amount the server chose under updated chain state. + // amount the server chose under updated chain state. Σinputs + // =130_000; with change=59_000 plus the unchanged 40_000 and + // 25_000 outputs the realised fee is 6_000 — matches the + // declared OperatorFeeSat and stays within the 10_000 cap + // (#379). second := quoteFromIntents(t, intents, 6_000) second.SealPass = 1 - second.VTXOQuotes[1].AmountSat = 58_000 // Change leg shifted. + second.VTXOQuotes[1].AmountSat = 59_000 // Change leg shifted. tr, err := s.ProcessEvent( context.Background(), @@ -493,7 +545,9 @@ func TestEvaluateQuoteRejectsZeroCap(t *testing.T) { quote := quoteFromIntents(t, intents, 1) // Small positive fee. env := quoteReceivedTestEnv(0) // Unset cap. - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok, "zero cap must reject") require.Contains(t, rej.Reason, "cap is unset") @@ -509,7 +563,9 @@ func TestEvaluateQuoteRejectsNegativeOperatorFee(t *testing.T) { quote := quoteFromIntents(t, intents, -1) env := quoteReceivedTestEnv(10_000) - decision := evaluateQuote(env, RoundID{}, intents, quote) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) rej, ok := decision.(*QuoteRejected) require.True(t, ok) require.Contains(t, rej.Reason, "negative") @@ -525,3 +581,373 @@ func TestEvaluateQuoteRejectsNegativeOperatorFee(t *testing.T) { _, isFail := tr.NextState.(*ClientFailedState) require.True(t, isFail) } + +// TestEvaluateQuoteRejectsChangeUnderpaymentBypass covers the #379 +// fee-cap bypass: a malicious operator quotes an OperatorFeeSat +// that sits comfortably under env.MaxOperatorFee while shaving the +// IsChange=true VTXO output by a much larger delta. The echo +// validation intentionally permits change-output amount deviation, +// so without the realised-fee recomputation the client would +// accept and sign a round whose actual economic fee exceeds the +// cap. The realised-fee check in evaluateQuote must catch this. +func TestEvaluateQuoteRejectsChangeUnderpaymentBypass(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 1_000) + + // Σinputs=130_000; honest outputs sum to 125_000 so the + // declared 1_000-sat fee is a lie unless the change output + // is shaved. Drop the change leg by 20_000 sat (60_000 → + // 40_000) so Σoutputs=105_000 and the realised fee is + // 25_000 — well above the 10_000 cap. + quote.VTXOQuotes[1].AmountSat = 40_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "change underpayment must be rejected, got %T", decision, + ) + // Either the cap-exceeded message or the dishonesty-mismatch + // message is acceptable — both protect the client. + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteRejectsChangeUnderpaymentBelowCap covers the +// subtler #379 variant: the change shave keeps the realised fee +// just within MaxOperatorFee, but the operator still lies about +// what they took. The dishonesty mismatch check must reject so +// the FeePaidMsg accounting cannot diverge from on-chain reality +// (the confirmation-time accounting trusts OperatorFeeSat as +// authoritative — see computeClientOperatorFee). +func TestEvaluateQuoteRejectsChangeUnderpaymentBelowCap(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Σinputs=130_000; declared fee=5_000. Shave the change leg + // from 60_000 to 57_000 so Σoutputs=122_000 and realised + // fee=8_000. That sits under the 10_000 cap but disagrees + // with the declared 5_000. + quote.VTXOQuotes[1].AmountSat = 57_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "dishonest fee declaration must be rejected, got %T", + decision, + ) + require.Contains(t, rej.Reason, "disagrees with realised fee") +} + +// TestEvaluateQuoteRejectsSingleOutputUnderpayment covers the #379 +// variant on single-output intents. With totalOutputs==1 the echo +// validator skips the per-output equality check (treating the lone +// output as implicit change), so the only protection against the +// operator silently lowering that output is the realised-fee +// recomputation. Build a single-output intent, have the operator +// declare a small fee while quoting a much smaller output amount, +// and assert rejection. +func TestEvaluateQuoteRejectsSingleOutputUnderpayment(t *testing.T) { + t.Parallel() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + req := mkReq(t, op, 0x30, true) + req.req.Amount = 100_000 + req.req.IsChange = false + + intents := Intents{ + Boarding: []BoardingIntent{{ + BoardingIntent: WalletBoardingIntent{ + ChainInfo: BoardingChainInfo{ + Amount: 100_000, + }, + }, + }}, + VTXOs: []types.VTXORequest{ + req.req, + }, + } + + // Operator declares a tiny 500-sat fee while quoting the + // lone VTXO output down by 20_000 sat. Without #379's + // realised-fee check, echo validation passes (single-output + // intent is implicit change) and the client signs a round + // paying 20_000 in fees against a 10_000 cap. + script, err := req.req.EffectivePkScript() + require.NoError(t, err) + + var quoteID [32]byte + for i := range quoteID { + quoteID[i] = byte(i + 1) + } + + recipientKey := req.req.SigningKey.PubKey.SerializeCompressed() + quote := &ClientQuote{ + QuoteID: quoteID, + OperatorFeeSat: 500, + VTXOQuotes: []VTXOQuoteEntry{{ + PkScript: script, + // 20_000 sat underpayment. + AmountSat: 80_000, + RecipientKey: recipientKey, + }}, + } + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "single-output underpayment must be rejected, got %T", + decision, + ) + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestEvaluateQuoteRejectsRealisedFeeNegative covers the symmetric +// case: the operator quotes outputs that exceed the available +// inputs (i.e. the quote claims to mint value). The realised-fee +// check must reject — the existing balance guard at intent +// composition catches this on the input side, but a malicious +// operator could inflate a non-change output post-quote, and the +// FSM has no second balance gate before signing. +func TestEvaluateQuoteRejectsRealisedFeeNegative(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Σinputs=130_000; inflate the change leg from 60_000 to + // 200_000 so Σoutputs=265_000 and realised fee is −135_000. + quote.VTXOQuotes[1].AmountSat = 200_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True(t, ok) + require.Contains(t, rej.Reason, "realised fee is negative") +} + +// TestEvaluateQuoteAcceptsExactlyAtCap covers the edge where the +// realised fee equals the cap exactly. The check is "exceeds cap" +// so equality must accept. +func TestEvaluateQuoteAcceptsExactlyAtCap(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 10_000) + + // Σinputs=130_000; shave change from 60_000 to 55_000 so + // Σoutputs=120_000 and realised fee=10_000, exactly at cap. + quote.VTXOQuotes[1].AmountSat = 55_000 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True(t, ok, "exactly-at-cap realised fee must accept") +} + +// TestEvaluateQuoteRealisedFeeUsesForfeitStore covers the +// forfeit-only flow (refresh rounds): inputs come from VTXOStore +// lookups rather than boarding ChainInfo. A malicious operator +// that understates the change output must still be caught when +// the inputs are sourced from the store. +func TestEvaluateQuoteRealisedFeeUsesForfeitStore(t *testing.T) { + t.Parallel() + + opPriv, err := btcec.NewPrivateKey() + require.NoError(t, err) + op := opPriv.PubKey() + + // Two recipient VTXOs and one change VTXO, no boarding. + reqA := mkReq(t, op, 0x40, true) + reqA.req.Amount = 30_000 + reqA.req.IsChange = false + + reqB := mkReq(t, op, 0x50, true) + reqB.req.Amount = 60_000 + reqB.req.IsChange = true + + outpoint := wire.OutPoint{Index: 0} + forfeit := types.ForfeitRequest{ + VTXOOutpoint: &outpoint, + // Fallback used when store is nil. + Amount: 100_000, + } + + intents := Intents{ + VTXOs: []types.VTXORequest{ + reqA.req, + reqB.req, + }, + Forfeits: []types.ForfeitRequest{ + forfeit, + }, + } + + scriptA, err := reqA.req.EffectivePkScript() + require.NoError(t, err) + scriptB, err := reqB.req.EffectivePkScript() + require.NoError(t, err) + + var quoteID [32]byte + for i := range quoteID { + quoteID[i] = byte(i + 1) + } + + // Honest fee would be 100_000−90_000=10_000 (exactly at + // cap). Malicious operator declares 1_000 while shaving the + // change leg from 60_000 to 30_000 (Σoutputs=60_000, + // realised=40_000). + keyA := reqA.req.SigningKey.PubKey.SerializeCompressed() + keyB := reqB.req.SigningKey.PubKey.SerializeCompressed() + quote := &ClientQuote{ + QuoteID: quoteID, + OperatorFeeSat: 1_000, + VTXOQuotes: []VTXOQuoteEntry{ + { + PkScript: scriptA, + AmountSat: 30_000, + RecipientKey: keyA, + }, + { + PkScript: scriptB, + AmountSat: 30_000, + RecipientKey: keyB, + }, + }, + } + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "refresh-round change underpayment must be rejected, "+ + "got %T", decision, + ) + require.True( + t, + containsAny( + rej.Reason, []string{ + "realised operator fee", + "disagrees with realised fee", + }, + ), + "unexpected rejection reason: %q", + rej.Reason, + ) +} + +// TestRealisedFeeIncludesZeroValueOutputs verifies that a quote +// whose echoed amounts include a zero-value entry contributes that +// zero to the realised-fee sum (i.e. the loop no longer filters +// zero). The honest case is constructed so the realised fee matches +// the declared OperatorFeeSat exactly; a stray filter or arithmetic +// drift would push realised != declared and trigger the dishonesty +// rejection. We exercise a zero-value leave entry because that is +// the only on-chain shape (e.g. OP_RETURN markers) where zero is +// arguably legitimate; the VTXO side is rejected upstream as dust, +// but the realised-fee sum is shape-agnostic by design. +func TestRealisedFeeIncludesZeroValueOutputs(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + + // Replace the leave intent's value with zero so the echo and + // the realised sum both see a 0-sat output. Σinputs=130_000; + // VTXO outputs sum to 40_000+60_000=100_000; leave sum drops + // from 25_000 to 0, so the realised fee climbs from 5_000 to + // 30_000. Declare 30_000 to keep the dishonesty check happy + // and bump the cap accordingly. + intents.Leaves[0].Output.Value = 0 + + quote := quoteFromIntents(t, intents, 30_000) + env := quoteReceivedTestEnv(30_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + _, ok := decision.(*QuoteAccepted) + require.True( + t, ok, "zero-value leave output must be summed into "+ + "realised fee, got %T", decision, + ) +} + +// TestRealisedFeeRejectsNegativeOutput verifies that a quote with +// a negative AmountSat is rejected at the realised-fee computation +// step, rather than being silently filtered. A negative output +// would subtract a positive value from Σoutputs and inflate the +// realised fee in the operator's favor; the previous `if amt > 0` +// filter masked this by treating the entry as zero. +func TestRealisedFeeRejectsNegativeOutput(t *testing.T) { + t.Parallel() + + intents, _ := buildEchoTestIntents(t) + quote := quoteFromIntents(t, intents, 5_000) + + // Inject a negative leave-output amount. The echo validator + // runs first; rebuild the intent's leave value to match so we + // exercise the realisedQuoteFee branch rather than tripping + // the non-change-amount echo check. + intents.Leaves[0].Output.Value = -1 + quote.LeaveQuotes[0].AmountSat = -1 + + env := quoteReceivedTestEnv(10_000) + decision := evaluateQuote( + context.Background(), env, RoundID{}, intents, quote, + ) + rej, ok := decision.(*QuoteRejected) + require.True( + t, ok, "negative output must be rejected, got %T", decision, + ) + require.Contains(t, rej.Reason, "negative") +} + +// containsAny reports whether any of the substrings appears in s. +func containsAny(s string, subs []string) bool { + for _, sub := range subs { + if sub != "" && strings.Contains(s, sub) { + return true + } + } + + return false +} diff --git a/round/quote_received_state_test.go b/round/quote_received_state_test.go index ba7493e16..c1c21d1bf 100644 --- a/round/quote_received_state_test.go +++ b/round/quote_received_state_test.go @@ -105,23 +105,23 @@ func TestEvaluateQuoteRejectsFeeAboveCap(t *testing.T) { env := quoteReceivedTestEnv(1000) empty := Intents{} - // Fee above cap → reject. + // Fee above cap → reject (early belt-and-braces check on the + // operator-declared field; the realised-fee check below is + // the authoritative defense — see #379). q := &ClientQuote{ OperatorFeeSat: 1500, RejectReason: roundpb.QuoteReason_QUOTE_OK, } - decision := evaluateQuote(env, RoundID{}, empty, q) + decision := evaluateQuote( + context.Background(), env, RoundID{}, empty, q, + ) _, isReject := decision.(*QuoteRejected) require.True(t, isReject) - // Fee at cap → accept. - q.OperatorFeeSat = 1000 - decision = evaluateQuote(env, RoundID{}, empty, q) - _, isAccept := decision.(*QuoteAccepted) - require.True(t, isAccept) - // Nil quote → reject defensively. - decision = evaluateQuote(env, RoundID{}, empty, nil) + decision = evaluateQuote( + context.Background(), env, RoundID{}, empty, nil, + ) _, isReject = decision.(*QuoteRejected) require.True(t, isReject) @@ -131,7 +131,9 @@ func TestEvaluateQuoteRejectsFeeAboveCap(t *testing.T) { OperatorFeeSat: 0, RejectReason: roundpb.QuoteReason_INSUFFICIENT_RESIDUAL, } - decision = evaluateQuote(env, RoundID{}, empty, q) + decision = evaluateQuote( + context.Background(), env, RoundID{}, empty, q, + ) _, isReject = decision.(*QuoteRejected) require.True(t, isReject) } diff --git a/round/transitions.go b/round/transitions.go index dcc292598..d2d6d03a2 100644 --- a/round/transitions.go +++ b/round/transitions.go @@ -613,7 +613,7 @@ func (s *IntentSentState) ProcessEvent(ctx context.Context, event ClientEvent, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{ @@ -766,8 +766,8 @@ func designateChangeMarker(vtxoReqs []types.VTXORequest, } } -func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, - quote *ClientQuote) ClientEvent { +func evaluateQuote(ctx context.Context, env *ClientEnvironment, roundID RoundID, + intents Intents, quote *ClientQuote) ClientEvent { if quote == nil { return &QuoteRejected{ @@ -837,6 +837,13 @@ func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, "refusing to sign", } } + + // Belt-and-braces cap check on the operator-declared fee. + // The realised-fee check below is the authoritative defense, + // but rejecting a self-incriminating declaration early keeps + // the diagnostic message close to the field the operator + // chose. A malicious operator can lie about this number, so + // we never rely on it alone (see realisedQuoteFee). if quote.OperatorFeeSat > feeCap { return &QuoteRejected{ RoundID: roundID, @@ -856,12 +863,137 @@ func evaluateQuote(env *ClientEnvironment, roundID RoundID, intents Intents, } } + // Realised-fee cap enforcement (#379). The + // quote.OperatorFeeSat field is an operator-supplied claim; + // the actual economic fee the client will pay is + // Σ(inputs) − Σ(quoted outputs). A malicious operator can + // quote a small OperatorFeeSat while reducing the change + // output (or any quote-decided amount) by a much larger + // delta, so cap enforcement against the declared field alone + // is bypassable. Recompute the realised fee from the + // authoritative input amounts (boarding ChainInfo, VTXOStore + // forfeit values) and the quote's positional output amounts; + // reject if it exceeds the cap or if the quote's declared + // fee disagrees with the realised value (operator dishonesty). + realised, err := realisedQuoteFee(ctx, env, intents, quote) + if err != nil { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised fee computation failed: %v", err, + ), + } + } + if realised < 0 { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised fee is negative: outputs exceed "+ + "inputs by %d sat", -realised, + ), + } + } + if realised > feeCap { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "realised operator fee %d exceeds cap %d "+ + "(quoted operator_fee_sat=%d)", + realised, feeCap, quote.OperatorFeeSat, + ), + } + } + if realised != quote.OperatorFeeSat { + return &QuoteRejected{ + RoundID: roundID, + QuoteID: quote.QuoteID, + Reason: fmt.Sprintf( + "quoted operator_fee_sat=%d disagrees with "+ + "realised fee=%d (Σinputs−Σoutputs)", + quote.OperatorFeeSat, realised, + ), + } + } + return &QuoteAccepted{ RoundID: roundID, QuoteID: quote.QuoteID, } } +// realisedQuoteFee returns the economic operator fee the client +// will pay if it signs the supplied quote, computed as +// Σ(authoritative inputs) − Σ(quoted outputs). Inputs are sourced +// from the client's own intent composition (boarding ChainInfo +// amounts and forfeit values looked up from the VTXOStore) so a +// malicious operator cannot inflate them; outputs are sourced from +// the quote's positional VTXOQuotes / LeaveQuotes amounts because +// those are the values the server will actually stamp into the +// commitment tx and VTXO tree. This is the authoritative cap- +// enforcement signal: every other field on the quote (notably +// OperatorFeeSat) is operator-attested and may be a lie. +// +// Returns an error only when the VTXOStore lookup for a forfeited +// VTXO fails; an unset store falls back to the embedded forfeit +// Amount hint so harness paths without persistence keep working. +func realisedQuoteFee(ctx context.Context, env *ClientEnvironment, + intents Intents, quote *ClientQuote) (int64, error) { + + // Sum inputs and outputs without filtering zero, so the + // realised fee equals Σinputs−Σoutputs exactly. Filtering + // zero is a no-op arithmetically but invites the reader to + // believe negative values are also benignly absorbed; they + // are not — a negative amount on either side would silently + // shift the realised fee in a direction that lets a hostile + // operator pass the cap. Reject any negative value explicitly + // instead so callers see a diagnostic rather than a quietly + // wrong realised fee. + var inputsSat int64 + for i := range intents.Boarding { + amt := int64(intents.Boarding[i].ChainInfo.Amount) + if amt < 0 { + return 0, fmt.Errorf("negative boarding input "+ + "amount: %d sat", amt) + } + inputsSat += amt + } + + // Forfeit values must come from the VTXOStore (or, when the + // store is nil, the embedded Amount hint). Trusting any + // operator-supplied number here would re-open the same + // inflation hole the cap is meant to close. + forfeitAmt, err := computeTotalForfeitAmount( + ctx, env.VTXOStore, intents.Forfeits, + ) + if err != nil { + return 0, fmt.Errorf("forfeit amount lookup: %w", err) + } + inputsSat += int64(forfeitAmt) + + var outputsSat int64 + for i := range quote.VTXOQuotes { + amt := quote.VTXOQuotes[i].AmountSat + if amt < 0 { + return 0, fmt.Errorf("negative vtxo output amount at "+ + "index %d: %d sat", i, amt) + } + outputsSat += amt + } + for i := range quote.LeaveQuotes { + amt := quote.LeaveQuotes[i].AmountSat + if amt < 0 { + return 0, fmt.Errorf("negative leave output amount at "+ + "index %d: %d sat", i, amt) + } + outputsSat += amt + } + + return inputsSat - outputsSat, nil +} + // quoteRejectReason formats server-side quote rejections for operator-facing // logs and operation status output. func quoteRejectReason(reason roundpb.QuoteReason) string { @@ -1004,7 +1136,7 @@ func (s *QuoteReceivedState) ProcessEvent(ctx context.Context, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{ @@ -1196,7 +1328,7 @@ func (s *RoundJoinedState) ProcessEvent(ctx context.Context, event ClientEvent, } decision := evaluateQuote( - env, evt.RoundID, s.Intents, evt.Quote, + ctx, env, evt.RoundID, s.Intents, evt.Quote, ) return &ClientStateTransition{