fix: ibc transfer memo and receiver length check (#2551)

CHANGELOG.md

@@ -44,7 +44,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 # Changelog
 
-## Unreleased
+## v6.5.0
+
+### Bug Fixes
+
+- [2551](https://github.com/umee-network/umee/pull/2551) Restrict length of IBC transfer memo and receiver fields.
 
 ## v6.4.1 - 2024-04-30
 

RELEASE_NOTES.md

@@ -6,64 +6,10 @@
 
 The Release Procedure is defined in the [CONTRIBUTING](CONTRIBUTING.md#release-procedure) document.
 
-## v6.4.1
+## v6.5.0
 
-This release updates our dependencies and applies latest patches to the v6.4.x line. All validators must update to this patch release.
+In this release, we are introducing validations for the IBC transfer message receiver address and memo fields. These enhancements aim to address and resolve the recent incident involving spam IBC transfer transactions.
 
-## v6.4.0
-
-Highlights:
-
-- Cosmos SDK v0.47.10 patch update.
-- IBC Hooks: we integrated ICS20 Memo handling.
-- Integrated Packet Forwarding Middleware.
-- Update `uibc/MsgGovUpdateQuota` Msg type to handle the new inflow parameters.
-- Update `uibc/QueryAllOutflowsResponse` to include denom symbol (token name) in every outflow.
-
-[CHANGELOG](CHANGELOG.md)
-
-### IBC Hooks
-
-This release brings the first part of the seamless cross-chain money market transactions. At UX, we want to provide the best User Experience for handling lending and leverage. In this release, we support the following `x/leverage` messages:
-
-- `MsgSupply`
-- `MsgSupplyCollateral`
-- `MsgLiquidate`
-
-The operation can only use tokens that are part of the IBC transfer (after any intermediate deductions) and the supplier / liquidator must be the IBC recipient (acting on someone else's behalf is not allowed). Authz is not supported. The remaining tokens will be credited to the recipient.
-
-Documentation: [x/uibc/README.md](https://github.com/umee-network/umee/blob/v6.4.0/x/uibc/README.md#ibc-ics20-hooks)
-
-### Validators
-
-**Upgrade Title** (for Cosmovisor): **v6.4**.
-
-Update Price Feeder to `umee/2.4.1+`.
-
-NOTE: after the upgrade, you should restart your Price Feeder. We observed that Price Feeder doesn't correctly re-established a connection after the chain upgrade.
-
-#### libwasmvm update
-
-Our dependencies have been updated. The binary requires `libwasmvm v1.5.2`. When you build the binary from source on the server machine you probably don't need any change. However, when you download a binary from GitHub, or from another source, make sure you update the `/usr/lib/libwasmvm.<cpu_arch>.so`. For example:
-
-- copy from `$GOPATH/pkg/mod/github.com/!cosm!wasm/[email protected]/internal/api/libwasmvm.$(uname -m).so`
-- or download from github `wget https://raw.githubusercontent.com/CosmWasm/wasmvm/v1.5.2/internal/api/libwasmvm.$(uname -m).so -O /lib/libwasmvm.$(uname -m).so`
-
-You don't need to do anything if you are using our Docker image.
-
-### Upgrade instructions
-
-- Download latest binary or build from source.
-- Make sure `libwasmvm.$(uname -m).so` is properly linked
-  - Run the binary to make sure it works for you: `umeed version`
-- Wait for software upgrade proposal to pass and trigger the chain upgrade.
-- Swap binaries.
-- Ensure latest Price Feeder (see [compatibility matrix](https://github.com/umee-network/umee/#release-compatibility-matrix)) is running and ensure your price feeder configuration is up-to-date.
-- Restart the chain.
-- Restart Price Feeder.
-
-You can use Cosmovisor → see [instructions](https://github.com/umee-network/umee/#cosmovisor).
-
-#### Docker
-
-Docker images are available in [ghcr.io umee-network](https://github.com/umee-network/umee/pkgs/container/umeed) repository.
+- Maximum length for IBC transfer memo field: 32,768 characters
+- Maximum length for IBC transfer receiver address field: 2,048 characters
+

app/upgrades.go

@@ -49,8 +49,9 @@ func (app UmeeApp) RegisterUpgradeHandlers() {
 	app.registerOutdatedPlaceholderUpgrade("v6.1")
 	app.registerOutdatedPlaceholderUpgrade("v6.2")
 	app.registerUpgrade("v6.3", upgradeInfo)
-
 	app.registerUpgrade6_4(upgradeInfo)
+
+	app.registerUpgrade("v6.5", upgradeInfo)
 }
 
 func (app *UmeeApp) registerUpgrade6_4(upgradeInfo upgradetypes.Plan) {

tests/e2e/e2e_ibc_test.go

@@ -11,7 +11,9 @@ import (
 	appparams "github.com/umee-network/umee/v6/app/params"
 	setup "github.com/umee-network/umee/v6/tests/e2e/setup"
 	"github.com/umee-network/umee/v6/tests/grpc"
+	"github.com/umee-network/umee/v6/tests/tsdk"
 	"github.com/umee-network/umee/v6/util/coin"
+	ibcutil "github.com/umee-network/umee/v6/util/ibc"
 	"github.com/umee-network/umee/v6/x/uibc"
 )
 
@@ -164,6 +166,14 @@ func (s *E2ETest) TestIBCTokenTransfer() {
 		// supply don't change
 		s.checkSupply(gaiaAPIEndpoint, umeeIBCHash, math.ZeroInt())
 
+		// << Receiver Addr = maximum length + 1
+		// send $110 UMEE from umee to gaia (token_quota is 100$)
+		recvAddr := tsdk.GenerateString(ibcutil.MaximumMemoLength + 1)
+		s.SendIBC(s.Chain.ID, setup.GaiaChainID, recvAddr, exceedUmee, true, "", "")
+		// check the ibc (umee) quota after ibc txs - this one should have failed
+		// supply don't change
+		s.checkSupply(gaiaAPIEndpoint, umeeIBCHash, math.ZeroInt())
+
 		// send $110 ATOM from umee to gaia
 		exceedAtom := mulCoin(atomQuota, "1.1")
 		// supply will be not be decreased because sending amount is more than token quota so it will fail

tests/e2e/setup/utils.go

@@ -142,10 +142,14 @@ func (s *E2ETestSuite) SendIBC(srcChainID, dstChainID, recipient string, token s
 			if i < 4 {
 				continue
 			}
-			s.Require().Failf("failed to find transaction hash in output outBuf: %s  errBuf: %s", outBuf.String(), errBuf.String())
+			if !strings.Contains(outBuf.String(), "bad packet in rate limit's SendPacket") {
+				s.Require().Failf("failed to find transaction hash in output outBuf: %s  errBuf: %s",
+					outBuf.String(), errBuf.String())
+			}
 		}
 
-		// s.Require().NotEmptyf(txHash, "failed to find transaction hash in output outBuf: %s  errBuf: %s", outBuf.String(), errBuf.String())
+		// s.Require().NotEmptyf(txHash, "failed to find transaction hash in output outBuf: %s
+		// errBuf: %s", outBuf.String(), errBuf.String())
 		// endpoint := s.UmeeREST()
 		// if strings.Contains(srcChainID, "gaia") {
 		// 	endpoint = s.GaiaREST()

tests/tsdk/string.go

@@ -0,0 +1,13 @@
+package tsdk
+
+import "math/rand"
+
+func GenerateString(length uint) string {
+	// character set used for generating a random string in GenerateString
+	charset := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+	bytes := make([]byte, length)
+	for i := range bytes {
+		bytes[i] = charset[rand.Intn(len(charset))] //nolint
+	}
+	return string(bytes)
+}

tests/tsdk/string_test.go

@@ -0,0 +1,15 @@
+package tsdk_test
+
+import (
+	"testing"
+
+	"github.com/umee-network/umee/v6/tests/tsdk"
+	"gotest.tools/v3/assert"
+)
+
+// TestGenerateString checks the randomness and length properties of the generated string.
+func TestGenerateString(t *testing.T) {
+	length := 10
+	str := tsdk.GenerateString(uint(length))
+	assert.Equal(t, len(str), length, "Generated string length should match the input length")
+}

util/ibc/ibc.go

@@ -2,13 +2,33 @@ package ibc
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	sdkmath "cosmossdk.io/math"
-	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
+	ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors"
 )
 
+const (
+	MaximumReceiverLength = 2048  // maximum length of the receiver address in bytes (value chosen arbitrarily)
+	MaximumMemoLength     = 32768 // maximum length of the memo in bytes (value chosen arbitrarily)
+)
+
+func ValidateRecvAddr(receiver string) error {
+	if len(receiver) > MaximumReceiverLength {
+		return ibcerrors.ErrInvalidAddress.Wrapf("recipient address must not exceed %d bytes", MaximumReceiverLength)
+	}
+	return nil
+}
+
+func ValidateMemo(memo string) error {
+	if len(memo) > MaximumMemoLength {
+		return fmt.Errorf("memo must not exceed %d bytes", MaximumMemoLength)
+	}
+	return nil
+}
+
 // GetFundsFromPacket returns transfer amount and denom
 func GetFundsFromPacket(data []byte) (sdkmath.Int, string, error) {
 	var packetData transfertypes.FungibleTokenPacketData
@@ -17,9 +37,17 @@ func GetFundsFromPacket(data []byte) (sdkmath.Int, string, error) {
 		return sdkmath.Int{}, "", err
 	}
 
+	if err := ValidateRecvAddr(packetData.Receiver); err != nil {
+		return sdkmath.Int{}, "", err
+	}
+
+	if err := ValidateMemo(packetData.Memo); err != nil {
+		return sdkmath.Int{}, "", err
+	}
+
 	amount, ok := sdkmath.NewIntFromString(packetData.Amount)
 	if !ok {
-		return sdkmath.Int{}, "", sdkerrors.ErrInvalidRequest.Wrapf("invalid transfer amount %s", packetData.Amount)
+		return sdkmath.Int{}, "", ibcerrors.ErrInvalidRequest.Wrapf("invalid transfer amount %s", packetData.Amount)
 	}
 
 	return amount, GetLocalDenom(packetData.Denom), nil

util/ibc/ibc_test.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/cometbft/cometbft/crypto"
 	ibctransfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
+	"github.com/umee-network/umee/v6/tests/tsdk"
 	"gotest.tools/v3/assert"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -32,6 +33,23 @@ func TestGetFundsFromPacket(t *testing.T) {
 	assert.NilError(t, err)
 	assert.Equal(t, denom, fdenom)
 	assert.Equal(t, famount.String(), amount)
+
+	// invalid address
+	data.Receiver = tsdk.GenerateString(MaximumReceiverLength + 1)
+	_, _, err = GetFundsFromPacket(data.GetBytes())
+	assert.ErrorContains(t, err, "recipient address must not exceed")
+
+	// invalid memo
+	data.Receiver = AddressFromString("a4")
+	data.Memo = tsdk.GenerateString(MaximumMemoLength + 1)
+	_, _, err = GetFundsFromPacket(data.GetBytes())
+	assert.ErrorContains(t, err, "memo must not exceed")
+
+	// valid address and memo
+	data.Receiver = tsdk.GenerateString(MaximumReceiverLength)
+	data.Memo = tsdk.GenerateString(MaximumMemoLength)
+	_, _, err = GetFundsFromPacket(data.GetBytes())
+	assert.NilError(t, err, "Should handle valid inputs without error")
 }
 
 func TestGetLocalDenom(t *testing.T) {

x/oracle/keeper/historic_price.go

@@ -45,7 +45,7 @@ func (k Keeper) CalcAndSetHistoricMedian(
 
 	median, err := decmath.Median(historicPrices)
 	if err != nil {
-		return errors.Wrap(err, "denom: "+denom) //nolint: goconst
+		return errors.Wrap(err, "denom: "+denom)
 	}
 
 	block := uint64(ctx.BlockHeight())