imp(statemachine)!: add length validation of string fields in messages

Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Du Nguyen <dunguyen643@gmail.com> Co-authored-by: Charly <charly@interchain.io>
notional-labs · Oct 31, 2023 · ac5b2ca · ac5b2ca
1 parent 50ac22e
commit ac5b2ca
Show file tree

Hide file tree

Showing 10 changed files with 64 additions and 2 deletions.
diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs.go b/modules/apps/27-interchain-accounts/controller/types/msgs.go
@@ -12,6 +12,8 @@ import (
 	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 )
 
+const MaximumOwnerLength = 2048 // maximum length of the owner in bytes (value chosen arbitrarily)
+
 var (
 	_ sdk.Msg = (*MsgRegisterInterchainAccount)(nil)
 	_ sdk.Msg = (*MsgSendTx)(nil)
@@ -41,6 +43,10 @@ func (msg MsgRegisterInterchainAccount) ValidateBasic() error {
 		return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "owner address cannot be empty")
 	}
 
+	if len(msg.Owner) > MaximumOwnerLength {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength)
+	}
+
 	return nil
 }
 
@@ -74,6 +80,10 @@ func (msg MsgSendTx) ValidateBasic() error {
 		return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "owner address cannot be empty")
 	}
 
+	if len(msg.Owner) > MaximumOwnerLength {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength)
+	}
+
 	if err := msg.PacketData.ValidateBasic(); err != nil {
 		return errorsmod.Wrap(err, "invalid interchain account packet data")
 	}

diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go
@@ -64,6 +64,13 @@ func TestMsgRegisterInterchainAccountValidateBasic(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"owner address is too long",
+			func() {
+				msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1)
+			},
+			false,
+		},
 	}
 
 	for i, tc := range testCases {
@@ -121,6 +128,13 @@ func TestMsgSendTxValidateBasic(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"owner address is too long",
+			func() {
+				msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1)
+			},
+			false,
+		},
 		{
 			"relative timeout is not set",
 			func() {

diff --git a/modules/apps/27-interchain-accounts/types/account_test.go b/modules/apps/27-interchain-accounts/types/account_test.go
@@ -79,7 +79,7 @@ func (suite *TypesTestSuite) TestValidateAccountAddress() {
 		},
 		{
 			"address is too long",
-			ibctesting.LongString,
+			ibctesting.GenerateString(uint(types.DefaultMaxAddrLength) + 1),
 			false,
 		},
 	}

diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go
@@ -12,6 +12,8 @@ import (
 	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 )
 
+const MaximumCounterpartyPayeeLength = 2048 // maximum length of the counterparty payee in bytes (value chosen arbitrarily)
+
 var (
 	_ sdk.Msg = (*MsgRegisterPayee)(nil)
 	_ sdk.Msg = (*MsgRegisterCounterpartyPayee)(nil)
@@ -100,6 +102,10 @@ func (msg MsgRegisterCounterpartyPayee) ValidateBasic() error {
 		return ErrCounterpartyPayeeEmpty
 	}
 
+	if len(msg.CounterpartyPayee) > MaximumCounterpartyPayeeLength {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "counterparty payee address must not exceed %d bytes", MaximumCounterpartyPayeeLength)
+	}
+
 	return nil
 }
 

diff --git a/modules/apps/29-fee/types/msgs_test.go b/modules/apps/29-fee/types/msgs_test.go
@@ -139,6 +139,13 @@ func TestMsgRegisterCountepartyPayeeValidation(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"invalid counterparty payee address: too long",
+			func() {
+				msg.CounterpartyPayee = ibctesting.GenerateString(types.MaximumCounterpartyPayeeLength + 1)
+			},
+			false,
+		},
 	}
 
 	for i, tc := range testCases {

diff --git a/modules/apps/transfer/types/errors.go b/modules/apps/transfer/types/errors.go
@@ -15,4 +15,5 @@ var (
 	ErrReceiveDisabled         = errorsmod.Register(ModuleName, 8, "fungible token transfers to this chain are disabled")
 	ErrMaxTransferChannels     = errorsmod.Register(ModuleName, 9, "max transfer channels")
 	ErrInvalidAuthorization    = errorsmod.Register(ModuleName, 10, "invalid transfer authorization")
+	ErrInvalidMemo             = errorsmod.Register(ModuleName, 11, "invalid memo")
 )
diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go
@@ -12,6 +12,11 @@ import (
 	ibcerrors "github.com/cosmos/ibc-go/v8/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)
+)
+
 var (
 	_ sdk.Msg              = (*MsgUpdateParams)(nil)
 	_ sdk.Msg              = (*MsgTransfer)(nil)
@@ -91,6 +96,12 @@ func (msg MsgTransfer) ValidateBasic() error {
 	if strings.TrimSpace(msg.Receiver) == "" {
 		return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "missing recipient address")
 	}
+	if len(msg.Receiver) > MaximumReceiverLength {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "recipient address must not exceed %d bytes", MaximumReceiverLength)
+	}
+	if len(msg.Memo) > MaximumMemoLength {
+		return errorsmod.Wrapf(ErrInvalidMemo, "memo must not exceed %d bytes", MaximumMemoLength)
+	}
 	return ValidateIBCDenom(msg.Token.Denom)
 }
 

diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go
@@ -60,11 +60,13 @@ func TestMsgTransferValidation(t *testing.T) {
 		{"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ""), false},
 		{"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coin, sender, receiver, timeoutHeight, 0, ""), false},
 		{"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coin, sender, receiver, timeoutHeight, 0, ""), false},
+		{"too long memo", types.NewMsgTransfer(validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), false},
 		{"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coin, sender, receiver, timeoutHeight, 0, ""), false},
 		{"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoin, sender, receiver, timeoutHeight, 0, ""), false},
 		{"zero coin", types.NewMsgTransfer(validPort, validChannel, zeroCoin, sender, receiver, timeoutHeight, 0, ""), false},
 		{"missing sender address", types.NewMsgTransfer(validPort, validChannel, coin, emptyAddr, receiver, timeoutHeight, 0, ""), false},
 		{"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, "", timeoutHeight, 0, ""), false},
+		{"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), false},
 		{"empty coin", types.NewMsgTransfer(validPort, validChannel, sdk.Coin{}, sender, receiver, timeoutHeight, 0, ""), false},
 	}
 

diff --git a/testing/utils.go b/testing/utils.go
@@ -1,6 +1,7 @@
 package ibctesting
 
 import (
+	"math/rand"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -23,3 +24,12 @@ func ApplyValSetChanges(tb testing.TB, valSet *tmtypes.ValidatorSet, valUpdates
 
 	return newVals
 }
+
+// GenerateString generates a random string of the given length in bytes
+func GenerateString(length uint) string {
+	bytes := make([]byte, length)
+	for i := range bytes {
+		bytes[i] = charset[rand.Intn(len(charset))]
+	}
+	return string(bytes)
+}
diff --git a/testing/values.go b/testing/values.go
@@ -43,7 +43,8 @@ const (
 	Title       = "title"
 	Description = "description"
 
-	LongString = "LoremipsumdolorsitameconsecteturadipiscingeliseddoeiusmodtemporincididuntutlaboreetdoloremagnaaliquUtenimadminimveniamquisnostrudexercitationullamcolaborisnisiutaliquipexeacommodoconsequDuisauteiruredolorinreprehenderitinvoluptateelitsseillumoloreufugiatnullaariaturEcepteurintoccaectupidatatonroidentuntnulpauifficiaeseruntmollitanimidestlaborum"
+	// character set used for generating a random string in GenerateString
+	charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
 )
 
 var (