From e2a196955be3e8f813c5322b9cb8b33be5866e8d Mon Sep 17 00:00:00 2001
From: Alex Johnson <alex@skip.money>
Date: Thu, 21 Mar 2024 12:01:43 -0400
Subject: [PATCH] fix: market map validation (#228)

* add note:

* field len

* field len

* fix

* tickerstr

* route

* big

---------

Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>
---
 pkg/types/currency_pair.go         | 16 +++++++++++++++-
 pkg/types/currency_pair_test.go    | 17 +++++++++++++++++
 tests/integration/go.mod           |  4 ++--
 tests/integration/go.sum           |  2 ++
 testutil/testutil.go               | 13 +++++++++++++
 x/marketmap/keeper/keeper.go       |  8 +++++---
 x/marketmap/types/market.go        |  4 ++--
 x/marketmap/types/msg.go           |  2 +-
 x/marketmap/types/path.go          |  8 +++++++-
 x/marketmap/types/path_test.go     | 25 +++++++++++++++++++++++++
 x/marketmap/types/provider.go      | 13 +++++++++++++
 x/marketmap/types/provider_test.go | 19 +++++++++++++++++--
 x/marketmap/types/ticker.go        |  6 ++++++
 x/marketmap/types/ticker_test.go   | 15 +++++++++++++++
 14 files changed, 140 insertions(+), 12 deletions(-)
 create mode 100644 testutil/testutil.go

diff --git a/pkg/types/currency_pair.go b/pkg/types/currency_pair.go
index eaeed17c7..7f359192a 100644
--- a/pkg/types/currency_pair.go
+++ b/pkg/types/currency_pair.go
@@ -6,7 +6,8 @@ import (
 )
 
 const (
-	ethereum = "ETHEREUM"
+	ethereum         = "ETHEREUM"
+	MaxCPFieldLength = 128
 )
 
 // NewCurrencyPair returns a new CurrencyPair with the given base and quote strings.
@@ -31,9 +32,22 @@ func (cp *CurrencyPair) ValidateBasic() error {
 	if strings.ToUpper(cp.Quote) != cp.Quote {
 		return fmt.Errorf("incorrectly formatted quote string, expected: %s got: %s", strings.ToUpper(cp.Quote), cp.Quote)
 	}
+
+	if len(cp.Base) > MaxCPFieldLength || len(cp.Quote) > MaxCPFieldLength {
+		return fmt.Errorf("string field exceeds maximum length of %d", MaxCPFieldLength)
+	}
+
 	return nil
 }
 
+// Invert returns an inverted version of cp (where the Base and Quote are swapped).
+func (cp *CurrencyPair) Invert() CurrencyPair {
+	return CurrencyPair{
+		Base:  cp.Quote,
+		Quote: cp.Base,
+	}
+}
+
 // String returns a string representation of the CurrencyPair, in the following form "ETH/BTC".
 func (cp CurrencyPair) String() string {
 	return fmt.Sprintf("%s/%s", cp.Base, cp.Quote)
diff --git a/pkg/types/currency_pair_test.go b/pkg/types/currency_pair_test.go
index 7c9565062..72f34774e 100644
--- a/pkg/types/currency_pair_test.go
+++ b/pkg/types/currency_pair_test.go
@@ -6,6 +6,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	slinkytypes "github.com/skip-mev/slinky/pkg/types"
+	"github.com/skip-mev/slinky/testutil"
 )
 
 func TestValidateBasic(t *testing.T) {
@@ -46,6 +47,22 @@ func TestValidateBasic(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"Base is too long - fail",
+			slinkytypes.CurrencyPair{
+				Base:  testutil.RandomString(slinkytypes.MaxCPFieldLength + 1),
+				Quote: "AA",
+			},
+			false,
+		},
+		{
+			"Quote is too long - fail",
+			slinkytypes.CurrencyPair{
+				Base:  "BB",
+				Quote: testutil.RandomString(slinkytypes.MaxCPFieldLength + 1),
+			},
+			false,
+		},
 		{
 			"if both Quote + Base are formatted correctly - pass",
 			slinkytypes.CurrencyPair{
diff --git a/tests/integration/go.mod b/tests/integration/go.mod
index a08e65666..22d61e554 100644
--- a/tests/integration/go.mod
+++ b/tests/integration/go.mod
@@ -194,7 +194,7 @@ require (
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/crypto v0.21.0 // indirect
 	golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 // indirect
-	golang.org/x/mod v0.15.0 // indirect
+	golang.org/x/mod v0.16.0 // indirect
 	golang.org/x/net v0.22.0 // indirect
 	golang.org/x/oauth2 v0.16.0 // indirect
 	golang.org/x/sync v0.6.0 // indirect
@@ -202,7 +202,7 @@ require (
 	golang.org/x/term v0.18.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	golang.org/x/time v0.5.0 // indirect
-	golang.org/x/tools v0.18.0 // indirect
+	golang.org/x/tools v0.19.0 // indirect
 	google.golang.org/api v0.162.0 // indirect
 	google.golang.org/appengine v1.6.8 // indirect
 	google.golang.org/genproto v0.0.0-20240213162025-012b6fc9bca9 // indirect
diff --git a/tests/integration/go.sum b/tests/integration/go.sum
index 181cfe152..f52a61016 100644
--- a/tests/integration/go.sum
+++ b/tests/integration/go.sum
@@ -1160,6 +1160,7 @@ golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
 golang.org/x/mod v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8=
 golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
+golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -1449,6 +1450,7 @@ golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
 golang.org/x/tools v0.18.0 h1:k8NLag8AGHnn+PHbl7g43CtqZAwG60vZkLqgyZgIHgQ=
 golang.org/x/tools v0.18.0/go.mod h1:GL7B4CwcLLeo59yx/9UWWuNOW1n3VZ4f5axWfML7Lcg=
+golang.org/x/tools v0.19.0/go.mod h1:qoJWxmGSIBmAeriMx19ogtrEPrGtDbPK634QFIcLAhc=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
diff --git a/testutil/testutil.go b/testutil/testutil.go
new file mode 100644
index 000000000..b9d73c1b6
--- /dev/null
+++ b/testutil/testutil.go
@@ -0,0 +1,13 @@
+package testutil
+
+import "math/rand"
+
+// RandomString generates a random string of length N.
+func RandomString(length int) string {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+	result := make([]byte, length)
+	for i := range result {
+		result[i] = charset[rand.Intn(len(charset))]
+	}
+	return string(result)
+}
diff --git a/x/marketmap/keeper/keeper.go b/x/marketmap/keeper/keeper.go
index 8f623ebd2..211f906c2 100644
--- a/x/marketmap/keeper/keeper.go
+++ b/x/marketmap/keeper/keeper.go
@@ -115,16 +115,18 @@ func (k *Keeper) GetAllTickersMap(ctx sdk.Context) (map[string]types.Ticker, err
 // CreateTicker initializes a new Ticker.
 // The Ticker.String corresponds to a market, and must be unique.
 func (k *Keeper) CreateTicker(ctx sdk.Context, ticker types.Ticker) error {
+	tickerString := types.TickerString(ticker.String())
+
 	// Check if Ticker already exists for the provider
-	alreadyExists, err := k.tickers.Has(ctx, types.TickerString(ticker.String()))
+	alreadyExists, err := k.tickers.Has(ctx, tickerString)
 	if err != nil {
 		return err
 	}
 	if alreadyExists {
-		return types.NewTickerAlreadyExistsError(types.TickerString(ticker.String()))
+		return types.NewTickerAlreadyExistsError(tickerString)
 	}
 	// Create the config
-	return k.tickers.Set(ctx, types.TickerString(ticker.String()), ticker)
+	return k.tickers.Set(ctx, tickerString, ticker)
 }
 
 // GetAllProvidersMap returns the set of Providers objects currently stored in state
diff --git a/x/marketmap/types/market.go b/x/marketmap/types/market.go
index fa7569390..9d803e1df 100644
--- a/x/marketmap/types/market.go
+++ b/x/marketmap/types/market.go
@@ -75,7 +75,7 @@ func (mm *MarketMap) String() string {
 // ValidateIndexPriceAggregation validates the market map configuration and its expected configuration for
 // this aggregator. In particular, this will
 //
-//  1. Ensure that the market map is valid (ValidateBasic). This ensure's that each of the provider's
+//  1. Ensure that the market map is valid (ValidateBasic). This ensures that each of the provider's
 //     markets are supported by the market map.
 //  2. Ensure that each path has a corresponding ticker.
 //  3. Ensure that each path has a valid number of operations.
@@ -84,7 +84,7 @@ func ValidateIndexPriceAggregation(
 	marketMap MarketMap,
 ) error {
 	for ticker, paths := range marketMap.Paths {
-		// The ticker must be supported by the market map. Otherwise we do not how to resolve the
+		// The ticker must be supported by the market map. Otherwise, we do not how to resolve the
 		// prices.
 		if _, ok := marketMap.Tickers[ticker]; !ok {
 			return fmt.Errorf("path includes a ticker that is not supported: %s", ticker)
diff --git a/x/marketmap/types/msg.go b/x/marketmap/types/msg.go
index e77e8a5cc..fe57efaac 100644
--- a/x/marketmap/types/msg.go
+++ b/x/marketmap/types/msg.go
@@ -37,7 +37,7 @@ func (m *MsgUpdateMarketMap) ValidateBasic() error {
 			)
 		}
 
-		seenProviders := make(map[string]struct{})
+		seenProviders := make(map[string]struct{}, len(market.Providers.Providers))
 		for _, provider := range market.Providers.Providers {
 			if err := provider.ValidateBasic(); err != nil {
 				return err
diff --git a/x/marketmap/types/path.go b/x/marketmap/types/path.go
index e7226aab3..06269b4c1 100644
--- a/x/marketmap/types/path.go
+++ b/x/marketmap/types/path.go
@@ -109,6 +109,12 @@ func (p *Path) ValidateBasic() error {
 			return err
 		}
 
+		if op.Invert {
+			if _, ok := seen[op.CurrencyPair.Invert()]; ok {
+				return fmt.Errorf("duplicated pair found")
+			}
+		}
+
 		if _, ok := seen[op.CurrencyPair]; ok {
 			return fmt.Errorf("path is not a directed acyclic graph")
 		}
@@ -166,7 +172,7 @@ func (p *Paths) ValidateBasic(cp slinkytypes.CurrencyPair) error {
 
 		route := path.ShowRoute()
 		if _, ok := routes[route]; ok {
-			return fmt.Errorf("duplicate path found: %s", route)
+			return fmt.Errorf("duplicate route found: %s", route)
 		}
 		routes[route] = struct{}{}
 
diff --git a/x/marketmap/types/path_test.go b/x/marketmap/types/path_test.go
index 5aa03e803..4e8cc62cd 100644
--- a/x/marketmap/types/path_test.go
+++ b/x/marketmap/types/path_test.go
@@ -143,6 +143,15 @@ var (
 		},
 	}
 
+	usdteth = types.Ticker{
+		CurrencyPair: slinkytypes.CurrencyPair{
+			Base:  "USDT",
+			Quote: "ETHEREUM",
+		},
+		Decimals:         8,
+		MinProviderCount: 1,
+	}
+
 	tickers = map[string]types.Ticker{
 		btcusdt.String(): btcusdt,
 		usdcusd.String(): usdcusd,
@@ -319,6 +328,22 @@ func TestPath(t *testing.T) {
 			target: "",
 			expErr: true,
 		},
+		{
+			name: "invalid path with multiple operations inverted duplicate",
+			path: types.Path{
+				Operations: []types.Operation{
+					{
+						CurrencyPair: ethusdt.CurrencyPair,
+					},
+					{
+						CurrencyPair: usdteth.CurrencyPair,
+						Invert:       true,
+					},
+				},
+			},
+			target: "",
+			expErr: true,
+		},
 		{
 			name: "valid path with a single operation",
 			path: types.Path{
diff --git a/x/marketmap/types/provider.go b/x/marketmap/types/provider.go
index 2b752dcec..bc51ccc0e 100644
--- a/x/marketmap/types/provider.go
+++ b/x/marketmap/types/provider.go
@@ -4,6 +4,11 @@ import (
 	"fmt"
 )
 
+const (
+	MaxProviderNameFieldLength   = 128
+	MaxProviderTickerFieldLength = 256
+)
+
 // ValidateBasic performs basic validation on Providers.
 func (p *Providers) ValidateBasic() error {
 	for _, provider := range p.Providers {
@@ -21,10 +26,18 @@ func (pc *ProviderConfig) ValidateBasic() error {
 		return fmt.Errorf("provider name must not be empty")
 	}
 
+	if len(pc.Name) > MaxProviderNameFieldLength {
+		return fmt.Errorf("provider length is longer than maximum length of %d", MaxProviderNameFieldLength)
+	}
+
 	if len(pc.OffChainTicker) == 0 {
 		return fmt.Errorf("provider offchain ticker must not be empty")
 	}
 
+	if len(pc.OffChainTicker) > MaxProviderTickerFieldLength {
+		return fmt.Errorf("provider offchain ticker is longer than maximum length of %d", MaxProviderTickerFieldLength)
+	}
+
 	return nil
 }
 
diff --git a/x/marketmap/types/provider_test.go b/x/marketmap/types/provider_test.go
index 5fe71bb64..a66dcccaf 100644
--- a/x/marketmap/types/provider_test.go
+++ b/x/marketmap/types/provider_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/skip-mev/slinky/testutil"
 	"github.com/skip-mev/slinky/x/marketmap/types"
 )
 
@@ -16,20 +17,34 @@ func TestProviderConfigValidateBasic(t *testing.T) {
 		}
 		require.NoError(t, pc.ValidateBasic())
 	})
-	t.Run("invalid name - fail", func(t *testing.T) {
+	t.Run("invalid empty name - fail", func(t *testing.T) {
 		pc := types.ProviderConfig{
 			Name:           "",
 			OffChainTicker: "ticker",
 		}
 		require.Error(t, pc.ValidateBasic())
 	})
-	t.Run("valid offchain ticker - fail", func(t *testing.T) {
+	t.Run("invalid empty offchain ticker - fail", func(t *testing.T) {
 		pc := types.ProviderConfig{
 			Name:           "mexc",
 			OffChainTicker: "",
 		}
 		require.Error(t, pc.ValidateBasic())
 	})
+	t.Run("invalid too long name - fail", func(t *testing.T) {
+		pc := types.ProviderConfig{
+			Name:           testutil.RandomString(types.MaxProviderNameFieldLength + 1),
+			OffChainTicker: "ticker",
+		}
+		require.Error(t, pc.ValidateBasic())
+	})
+	t.Run("invalid too long offchain ticker - fail", func(t *testing.T) {
+		pc := types.ProviderConfig{
+			Name:           "mexc",
+			OffChainTicker: testutil.RandomString(types.MaxProviderTickerFieldLength + 1),
+		}
+		require.Error(t, pc.ValidateBasic())
+	})
 }
 
 func TestProvidersEqual(t *testing.T) {
diff --git a/x/marketmap/types/ticker.go b/x/marketmap/types/ticker.go
index 85232a9f6..b8321606d 100644
--- a/x/marketmap/types/ticker.go
+++ b/x/marketmap/types/ticker.go
@@ -17,6 +17,8 @@ const (
 	DefaultMinProviderCount = 1
 	// MaxPathLength is the maximum length of a path for a ticker conversion.
 	MaxPathLength = 3
+	// MaxMetadataJSONFieldLength is the maximum length of the MetadataJSON field.
+	MaxMetadataJSONFieldLength = 16384
 )
 
 // NewTicker returns a new Ticker instance. A Ticker represents a price feed for
@@ -51,6 +53,10 @@ func (t *Ticker) ValidateBasic() error {
 		return err
 	}
 
+	if len(t.Metadata_JSON) > MaxMetadataJSONFieldLength {
+		return fmt.Errorf("metadata json field is longer than maximum length of %d", MaxMetadataJSONFieldLength)
+	}
+
 	return json.IsValid([]byte(t.Metadata_JSON))
 }
 
diff --git a/x/marketmap/types/ticker_test.go b/x/marketmap/types/ticker_test.go
index 174c1b2c0..1716a8e48 100644
--- a/x/marketmap/types/ticker_test.go
+++ b/x/marketmap/types/ticker_test.go
@@ -3,6 +3,8 @@ package types_test
 import (
 	"testing"
 
+	"github.com/skip-mev/slinky/testutil"
+
 	"github.com/stretchr/testify/require"
 
 	slinkytypes "github.com/skip-mev/slinky/pkg/types"
@@ -27,6 +29,19 @@ func TestTicker(t *testing.T) {
 			},
 			expErr: false,
 		},
+		{
+			name: "invalid metadata length",
+			ticker: types.Ticker{
+				CurrencyPair: slinkytypes.CurrencyPair{
+					Base:  "BITCOIN",
+					Quote: "USDT",
+				},
+				Decimals:         8,
+				MinProviderCount: 1,
+				Metadata_JSON:    testutil.RandomString(types.MaxMetadataJSONFieldLength + 1),
+			},
+			expErr: true,
+		},
 		{
 			name: "empty base",
 			ticker: types.Ticker{