Merge branch 'main' into carlos/6095-allow-ics20-authorisation-for-a-…

…specific-memo-string

CHANGELOG.md

@@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able.
+* (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages.
 * (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other.
 * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization.
 

modules/apps/27-interchain-accounts/controller/keeper/migrations.go

@@ -29,24 +29,23 @@ func NewMigrator(k *Keeper) Migrator {
 // are owned by the controller submodule and ibc.
 func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error {
 	if m.keeper != nil {
-		logger := m.keeper.Logger(ctx)
 		filteredChannels := m.keeper.channelKeeper.GetAllChannelsWithPortPrefix(ctx, icatypes.ControllerPortPrefix)
 		for _, ch := range filteredChannels {
 			name := host.ChannelCapabilityPath(ch.PortId, ch.ChannelId)
 			capability, found := m.keeper.scopedKeeper.GetCapability(ctx, name)
 			if !found {
-				logger.Error(fmt.Sprintf("failed to find capability: %s", name))
+				m.keeper.Logger(ctx).Error(fmt.Sprintf("failed to find capability: %s", name))
 				return errorsmod.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", name)
 			}
 
 			isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, capability, name)
 			if !isAuthenticated {
-				logger.Error(fmt.Sprintf("expected capability owner: %s", controllertypes.SubModuleName))
+				m.keeper.Logger(ctx).Error(fmt.Sprintf("expected capability owner: %s", controllertypes.SubModuleName))
 				return errorsmod.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", controllertypes.SubModuleName)
 			}
 
 			m.keeper.SetMiddlewareEnabled(ctx, ch.PortId, ch.ConnectionHops[0])
-			logger.Info("successfully migrated channel capability", "name", name)
+			m.keeper.Logger(ctx).Info("successfully migrated channel capability", "name", name)
 		}
 	}
 	return nil

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -116,9 +116,8 @@ func (im IBCModule) OnRecvPacket(
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	logger := im.keeper.Logger(ctx)
 	if !im.keeper.GetParams(ctx).HostEnabled {
-		logger.Info("host submodule is disabled")
+		im.keeper.Logger(ctx).Info("host submodule is disabled")
 		keeper.EmitHostDisabledEvent(ctx, packet)
 		return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)
 	}
@@ -127,9 +126,9 @@ func (im IBCModule) OnRecvPacket(
 	ack := channeltypes.NewResultAcknowledgement(txResponse)
 	if err != nil {
 		ack = channeltypes.NewErrorAcknowledgement(err)
-		logger.Error(fmt.Sprintf("%s sequence %d", err.Error(), packet.Sequence))
+		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", err.Error(), packet.Sequence))
 	} else {
-		logger.Info("successfully handled packet", "sequence", packet.Sequence)
+		im.keeper.Logger(ctx).Info("successfully handled packet", "sequence", packet.Sequence)
 	}
 
 	// Emit an event indicating a successful or failed acknowledgement.

modules/apps/27-interchain-accounts/host/keeper/handshake.go

@@ -30,7 +30,6 @@ func (k Keeper) OnChanOpenTry(
 	counterparty channeltypes.Counterparty,
 	counterpartyVersion string,
 ) (string, error) {
-	logger := k.Logger(ctx)
 	if portID != icatypes.HostPortID {
 		return "", errorsmod.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.HostPortID, portID)
 	}
@@ -43,7 +42,7 @@ func (k Keeper) OnChanOpenTry(
 			return "", errorsmod.Wrapf(err, "failed to retrieve connection %s", connectionHops[0])
 		}
 
-		logger.Debug("counterparty version is invalid, proposing default metadata")
+		k.Logger(ctx).Debug("counterparty version is invalid, proposing default metadata")
 		metadata = icatypes.NewDefaultMetadata(connection.Counterparty.ConnectionId, connectionHops[0])
 	}
 

modules/apps/27-interchain-accounts/host/types/params.go

@@ -9,6 +9,8 @@ import (
 const (
 	// DefaultHostEnabled is the default value for the host param (set to true)
 	DefaultHostEnabled = true
+	// Maximum length of the allowlist
+	MaxAllowListLength = 500
 )
 
 // NewParams creates a new parameter configuration for the host submodule
@@ -30,6 +32,10 @@ func (p Params) Validate() error {
 }
 
 func validateAllowlist(allowMsgs []string) error {
+	if len(allowMsgs) > MaxAllowListLength {
+		return fmt.Errorf("allow list length must not exceed %d items", MaxAllowListLength)
+	}
+
 	if slices.Contains(allowMsgs, AllowAllHostMsgs) && len(allowMsgs) > 1 {
 		return fmt.Errorf("allow list must have only one element because the allow all host messages wildcard (%s) is present", AllowAllHostMsgs)
 	}

modules/apps/27-interchain-accounts/host/types/params_test.go

@@ -14,4 +14,5 @@ func TestValidateParams(t *testing.T) {
 	require.Error(t, types.NewParams(true, []string{""}).Validate())
 	require.Error(t, types.NewParams(true, []string{" "}).Validate())
 	require.Error(t, types.NewParams(true, []string{"*", "/cosmos.bank.v1beta1.MsgSend"}).Validate())
+	require.Error(t, types.NewParams(true, make([]string, types.MaxAllowListLength+1)).Validate())
 }

modules/apps/transfer/ibc_module.go

@@ -180,14 +180,13 @@ func (im IBCModule) OnRecvPacket(
 	packet channeltypes.Packet,
 	relayer sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	logger := im.keeper.Logger(ctx)
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
 	var data types.FungibleTokenPacketData
 	var ackErr error
 	if err := json.Unmarshal(packet.GetData(), &data); err != nil {
 		ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data")
-		logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
+		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
 		ack = channeltypes.NewErrorAcknowledgement(ackErr)
 	}
 
@@ -198,9 +197,9 @@ func (im IBCModule) OnRecvPacket(
 		if err != nil {
 			ack = channeltypes.NewErrorAcknowledgement(err)
 			ackErr = err
-			logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
+			im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
 		} else {
-			logger.Info("successfully handled ICS-20 packet", "sequence", packet.Sequence)
+			im.keeper.Logger(ctx).Info("successfully handled ICS-20 packet", "sequence", packet.Sequence)
 		}
 	}
 

modules/core/02-client/types/params.go

@@ -6,6 +6,9 @@ import (
 	"strings"
 )
 
+// Maximum length of the allowed clients list
+const MaxAllowedClientsLength = 200
+
 // DefaultAllowedClients are the default clients for the AllowedClients parameter.
 // By default it allows all client types.
 var DefaultAllowedClients = []string{AllowAllClients}
@@ -46,6 +49,10 @@ func (p Params) IsAllowedClient(clientType string) bool {
 // validateClients checks that the given clients are not blank and there are no duplicates.
 // If AllowAllClients wildcard (*) is used, then there should no other client types in the allow list
 func validateClients(clients []string) error {
+	if len(clients) > MaxAllowedClientsLength {
+		return fmt.Errorf("allowed clients length must not exceed %d items", MaxAllowedClientsLength)
+	}
+
 	if slices.Contains(clients, AllowAllClients) && len(clients) > 1 {
 		return fmt.Errorf("allow list must have only one element because the allow all clients wildcard (%s) is present", AllowAllClients)
 	}

modules/core/02-client/types/params_test.go

@@ -40,6 +40,7 @@ func TestValidateParams(t *testing.T) {
 		{"blank client", NewParams(" "), false},
 		{"duplicate clients", NewParams(exported.Tendermint, exported.Tendermint), false},
 		{"allow all clients plus valid client", NewParams(AllowAllClients, exported.Tendermint), false},
+		{"too many allowed clients", NewParams(make([]string, MaxAllowedClientsLength+1)...), false},
 	}
 
 	for _, tc := range testCases {

modules/core/03-connection/types/msgs.go

@@ -129,6 +129,9 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error {
 	if len(msg.CounterpartyVersions) == 0 {
 		return errorsmod.Wrap(ibcerrors.ErrInvalidVersion, "empty counterparty versions")
 	}
+	if len(msg.CounterpartyVersions) > MaxCounterpartyVersionsLength {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "counterparty versions must not exceed %d items", MaxCounterpartyVersionsLength)
+	}
 	for i, version := range msg.CounterpartyVersions {
 		if err := ValidateVersion(version); err != nil {
 			return errorsmod.Wrapf(err, "basic validation failed on version with index %d", i)

modules/core/03-connection/types/msgs_test.go

@@ -159,8 +159,10 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() {
 		{"empty proofConsensus", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, signer), false},
 		{"invalid consensusHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), signer), false},
 		{"empty singer", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ""), false},
-		{"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true},
 		{"invalid version", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{{}}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
+		{"too many counterparty versions", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, make([]*types.Version, types.MaxCounterpartyVersionsLength+1), 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
+		{"too many features in counterparty version", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{{"v1", make([]string, types.MaxFeaturesLength+1)}}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
+		{"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true},
 	}
 
 	for _, tc := range testCases {

modules/core/03-connection/types/version.go

@@ -27,6 +27,11 @@ var (
 	allowNilFeatureSet = map[string]bool{
 		DefaultIBCVersionIdentifier: false,
 	}
+
+	// MaxVersionsLength is the maximum number of versions that can be supported
+	MaxCounterpartyVersionsLength = 100
+	// MaxFeaturesLength is the maximum number of features that can be supported
+	MaxFeaturesLength = 100
 )
 
 // NewVersion returns a new instance of Version.
@@ -56,6 +61,9 @@ func ValidateVersion(version *Version) error {
 	if strings.TrimSpace(version.Identifier) == "" {
 		return errorsmod.Wrap(ErrInvalidVersion, "version identifier cannot be blank")
 	}
+	if len(version.Features) > MaxFeaturesLength {
+		return errorsmod.Wrapf(ErrInvalidVersion, "features length must not exceed %d items", MaxFeaturesLength)
+	}
 	for i, feature := range version.Features {
 		if strings.TrimSpace(feature) == "" {
 			return errorsmod.Wrapf(ErrInvalidVersion, "feature cannot be blank, index %d", i)

modules/light-clients/07-tendermint/migrations/migrations.go

@@ -40,8 +40,7 @@ func PruneExpiredConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, clientK
 		totalPruned += ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState)
 	}
 
-	clientLogger := clientKeeper.Logger(ctx)
-	clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned)
+	clientKeeper.Logger(ctx).Info("pruned expired tendermint consensus states", "total", totalPruned)
 
 	return totalPruned, nil
 }