From fb7df578a1b5ee383bf3b1e8de5872bd8b6f67bd Mon Sep 17 00:00:00 2001 From: Sai Kumar <17549398+gsk967@users.noreply.github.com> Date: Tue, 18 Jun 2024 16:33:30 +0530 Subject: [PATCH] fix: ibc transfer memo and receiver length check (#2551) --- CHANGELOG.md | 6 ++- RELEASE_NOTES.md | 64 +++---------------------------- app/upgrades.go | 3 +- tests/e2e/e2e_ibc_test.go | 10 +++++ tests/e2e/setup/utils.go | 8 +++- tests/tsdk/string.go | 13 +++++++ tests/tsdk/string_test.go | 15 ++++++++ util/ibc/ibc.go | 32 +++++++++++++++- util/ibc/ibc_test.go | 18 +++++++++ x/oracle/keeper/historic_price.go | 2 +- 10 files changed, 105 insertions(+), 66 deletions(-) create mode 100644 tests/tsdk/string.go create mode 100644 tests/tsdk/string_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3f54f4e9..6c0277afa8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index a2cf61c5d9..f6ebb88ce5 100644 --- a/RELEASE_NOTES.md +++ b/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..so`. For example: - -- copy from `$GOPATH/pkg/mod/github.com/!cosm!wasm/wasmvm@v1.5.2/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 + \ No newline at end of file diff --git a/app/upgrades.go b/app/upgrades.go index ae6414cb61..51bfc96fa5 100644 --- a/app/upgrades.go +++ b/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) { diff --git a/tests/e2e/e2e_ibc_test.go b/tests/e2e/e2e_ibc_test.go index 6246dc69a9..8d519e2788 100644 --- a/tests/e2e/e2e_ibc_test.go +++ b/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 diff --git a/tests/e2e/setup/utils.go b/tests/e2e/setup/utils.go index d86c59c5b9..6332195f5d 100644 --- a/tests/e2e/setup/utils.go +++ b/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() diff --git a/tests/tsdk/string.go b/tests/tsdk/string.go new file mode 100644 index 0000000000..96a8e2980c --- /dev/null +++ b/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) +} diff --git a/tests/tsdk/string_test.go b/tests/tsdk/string_test.go new file mode 100644 index 0000000000..df114fe148 --- /dev/null +++ b/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") +} diff --git a/util/ibc/ibc.go b/util/ibc/ibc.go index 6c393ade3d..20bd2463f2 100644 --- a/util/ibc/ibc.go +++ b/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 diff --git a/util/ibc/ibc_test.go b/util/ibc/ibc_test.go index 31575de31e..4848618be0 100644 --- a/util/ibc/ibc_test.go +++ b/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) { diff --git a/x/oracle/keeper/historic_price.go b/x/oracle/keeper/historic_price.go index 0ddde0db2e..c09903dc0b 100644 --- a/x/oracle/keeper/historic_price.go +++ b/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())