Refuse a hop address, a foreign quote and a slippage outside a percentage

An adversarial pass over the calldata found three more ways a crafted quote
became a signable transaction.

A hop address was taken as given. An address is twenty bytes and the path packs
it against a uint24 with nothing marking the boundary, so a short one shifted
every byte after it — and the path still parsed, as a route through pools
nobody named. Width is the only thing separating the two, so both hop addresses
are now checked the way the swapper already was.

A quote says which chain priced it, and that was ignored. Building this chain's
router call from another chain's quote addresses pools by the same numbers on a
book that never quoted them, and these chains share deployment addresses, so it
is a real trade at a price nobody was shown.

Slippage is a percentage of an amount and lives between none of it and all of
it. Below zero the factor inverted and the bound grew a minus sign; above a
hundred it ran past a word. Clamping the percentage bounds both directions at
one place, where the alternative was clamping every product it reaches.

IsAddress is exported: whether a chain has an address to address a transaction
to is one question, and the process that mounts these handlers has to ask it.

Co-authored-by: Hanzo Dev <[email protected]>

engine/approval.go

@@ -23,7 +23,7 @@ import (
 // approves a router once and trades against it for as long as they use it; an
 // approval sized to the trade in hand would put a signature in front of every
 // swap after it.
-var unlimited = maxWord
+var unlimited = new(big.Int).Set(maxWord)
 
 var (
 	selAllowance = selector("allowance(address,address)")
@@ -64,7 +64,7 @@ func HandleCheckApproval(chainID int64, rpc, router string) http.HandlerFunc {
 			return
 		}
 
-		if !isHexAddress(req.WalletAddress) {
+		if !IsAddress(req.WalletAddress) {
 			badQuote(w, "walletAddress must be an address")
 			return
 		}
@@ -90,7 +90,7 @@ func HandleCheckApproval(chainID int64, rpc, router string) http.HandlerFunc {
 			writeJSON(w, http.StatusOK, none)
 			return
 		}
-		if !isHexAddress(req.Token) {
+		if !IsAddress(req.Token) {
 			badQuote(w, "token must be an address")
 			return
 		}

engine/quote.go

@@ -306,11 +306,11 @@ func (q *Quoter) serve(ctx context.Context, w http.ResponseWriter, req *quoteReq
 		tradeType = "EXACT_OUTPUT"
 	}
 
-	if !isHexAddress(req.TokenIn) {
+	if !IsAddress(req.TokenIn) {
 		badQuote(w, "tokenIn must be an address")
 		return
 	}
-	if !isHexAddress(req.TokenOut) {
+	if !IsAddress(req.TokenOut) {
 		badQuote(w, "tokenOut must be an address")
 		return
 	}
@@ -338,7 +338,7 @@ func (q *Quoter) serve(ctx context.Context, w http.ResponseWriter, req *quoteReq
 	}
 
 	swapper := nativeSentinel
-	if isHexAddress(req.Swapper) {
+	if IsAddress(req.Swapper) {
 		swapper = checksumAddress(req.Swapper)
 	}
 	slippage := 0.5
@@ -453,7 +453,7 @@ func (q *Quoter) book() []*pool {
 	raw := q.store.PoolsRaw()
 	out := make([]*pool, 0, len(raw))
 	for addr, p := range raw {
-		if p == nil || !isHexAddress(addr) || !isHexAddress(p.Token0) || !isHexAddress(p.Token1) {
+		if p == nil || !IsAddress(addr) || !IsAddress(p.Token0) || !IsAddress(p.Token1) {
 			continue
 		}
 		t0, t1 := strings.ToLower(p.Token0), strings.ToLower(p.Token1)
@@ -1044,7 +1044,10 @@ func bigString(n *big.Int) string {
 
 // ── addresses ─────────────────────────────────────────────────────────────
 
-func isHexAddress(s string) bool {
+// IsAddress reports whether s is a twenty-byte hex address. Exported because
+// the process that mounts these handlers decides whether a chain has a router
+// to address a transaction to, and that question has one answer.
+func IsAddress(s string) bool {
 	if len(s) != 42 || !strings.HasPrefix(s, "0x") {
 		return false
 	}

engine/quote_test.go

@@ -269,14 +269,14 @@ func TestParseWei(t *testing.T) {
 }
 
 func TestIsHexAddress(t *testing.T) {
-	if !isHexAddress(tokenA) || !isHexAddress("0x4888E4a2Ee0F03051c72D2BD3ACf755eD3498B3E") {
+	if !IsAddress(tokenA) || !IsAddress("0x4888E4a2Ee0F03051c72D2BD3ACf755eD3498B3E") {
 		t.Error("a 20-byte hex address must be accepted in either case")
 	}
 	for _, bad := range []string{"", "0x", "nothex", "4888e4a2ee0f03051c72d2bd3acf755ed3498b3e",
 		"0x4888e4a2ee0f03051c72d2bd3acf755ed3498b3", "0x4888e4a2ee0f03051c72d2bd3acf755ed3498b3ee",
 		"0xzzzz4a2ee0f03051c72d2bd3acf755ed3498b3e"} {
-		if isHexAddress(bad) {
-			t.Errorf("isHexAddress(%q) accepted", bad)
+		if IsAddress(bad) {
+			t.Errorf("IsAddress(%q) accepted", bad)
 		}
 	}
 }

engine/swap.go

@@ -91,12 +91,24 @@ func HandleSwap(chainID int64, router, wrapped string) http.HandlerFunc {
 			badQuote(w, "quote.route must contain at least one hop")
 			return
 		}
-		if !isHexAddress(q.Swapper) {
+		if !IsAddress(q.Swapper) {
 			badQuote(w, "quote.swapper must be an address")
 			return
 		}
 		swapper := checksumAddress(q.Swapper)
 
+		// A quote names the chain it was priced on. This process serves one
+		// chain, and building its router call from another chain's quote would
+		// address pools by the same numbers on a book that never quoted them —
+		// which on chains that share deployment addresses is a real trade at a
+		// price nobody was shown. The caller said which chain; believe it, and
+		// refuse when it is not this one.
+		if q.ChainID != 0 && q.ChainID != chainID {
+			badQuote(w, "quote is for chain "+strconv.FormatInt(q.ChainID, 10)+
+				", this one serves "+strconv.FormatInt(chainID, 10))
+			return
+		}
+
 		amountIn, okIn := parseWei(q.Input.Amount)
 		amountOut, okOut := parseWei(q.Output.Amount)
 		if !okIn || !okOut || amountIn.Sign() <= 0 || amountOut.Sign() <= 0 {
@@ -110,13 +122,14 @@ func HandleSwap(chainID int64, router, wrapped string) http.HandlerFunc {
 		tokens := make([]string, 0, len(hops)+1)
 		fees := make([]int64, 0, len(hops))
 		for i, h := range hops {
-			// A fee is a uint24, and the path packs it in exactly three bytes
-			// with an address either side. One that does not fit renders wider,
-			// shifting every byte after it, so the path would name real pools at
-			// addresses nobody asked for. A negative one renders as hex with a
-			// minus in it, which is not a number at all.
+			// An address is twenty bytes and a fee is a uint24. The path packs
+			// them end to end with nothing marking where one stops, so width is
+			// the only thing that says which is which. Anything narrower or
+			// wider shifts every byte after it and the path still parses — as a
+			// route through pools nobody named. A negative fee renders as hex
+			// with a minus in it, which is not a number at all.
 			fee, err := strconv.ParseInt(h.Fee, 10, 64)
-			if h.TokenIn.Address == "" || h.TokenOut.Address == "" || h.Fee == "" ||
+			if !IsAddress(h.TokenIn.Address) || !IsAddress(h.TokenOut.Address) ||
 				err != nil || fee < 0 || fee > 1<<24-1 {
 				badQuote(w, "quote.route hop "+strconv.Itoa(i)+" is missing tokenIn/tokenOut/fee")
 				return
@@ -128,9 +141,12 @@ func HandleSwap(chainID int64, router, wrapped string) http.HandlerFunc {
 			fees = append(fees, fee)
 		}
 
+		// Slippage is a percentage of the amount, so it lives between none and
+		// all of it. Outside that the factor it becomes goes negative or past
+		// what a word holds, and the bound stops being a bound.
 		slippage := 0.5
 		if q.Slippage != nil {
-			slippage = *q.Slippage
+			slippage = math.Min(100, math.Max(0, *q.Slippage))
 		}
 		exactIn := q.TradeType != "EXACT_OUTPUT"
 

engine/swap_test.go

@@ -2,6 +2,7 @@ package engine
 
 import (
 	"encoding/json"
+	"math/big"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -436,3 +437,95 @@ func TestSwapRefusesAFeeThatIsNotAUint24(t *testing.T) {
 		t.Errorf("calldata is %d hex chars after the selector, want %d", n, 7*64)
 	}
 }
+
+// An address is twenty bytes and the path packs it against a fee with nothing
+// marking the boundary. A short one shifts every byte after it and the path
+// still parses — as a route through pools nobody named — so it is refused
+// rather than signed.
+func TestSwapRefusesAHopAddressThatIsNotAnAddress(t *testing.T) {
+	for _, bad := range []string{"0xdead", "", "not-an-address", lusd + "00", "0x" + strings.Repeat("z", 40)} {
+		for _, where := range []string{"in", "out"} {
+			tin, tout := lusd, cyrus
+			if where == "in" {
+				tin = bad
+			} else {
+				tout = bad
+			}
+			code, out := serveSwap(t, `{"quote":{"chainId":96369,"swapper":"`+trader+`",
+				"input":{"token":"`+lusd+`","amount":"1000"},"output":{"token":"`+cyrus+`","amount":"1000"},
+				"tradeType":"EXACT_INPUT","slippage":0.5,
+				"route":[[{"type":"v3-pool","address":"0x0000000000000000000000000000000000000001",
+					"tokenIn":{"address":"`+tin+`","chainId":96369,"symbol":"A","decimals":"18"},
+					"tokenOut":{"address":"`+tout+`","chainId":96369,"symbol":"B","decimals":"6"},
+					"fee":"3000","amountIn":"1000","amountOut":"1000"}]]}}`)
+			if code != http.StatusBadRequest {
+				t.Errorf("token%s=%q: status = %d, want 400 (%v)", where, bad, code, out)
+			}
+		}
+	}
+}
+
+// A quote says which chain priced it. Building this chain's router call from
+// another chain's quote would address pools by the same numbers on a book that
+// never quoted them — and these chains share deployment addresses, so that is
+// a real trade at a price nobody was shown.
+func TestSwapRefusesAQuoteFromAnotherChain(t *testing.T) {
+	body := func(chainID string) string {
+		return `{"quote":{"chainId":` + chainID + `,"swapper":"` + trader + `",
+			"input":{"token":"` + lusd + `","amount":"1000"},"output":{"token":"` + cyrus + `","amount":"1000"},
+			"tradeType":"EXACT_INPUT","slippage":0.5,
+			"route":[[{"type":"v3-pool","address":"0x0000000000000000000000000000000000000001",
+				"tokenIn":{"address":"` + lusd + `","chainId":96369,"symbol":"LUSD","decimals":"18"},
+				"tokenOut":{"address":"` + cyrus + `","chainId":96369,"symbol":"CYRUS","decimals":"6"},
+				"fee":"3000","amountIn":"1000","amountOut":"1000"}]]}}`
+	}
+	code, out := serveSwap(t, body("200200"))
+	if code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want 400 for a quote priced on another chain: %v", code, out)
+	}
+	if d, _ := out["detail"].(string); !strings.Contains(d, "200200") || !strings.Contains(d, "96369") {
+		t.Errorf("detail = %q, want it to name both chains", d)
+	}
+	// This chain's own quote still works, and so does one that names no chain.
+	for _, id := range []string{"96369", "0"} {
+		if code, out := serveSwap(t, body(id)); code != http.StatusOK {
+			t.Errorf("chainId %s: status = %d, want 200 (%v)", id, code, out)
+		}
+	}
+}
+
+// Slippage is a percentage of an amount, so it lives between none of it and all
+// of it. Outside that the factor goes negative or past a word, and the bound
+// stops bounding: the calldata grows a character, or grows a minus sign.
+func TestSwapClampsSlippageToAPercentage(t *testing.T) {
+	const max = "115792089237316195423570985008687907853269984665640564039457584007913129639935"
+	check := func(t *testing.T, tradeType, slip, outAmt string, wantWord int, wantVal *big.Int) {
+		t.Helper()
+		got := swapCall(t, `{"quote":{"chainId":96369,"swapper":"`+trader+`",
+			"input":{"token":"`+lusd+`","amount":"1000"},"output":{"token":"`+cyrus+`","amount":"`+outAmt+`"},
+			"tradeType":"`+tradeType+`","slippage":`+slip+`,
+			"route":[[{"type":"v3-pool","address":"0x0000000000000000000000000000000000000001",
+				"tokenIn":{"address":"`+lusd+`","chainId":96369,"symbol":"LUSD","decimals":"18"},
+				"tokenOut":{"address":"`+cyrus+`","chainId":96369,"symbol":"CYRUS","decimals":"6"},
+				"fee":"3000","amountIn":"1000","amountOut":"`+outAmt+`"}]]}}`)
+		data, _ := got["data"].(string)
+		body := strings.TrimPrefix(data, "0x")[8:]
+		if len(body) != 7*64 {
+			t.Fatalf("slippage %s: %d hex chars after the selector, want %d — the calldata is misaligned", slip, len(body), 7*64)
+		}
+		if strings.ContainsAny(body, "-") {
+			t.Fatalf("slippage %s: calldata contains a minus sign: %s", slip, data)
+		}
+		if w := words(body); w[wantWord].Cmp(wantVal) != 0 {
+			t.Errorf("slippage %s: bound = %v, want %v", slip, w[wantWord], wantVal)
+		}
+	}
+	maxOut, _ := new(big.Int).SetString(max, 10)
+	// Below zero clamps to zero: the bound is the quoted amount itself.
+	check(t, "EXACT_INPUT", "-0.01", max, 5, maxOut)
+	check(t, "EXACT_INPUT", "-100", "1000", 5, big.NewInt(1000))
+	// Above a hundred clamps to a hundred, and for exact output the bound stops
+	// at a word rather than running past one.
+	check(t, "EXACT_OUTPUT", "-101", "1000", 5, big.NewInt(1000))
+	check(t, "EXACT_INPUT", "1000", "1000", 5, big.NewInt(0))
+}