round: tighten single-output quote echo amount validation (#378)

round/actor_test.go

@@ -1174,14 +1174,23 @@ func TestActorBuffersEarlyQuote(t *testing.T) {
 	roundID := testRoundID("early-quote")
 
 	// Send the quote BEFORE RoundJoined.
+	//
+	// This is a single-output boarding intent (IsChange=false on the
+	// lone VTXO), so the server treats the lone slot as implicit
+	// change and stamps (Amount − OperatorFeeSat) on it. Mirror that
+	// here so validateQuoteEchoes accepts -- the previously-loose
+	// implicit-change shortcut would have accepted any AmountSat,
+	// but issue #378 tightened the rule to require the exact
+	// (Amount − fee) deviation.
+	const operatorFeeSat = int64(1_000)
 	vtxoQuotes := make([]VTXOQuoteEntry, len(vtxos))
 	for i, v := range vtxos {
 		script, err := v.EffectivePkScript()
 		require.NoError(t, err)
 
 		vtxoQuotes[i] = VTXOQuoteEntry{
 			PkScript:     script,
-			AmountSat:    int64(v.Amount),
+			AmountSat:    int64(v.Amount) - operatorFeeSat,
 			RecipientKey: v.SigningKey.PubKey.SerializeCompressed(),
 		}
 	}
@@ -1193,7 +1202,7 @@ func TestActorBuffersEarlyQuote(t *testing.T) {
 
 	quote := &ClientQuote{
 		QuoteID:        quoteID,
-		OperatorFeeSat: 1_000,
+		OperatorFeeSat: operatorFeeSat,
 		VTXOQuotes:     vtxoQuotes,
 	}
 

round/quote_echo_test.go

@@ -1,6 +1,7 @@
 package round
 
 import (
+	"bytes"
 	"context"
 	"testing"
 	"time"
@@ -525,3 +526,193 @@ func TestEvaluateQuoteRejectsNegativeOperatorFee(t *testing.T) {
 	_, isFail := tr.NextState.(*ClientFailedState)
 	require.True(t, isFail)
 }
+
+// buildSingleVTXOIntents returns a deterministic intent carrying a
+// single non-change VTXORequest. This mirrors the wire shape produced
+// by:
+//
+//   - a single-recipient directed send whose coin selection covered
+//     the target exactly (no self-change),
+//   - a single-VTXO refresh,
+//   - or a single-input boarding flow.
+//
+// All four flows ship one output with IsChange=false; the server then
+// treats the lone slot as implicit change. Issue #378 reported that
+// the client previously skipped the amount echo entirely in this
+// case, leaving the lone output's value at the operator's discretion.
+func buildSingleVTXOIntents(t *testing.T) Intents {
+	t.Helper()
+
+	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
+
+	return Intents{
+		VTXOs: []types.VTXORequest{
+			req.req,
+		},
+	}
+}
+
+// buildSingleLeaveIntents returns a deterministic intent carrying a
+// single non-change LeaveRequest. Mirrors a single-VTXO offboard.
+func buildSingleLeaveIntents() Intents {
+	// A valid P2WPKH script is OP_0 <20-byte-hash>, total 22 bytes.
+	leavePkScript := append(
+		[]byte{0x00, 0x14}, bytes.Repeat([]byte{0xab}, 20)...,
+	)
+
+	return Intents{
+		Leaves: []*types.LeaveRequest{{
+			Output: &wire.TxOut{
+				PkScript: leavePkScript,
+				Value:    100_000,
+			},
+			IsChange: false,
+		}},
+	}
+}
+
+// quoteFromSingleVTXOWithFee builds a quote echoing the lone VTXO
+// entry with its amount reduced by the supplied operator fee. This
+// matches the honest server's behaviour for a single-output intent:
+// residual = Σin − Σ(fixed) − fee, stamped on the lone (implicit-
+// change) slot.
+func quoteFromSingleVTXOWithFee(t *testing.T, intents Intents,
+	operatorFeeSat int64) *ClientQuote {
+
+	t.Helper()
+	quote := quoteFromIntents(t, intents, operatorFeeSat)
+	quote.VTXOQuotes[0].AmountSat -= operatorFeeSat
+
+	return quote
+}
+
+// TestEvaluateQuoteEchoAcceptsSingleVTXOImplicitChangeFee verifies
+// the honest single-output path: server echoes (Amount − fee) on the
+// lone slot and the client accepts. Guards against over-tightening
+// the #378 fix into rejecting honest single-output refresh / leave /
+// boarding flows.
+func TestEvaluateQuoteEchoAcceptsSingleVTXOImplicitChangeFee(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleVTXOIntents(t)
+	quote := quoteFromSingleVTXOWithFee(t, intents, 2_500)
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	_, ok := decision.(*QuoteAccepted)
+	require.True(
+		t, ok, "honest single-output fee deduction must accept",
+	)
+}
+
+// TestEvaluateQuoteEchoRejectsSingleVTXOUnderpayment is the primary
+// regression test for issue #378. Before the fix, a server that
+// echoed an arbitrary smaller amount on a single non-change output
+// was accepted because the implicitChange shortcut skipped the
+// amount-equality check entirely. After the fix, only the exact
+// (Amount − OperatorFeeSat) deviation is permitted; anything else
+// rejects.
+//
+// This test MUST fail without the fix.
+func TestEvaluateQuoteEchoRejectsSingleVTXOUnderpayment(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleVTXOIntents(t)
+	quote := quoteFromIntents(t, intents, 1_000)
+
+	// Adversarial: operator claims a 1_000 sat fee but shaves
+	// 50_000 sat off the lone recipient. With the implicitChange
+	// shortcut this was silently accepted (fund theft); the
+	// tightened rule requires entry == Amount − OperatorFeeSat.
+	quote.VTXOQuotes[0].AmountSat = 50_000
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	rej, ok := decision.(*QuoteRejected)
+	require.True(t, ok, "single-output underpayment must reject")
+	require.Contains(t, rej.Reason, "implicit-change amount")
+}
+
+// TestEvaluateQuoteEchoRejectsSingleLeaveUnderpayment is the leave-
+// channel mirror of the #378 regression. A single-output offboard
+// that the server shaves beyond the quoted operator fee must reject.
+//
+// This test MUST fail without the fix.
+func TestEvaluateQuoteEchoRejectsSingleLeaveUnderpayment(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleLeaveIntents()
+	quote := quoteFromIntents(t, intents, 1_000)
+	quote.LeaveQuotes[0].AmountSat = 50_000
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	rej, ok := decision.(*QuoteRejected)
+	require.True(t, ok, "single-leave underpayment must reject")
+	require.Contains(t, rej.Reason, "implicit-change amount")
+}
+
+// TestEvaluateQuoteEchoAcceptsSingleLeaveImplicitChangeFee verifies
+// the honest single-leave path: server echoes (Value − fee) on the
+// lone slot and the client accepts.
+func TestEvaluateQuoteEchoAcceptsSingleLeaveImplicitChangeFee(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleLeaveIntents()
+	quote := quoteFromIntents(t, intents, 2_500)
+	quote.LeaveQuotes[0].AmountSat -= quote.OperatorFeeSat
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	_, ok := decision.(*QuoteAccepted)
+	require.True(t, ok, "honest single-leave fee deduction must "+
+		"accept")
+}
+
+// TestEvaluateQuoteEchoRejectsSingleVTXOOverpayment guards against a
+// hostile server INCREASING the lone output's amount (e.g. to inflate
+// the client's claimed balance prior to a follow-on attack). The
+// tightened rule requires an exact match.
+func TestEvaluateQuoteEchoRejectsSingleVTXOOverpayment(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleVTXOIntents(t)
+	quote := quoteFromIntents(t, intents, 1_000)
+
+	// Operator inflates the lone slot above (Amount − fee).
+	quote.VTXOQuotes[0].AmountSat = int64(intents.VTXOs[0].Amount) +
+		10_000
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	rej, ok := decision.(*QuoteRejected)
+	require.True(t, ok, "single-output overpayment must reject")
+	require.Contains(t, rej.Reason, "implicit-change amount")
+}
+
+// TestEvaluateQuoteEchoRejectsSingleVTXOMissingFeeDeduction guards
+// the boundary case where the server claims a non-zero operator fee
+// but echoes the lone slot at the full intent target (i.e. shifts
+// the fee somewhere else, like an off-tree mint). The tightened rule
+// rejects: the residual must be stamped on the implicit-change slot.
+func TestEvaluateQuoteEchoRejectsSingleVTXOMissingFeeDeduction(t *testing.T) {
+	t.Parallel()
+
+	intents := buildSingleVTXOIntents(t)
+	quote := quoteFromIntents(t, intents, 1_000)
+	// Intentionally do not subtract the fee: echo == Amount.
+
+	env := quoteReceivedTestEnv(10_000)
+	decision := evaluateQuote(env, RoundID{}, intents, quote)
+	rej, ok := decision.(*QuoteRejected)
+	require.True(
+		t, ok, "single-output missing fee deduction must reject",
+	)
+	require.Contains(t, rej.Reason, "implicit-change amount")
+}

round/transitions.go

@@ -905,9 +905,24 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) {
 	// combined VTXORequests + LeaveRequests, the server treats that
 	// sole output as implicit change and stamps the residual on it
 	// without requiring IsChange=true on the wire (#270, see the
-	// server's resolveChangeDesignation). Mirror that contract here
-	// so single-output boarding / refresh / leave intents do not
-	// trip the non-change amount-equality check below.
+	// server's resolveChangeDesignation). The server's residual on
+	// such a slot is Σin − Σ(fixed) − fee, and because the lone
+	// output IS the implicit-change slot it does not count toward
+	// Σ(fixed); upstream wallet flows (refresh / leave / boarding /
+	// single-recipient directed-send-with-no-self-change) all set
+	// the lone output's Amount to the input value, so the only
+	// honest deviation is exactly the quote-level OperatorFeeSat.
+	//
+	// Previously this branch unconditionally skipped the amount
+	// check whenever totalOutputs == 1 (issue #378). That admitted
+	// arbitrary shaving of the single output by a malicious or
+	// compromised operator endpoint -- e.g. a single-recipient
+	// directed send could be silently underpaid because the
+	// recipient slot is IsChange=false and the wallet relied on
+	// validateQuoteEchoes to enforce the amount. The relaxation now
+	// allows exactly one deviation -- (Amount − OperatorFeeSat) on
+	// the implicit-change slot -- which is bounded by the already-
+	// capped feeCap above.
 	totalOutputs := len(intents.VTXOs) + len(intents.Leaves)
 	implicitChange := totalOutputs == 1
 
@@ -935,11 +950,33 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) {
 				"echo mismatch", i), false
 		}
 
-		if !vtxoReq.IsChange && !implicitChange &&
-			entry.AmountSat != int64(vtxoReq.Amount) {
-			return fmt.Sprintf("vtxo[%d] non-change amount "+
-				"%d != intent target %d", i,
-				entry.AmountSat, int64(vtxoReq.Amount)), false
+		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 if !vtxoReq.IsChange {
+			// Multi-output intent: only the explicit
+			// IsChange=true slot may deviate from its
+			// intent target.
+			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
+			}
 		}
 	}
 
@@ -956,11 +993,32 @@ func validateQuoteEchoes(intents Intents, quote *ClientQuote) (string, bool) {
 				"mismatch", i), false
 		}
 
-		if !leaveReq.IsChange && !implicitChange &&
-			entry.AmountSat != leaveReq.Output.Value {
-			return fmt.Sprintf("leave[%d] non-change "+
-				"amount %d != intent target %d", i,
-				entry.AmountSat, leaveReq.Output.Value), false
+		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 if !leaveReq.IsChange {
+			// Multi-output intent: only the explicit
+			// IsChange=true slot may deviate from its
+			// intent target.
+			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
+			}
 		}
 	}