feat: Verify transfer addresses in Observer (#7943) (#7956)

x/bridge/observer/bitcoin/bitcoin.go

@@ -5,7 +5,6 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"slices"
 	"strings"
 	"sync/atomic"
 	"time"
@@ -205,7 +204,7 @@ func (b *ChainClient) processTx(height uint64, tx *btcjson.TxRawResult) (observe
 
 	memo, contains := getMemo(tx)
 	if !contains {
-		return observer.Transfer{}, false, fmt.Errorf("failed to get Tx memo")
+		return observer.Transfer{}, false, fmt.Errorf("malformed Tx memo")
 	}
 
 	return observer.Transfer{
@@ -222,7 +221,7 @@ func (b *ChainClient) processTx(height uint64, tx *btcjson.TxRawResult) (observe
 
 // isTxRelevant checks if a tx contains transfers to the BTC vault and calculates the total amount to transfer.
 // This method goes through all the Vouts and accumulates their values if Vouts.account[0] == vault.
-// Returns true is the tx is not relevant. Otherwise, false and the total amount to transfer.
+// Returns false if the tx is not relevant. Otherwise, true and the total amount to transfer.
 func (b *ChainClient) isTxRelevant(tx *btcjson.TxRawResult) (math.Uint, bool) {
 	var amount = sdk.NewUint(0)
 	for _, vout := range tx.Vout {
@@ -238,7 +237,7 @@ func (b *ChainClient) isTxRelevant(tx *btcjson.TxRawResult) (math.Uint, bool) {
 			continue
 		}
 
-		if slices.Contains(addresses, b.vaultAddr) {
+		if addresses[0] == b.vaultAddr {
 			a, err := getAmount(vout)
 			if err != nil {
 				continue
@@ -273,7 +272,10 @@ func getMemo(tx *btcjson.TxRawResult) (string, bool) {
 				// TODO: log?
 				continue
 			}
-			// TODO: verify the memo format
+			_, err = sdk.AccAddressFromBech32(string(decoded))
+			if err != nil {
+				continue
+			}
 			return string(decoded), true
 		}
 	}

x/bridge/observer/bitcoin/bitcoin_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cometbft/cometbft/libs/log"
 	"github.com/stretchr/testify/require"
 
+	_ "github.com/osmosis-labs/osmosis/v24/app/params" // init Osmosis address prefixes
 	"github.com/osmosis-labs/osmosis/v24/x/bridge/observer"
 	"github.com/osmosis-labs/osmosis/v24/x/bridge/observer/bitcoin"
 	bridgetypes "github.com/osmosis-labs/osmosis/v24/x/bridge/types"
@@ -104,9 +105,16 @@ func TestListenOutboundTransfer(t *testing.T) {
 	err = b.Start(ctx)
 	require.NoError(t, err)
 
-	// We expect Observer to observe 1 block with 2 Txs
-	// Only 1 Tx is sent to our vault address,
-	// so we should receive only 1 TxIn
+	// We expect Observer to observe 1 block with 8 Txs:
+	// - tx to our vault but without memo
+	// - tx to our vault with memo with invalid address
+	// - tx to our vault but with zero tokens
+	// - tx to our vault with invalid script type for output Vout
+	// - tx to our vault with invalid script type for memo Vout
+	// - tx to our vault with invalid tokens amount
+	// - unrelated tx
+	// + valid tx to our vault
+	// So, we should receive only 1 Transfer
 	txs := b.ListenOutboundTransfer()
 	var out observer.Transfer
 	require.Eventually(t, func() bool {

x/bridge/observer/osmosis/osmosis.go

@@ -8,6 +8,9 @@ import (
 
 	errorsmod "cosmossdk.io/errors"
 	"cosmossdk.io/math"
+	"github.com/btcsuite/btcd/btcutil"
+	btcdchaincfg "github.com/btcsuite/btcd/chaincfg"
+	abcitypes "github.com/cometbft/cometbft/abci/types"
 	"github.com/cometbft/cometbft/libs/log"
 	rpchttp "github.com/cometbft/cometbft/rpc/client/http"
 	coretypes "github.com/cometbft/cometbft/rpc/core/types"
@@ -19,6 +22,13 @@ import (
 	bridgetypes "github.com/osmosis-labs/osmosis/v24/x/bridge/types"
 )
 
+type Mode = string
+
+const (
+	ModeMainnet Mode = "mainnet"
+	ModeTestnet Mode = "testnet"
+)
+
 var (
 	ModuleName    = "osmosis-chain"
 	OsmoGasLimit  = uint64(200000)
@@ -28,6 +38,7 @@ var (
 
 type ChainClient struct {
 	logger             log.Logger
+	mode               Mode
 	osmoClient         *Client
 	cometRpc           *rpchttp.HTTP
 	stopChan           chan struct{}
@@ -40,13 +51,15 @@ type ChainClient struct {
 // NewChainClient returns new instance of `Osmosis`
 func NewChainClient(
 	logger log.Logger,
+	mode Mode,
 	osmoClient *Client,
 	cometRpc *rpchttp.HTTP,
 	txConfig cosmosclient.TxConfig,
 	signerAddr string,
 ) *ChainClient {
 	return &ChainClient{
 		logger:             logger.With("module", ModuleName),
+		mode:               mode,
 		osmoClient:         osmoClient,
 		cometRpc:           cometRpc,
 		stopChan:           make(chan struct{}),
@@ -114,14 +127,17 @@ func (c *ChainClient) SignalInboundTransfer(ctx context.Context, in observer.Tra
 	if err != nil {
 		return errorsmod.Wrapf(err, "Failed to sign tx for inbound transfer %s", in.Id)
 	}
-	_, err = c.osmoClient.BroadcastTx(ctx, bytes)
+	resp, err := c.osmoClient.BroadcastTx(ctx, bytes)
 	if err != nil {
 		return errorsmod.Wrapf(
 			err,
 			"Failed to broadcast tx to Osmosis for inbound transfer %s",
 			in.Id,
 		)
 	}
+	if resp.Code != abcitypes.CodeTypeOK {
+		return fmt.Errorf("Tx for inbound transfer %s failed inside Osmosis: %s", in.Id, resp.RawLog)
+	}
 	return nil
 }
 
@@ -170,6 +186,7 @@ func (c *ChainClient) processNewBlockTxs(ctx context.Context, height uint64, txs
 			))
 			continue
 		}
+
 		if res.IsErr() {
 			continue
 		}
@@ -180,6 +197,21 @@ func (c *ChainClient) processNewBlockTxs(ctx context.Context, height uint64, txs
 				continue
 			}
 
+			err = verifyOutboundDestAddress(
+				c.mode,
+				observer.ChainId(outbound.AssetId.SourceChain),
+				outbound.DestAddr,
+			)
+			if err != nil {
+				c.logger.Error(fmt.Sprintf(
+					"Invalid outbound destination address in Tx %s, block %d: %s",
+					txHash,
+					height,
+					err.Error(),
+				))
+				continue
+			}
+
 			out := outboundTransferFromMsg(
 				height,
 				txHash,
@@ -218,6 +250,21 @@ func outboundTransferFromMsg(
 	}
 }
 
+func verifyOutboundDestAddress(mode Mode, chainId observer.ChainId, addr string) error {
+	switch chainId {
+	case observer.ChainIdBitcoin:
+		switch mode {
+		case ModeMainnet:
+			_, err := btcutil.DecodeAddress(addr, &btcdchaincfg.MainNetParams)
+			return err
+		case ModeTestnet:
+			_, err := btcutil.DecodeAddress(addr, &btcdchaincfg.TestNet3Params)
+			return err
+		}
+	}
+	return fmt.Errorf("Unsupported outbound destination chain: %s", chainId)
+}
+
 // Height returns current height of the chain
 func (c *ChainClient) Height(context.Context) (uint64, error) {
 	return c.lastObservedHeight.Load(), nil

x/bridge/observer/osmosis/osmosis_test.go

@@ -3,6 +3,7 @@ package osmosis_test
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -100,6 +101,7 @@ func NewOsmosisTestSuite(t *testing.T, ctx context.Context) OsmosisTestSuite {
 	client := osmosis.NewClient(ChainId, conn, kr, app.GetEncodingConfig().TxConfig)
 	o := osmosis.NewChainClient(
 		log.NewNopLogger(),
+		osmosis.ModeTestnet,
 		client,
 		cometRpc,
 		app.GetEncodingConfig().TxConfig,
@@ -134,13 +136,19 @@ func readNewBlockEvent(t *testing.T, path string) coretypes.ResultEvent {
 	return result
 }
 
-func readTxCheck(t *testing.T, path string) abci.ResponseCheckTx {
+func readTxCheckBytes(t *testing.T, id int, path string) []byte {
 	dataStr, err := os.ReadFile(path)
 	require.NoError(t, err)
 	result := abci.ResponseCheckTx{}
 	err = json.Unmarshal([]byte(dataStr), &result)
 	require.NoError(t, err)
-	return result
+	checkResultsResp := cmtrpctypes.NewRPCSuccessResponse(
+		cmtrpctypes.JSONRPCIntID(id),
+		result,
+	)
+	checkResultsRaw, err := json.Marshal(checkResultsResp)
+	require.NoError(t, err)
+	return checkResultsRaw
 }
 
 func success(t *testing.T) http.HandlerFunc {
@@ -150,7 +158,7 @@ func success(t *testing.T) http.HandlerFunc {
 			c, err := upgrader.Upgrade(w, r, nil)
 			require.NoError(t, err)
 			defer c.Close()
-			newBlock := readNewBlockEvent(t, "./test_events/new_block_success.json")
+			newBlock := readNewBlockEvent(t, "./test_events/new_block.json")
 			newBlockResp := cmtrpctypes.NewRPCSuccessResponse(
 				cmtrpctypes.JSONRPCIntID(1),
 				newBlock,
@@ -160,14 +168,25 @@ func success(t *testing.T) http.HandlerFunc {
 			err = c.WriteMessage(1, newBlockRaw)
 			require.NoError(t, err)
 		case http.MethodPost:
-			checkResults := readTxCheck(t, "./test_events/tx_check_success.json")
-			checkResultsResp := cmtrpctypes.NewRPCSuccessResponse(
-				cmtrpctypes.JSONRPCIntID(0),
-				checkResults,
-			)
-			checkResultsRaw, err := json.Marshal(checkResultsResp)
+			bytes, err := io.ReadAll(r.Body)
+			require.NoError(t, err)
+			require.NoError(t, r.Body.Close())
+			var req cmtrpctypes.RPCRequest
+			err = json.Unmarshal(bytes, &req)
 			require.NoError(t, err)
-			_, err = w.Write(checkResultsRaw)
+			jsonId, ok := req.ID.(cmtrpctypes.JSONRPCIntID)
+			require.True(t, ok)
+			id := int(jsonId)
+
+			var resp []byte
+			switch id {
+			case 1:
+				resp = readTxCheckBytes(t, id, "./test_events/tx_check_error.json")
+			default:
+				resp = readTxCheckBytes(t, id, "./test_events/tx_check_success.json")
+			}
+
+			_, err = w.Write(resp)
 			require.NoError(t, err)
 		default:
 			t.Fatal("Unexpected request method", r.Method)
@@ -282,6 +301,11 @@ func TestListenOutboundTransfer(t *testing.T) {
 	ots.Start(t, ctx)
 	defer ots.Stop(t, ctx)
 
+	// We expect to observe 1 block with 3 Txs each with a `MsgOutboundTransfer` message:
+	// - valid tx to BTC address
+	// - failed tx
+	// - tx with invalid destination address
+	// So, we should to receive only 1 Transfer
 	transfers := ots.o.ListenOutboundTransfer()
 	var transfer observer.Transfer
 	require.Eventually(t, func() bool {
@@ -292,8 +316,8 @@ func TestListenOutboundTransfer(t *testing.T) {
 	expTransfer := observer.Transfer{
 		SrcChain: observer.ChainIdOsmosis,
 		DstChain: observer.ChainIdBitcoin,
-		Id:       "8593aa191651f6a3e2978fb5334b3e5b1e20abd72ad539f15c76f241fa696d3e",
-		Height:   881,
+		Id:       "8eb4b69be7144690f82a4e1485f4b85d23adc5267db5d3dab7affae57c8ce2a4",
+		Height:   2801,
 		Sender:   "osmo1pldlhnwegsj3lqkarz0e4flcsay3fuqgkd35ww",
 		To:       "2Mt1ttL5yffdfCGxpfxmceNE4CRUcAsBbgQ",
 		Asset:    bridgetypes.DefaultBitcoinDenomName,

x/bridge/observer/osmosis/test_events/new_block_success.json → x/bridge/observer/osmosis/test_events/new_block.json

@@ -9,49 +9,51 @@
                         "block": "11"
                     },
                     "chain_id": "my-test-chain",
-                    "height": "881",
-                    "time": "2024-04-01T14:17:10.715933958Z",
+                    "height": "2801",
+                    "time": "2024-04-03T09:47:40.739349321Z",
                     "last_block_id": {
-                        "hash": "55E2A190F9F1877A1C390F557E3208E45F2C5BD2F782C76E03ACB4266B17A359",
+                        "hash": "82A6EA06C84B7BF17D3173E1232B5CE820DFC7D7417B2E141B0CDB8F855F3CA1",
                         "parts": {
                             "total": 1,
-                            "hash": "D8E296FACC516BD1010A1BB4C9ABCCC512E2821873956839AA17C10FA63A911E"
+                            "hash": "0D5C54C6C15823FFE26B70B62BDCC0C5402C41804FD3AE1F3A1728E84AEEDC5C"
                         }
                     },
-                    "last_commit_hash": "428E15CBA9DB1089C9FC9BACE274CEA0F2664115F365D4AC3069425BDAEE4035",
-                    "data_hash": "CF137839AB61FD2046F0553A2F986504D2F37497120456D03391A9FF21992993",
+                    "last_commit_hash": "1F5E3CFD4001F02DFC16457F8E4B297EF43FF3AA9F989BD8F95F0F7E13990355",
+                    "data_hash": "70CF44DFF1E91F23C1474508793250E0E97BB5647D6E495A7378438E67389DB7",
                     "validators_hash": "1E811B4CEEF78E840C253AB5928C670A6A6B389A231496E9ABF8084FD6206FAF",
                     "next_validators_hash": "1E811B4CEEF78E840C253AB5928C670A6A6B389A231496E9ABF8084FD6206FAF",
                     "consensus_hash": "048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F",
-                    "app_hash": "281B4F7AFCF115D4F72A47A1BDC03A59F180C32E9719FED4D48C3A6FCC2C7DAF",
+                    "app_hash": "7573ABD8364FF3B5F118F7A64FF1BE39268CA710A72952021CE7CB1C267F4B89",
                     "last_results_hash": "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855",
                     "evidence_hash": "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855",
                     "proposer_address": "B81ECB7226036A4B8ACFC81EBDF24A2527D25BE8"
                 },
                 "data": {
                     "txs": [
-                        "CpgBCpUBCisvb3Ntb3Npcy5icmlkZ2UudjFiZXRhMS5Nc2dPdXRib3VuZFRyYW5zZmVyEmYKK29zbW8xcGxkbGhud2Vnc2ozbHFrYXJ6MGU0Zmxjc2F5M2Z1cWdrZDM1d3cSIzJNdDF0dEw1eWZmZGZDR3hwZnhtY2VORTRDUlVjQXNCYmdRGg4KB2JpdGNvaW4SA2J0YyICMTASZwpQCkYKHy9jb3Ntb3MuY3J5cHRvLnNlY3AyNTZrMS5QdWJLZXkSIwohAqGxGGMLwZu1ElZA/wZ9hdIPpYUzRWaKUtzzwx6sRbZlEgQKAggBGAgSEwoNCgVzdGFrZRIEMTAwMBDAmgwaQGATrlA3NQt5cMVeUjt1i0YngPv2SP3cdob0wJlZekreByBudAJnxJpWhLCuWKkrAZVCaXtHrV4SoYUsvHfKhMg="
+                        "CqABCp0BCisvb3Ntb3Npcy5icmlkZ2UudjFiZXRhMS5Nc2dPdXRib3VuZFRyYW5zZmVyEm4KK29zbW8xcGxkbGhud2Vnc2ozbHFrYXJ6MGU0Zmxjc2F5M2Z1cWdrZDM1d3cSK29zbW8xbWZrbDRwOTJscXZsZjdmaDVycGRzM2FmdnBhc2U3OGZ6NWgzNmwaDgoHYml0Y29pbhIDYnRjIgIxMBJnClAKRgofL2Nvc21vcy5jcnlwdG8uc2VjcDI1NmsxLlB1YktleRIjCiECobEYYwvBm7USVkD/Bn2F0g+lhTNFZopS3PPDHqxFtmUSBAoCCAEYDRITCg0KBXN0YWtlEgQxMDAwEMCaDBpAeyBpiYpl0iBgBB2g5OZyvKSgYT9pOrW4uxtsVV/IZ1pvbA1hcjXKNk9IKyL8Gcv6eS05ZXSbBmwtBY3L7Dey1w==",
+                        "CpgBCpUBCisvb3Ntb3Npcy5icmlkZ2UudjFiZXRhMS5Nc2dPdXRib3VuZFRyYW5zZmVyEmYKK29zbW8xcGxkbGhud2Vnc2ozbHFrYXJ6MGU0Zmxjc2F5M2Z1cWdrZDM1d3cSIzJNdDF0dEw1eWZmZGZDR3hwZnhtY2VORTRDUlVjQXNCYmdRGg4KB2JpdGNvaW4SA2J0YyICMTASZwpQCkYKHy9jb3Ntb3MuY3J5cHRvLnNlY3AyNTZrMS5QdWJLZXkSIwohAqGxGGMLwZu1ElZA/wZ9hdIPpYUzRWaKUtzzwx6sRbZlEgQKAggBGA4SEwoNCgVzdGFrZRIEMTAwMBDAmgwaQIvlf1BifOQRKamSTBaUK05SUMzybrfYfNYH3vvEBpuFW/w2H4p7YIPYKnhsdYv7D1pZCpFHfNZ5rScRIjnFlwg=",
+                        "CpgBCpUBCisvb3Ntb3Npcy5icmlkZ2UudjFiZXRhMS5Nc2dPdXRib3VuZFRyYW5zZmVyEmYKK29zbW8xcGxkbGhud2Vnc2ozbHFrYXJ6MGU0Zmxjc2F5M2Z1cWdrZDM1d3cSIzJNdDF0dEw1eWZmZGZDR3hwZnhtY2VORTRDUlVjQXNCYmdRGg4KB2JpdGNvaW4SA2J0YyICMTASZwpQCkYKHy9jb3Ntb3MuY3J5cHRvLnNlY3AyNTZrMS5QdWJLZXkSIwohAqGxGGMLwZu1ElZA/wZ9hdIPpYUzRWaKUtzzwx6sRbZlEgQKAggBGA8SEwoNCgVzdGFrZRIEMTAwMBDAmgwaQLlOXD9HIHgLrYbxtCyViEoaqlosMgqqGhHux4fQHPZ0E/rfxPNhq4lpJ9LzW4u2WKsmNsEMLYfMtIwd4IgScy4="
                     ]
                 },
                 "evidence": {
                     "evidence": null
                 },
                 "last_commit": {
-                    "height": "880",
+                    "height": "2800",
                     "round": 0,
                     "block_id": {
-                        "hash": "55E2A190F9F1877A1C390F557E3208E45F2C5BD2F782C76E03ACB4266B17A359",
+                        "hash": "82A6EA06C84B7BF17D3173E1232B5CE820DFC7D7417B2E141B0CDB8F855F3CA1",
                         "parts": {
                             "total": 1,
-                            "hash": "D8E296FACC516BD1010A1BB4C9ABCCC512E2821873956839AA17C10FA63A911E"
+                            "hash": "0D5C54C6C15823FFE26B70B62BDCC0C5402C41804FD3AE1F3A1728E84AEEDC5C"
                         }
                     },
                     "signatures": [
                         {
                             "block_id_flag": 2,
                             "validator_address": "B81ECB7226036A4B8ACFC81EBDF24A2527D25BE8",
-                            "timestamp": "2024-04-01T14:17:10.715933958Z",
-                            "signature": "yv+UN4V8RfB7JceRW2QrWLOlIyZwlp5421aAC9UH6BL3OBIIKHDGhy5nPFDQ19BQIreejLC1LSkbwhDSjMhGBw=="
+                            "timestamp": "2024-04-03T09:47:40.739349321Z",
+                            "signature": "qyR90k0Xw6WnZ8lORWtzcS3JJcFoA5tc6ahG32TXle/frx6xUgZ3gHRxwA/Ofy2CEnB5Y0MGkDYg9Jfy18ZQAw=="
                         }
                     ]
                 }

x/bridge/observer/osmosis/test_events/tx_check_error.json

@@ -0,0 +1,13 @@
+{
+    "code": 32,
+    "data": null,
+    "log": "account sequence mismatch, expected 10, got 9: incorrect account sequence",
+    "info": "",
+    "gas_wanted": "200000",
+    "gas_used": "39457",
+    "events": [],
+    "codespace": "sdk",
+    "sender": "",
+    "priority": "0",
+    "mempoolError": ""
+}