chore: add defensive check to ensure metadata does not change when reopening an active channel (backport #847)

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

@@ -1,6 +1,7 @@
 package keeper
 
 import (
+	"fmt"
 	"strings"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -49,9 +50,20 @@ func (k Keeper) OnChanOpenInit(
 		return err
 	}
 
-	activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], portID)
+	activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID)
 	if found {
-		return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
+		channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
+		if !found {
+			panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
+		}
+
+		if channel.State == channeltypes.OPEN {
+			return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
+		}
+
+		if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
+			return sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
+		}
 	}
 
 	return nil

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

@@ -30,6 +30,51 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 			},
 			true,
 		},
+		{
+			"success - previous active channel closed",
+			func() {
+				suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				channel := channeltypes.Channel{
+					State:          channeltypes.CLOSED,
+					Ordering:       channeltypes.ORDERED,
+					Counterparty:   counterparty,
+					ConnectionHops: []string{path.EndpointA.ConnectionID},
+					Version:        TestVersion,
+				}
+
+				path.EndpointA.SetChannel(channel)
+			},
+			true,
+		},
+		{
+			"invalid metadata -  previous metadata is different",
+			func() {
+				// set active channel to closed
+				suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+				closedChannel := channeltypes.Channel{
+					State:          channeltypes.CLOSED,
+					Ordering:       channeltypes.ORDERED,
+					Counterparty:   counterparty,
+					ConnectionHops: []string{path.EndpointA.ConnectionID},
+					Version:        TestVersion,
+				}
+
+				path.EndpointA.SetChannel(closedChannel)
+
+				// modify metadata
+				metadata.Version = "ics27-2"
+
+				versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+
+				channel.Version = string(versionBytes)
+			},
+			false,
+		},
 		{
 			"invalid order - UNORDERED",
 			func() {

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

@@ -1,6 +1,7 @@
 package keeper
 
 import (
+	"fmt"
 	"strings"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -47,8 +48,20 @@ func (k Keeper) OnChanOpenTry(
 		return "", err
 	}
 
-	if activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], counterparty.PortId); found {
-		return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
+	activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], counterparty.PortId)
+	if found {
+		channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
+		if !found {
+			panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
+		}
+
+		if channel.State == channeltypes.OPEN {
+			return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
+		}
+
+		if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
+			return "", sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
+		}
 	}
 
 	// On the host chain the capability may only be claimed during the OnChanOpenTry
@@ -83,9 +96,9 @@ func (k Keeper) OnChanOpenConfirm(
 	}
 
 	// It is assumed the controller chain will not allow multiple active channels to be created for the same connectionID/portID
-	// If the controller chain does allow multiple active channels to be created for the same connectionID/portID, 
+	// If the controller chain does allow multiple active channels to be created for the same connectionID/portID,
 	// disallowing overwriting the current active channel guarantees the channel can no longer be used as the controller
-	// and host will disagree on what the currently active channel is 
+	// and host will disagree on what the currently active channel is
 	k.SetActiveChannelID(ctx, channel.ConnectionHops[0], channel.Counterparty.PortId, channelID)
 
 	return nil

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

@@ -30,6 +30,36 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 			},
 			true,
 		},
+		{
+			"success - reopening closed active channel",
+			func() {
+				// create a new channel and set it in state
+				ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion)
+				suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)
+
+				// set the active channelID in state
+				suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)
+			}, true,
+		},
+		{
+			"invalid metadata - previous metadata is different",
+			func() {
+				// create a new channel and set it in state
+				ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion)
+				suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)
+
+				// set the active channelID in state
+				suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)
+
+				// modify metadata
+				metadata.Version = "ics27-2"
+
+				versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+
+				path.EndpointA.ChannelConfig.Version = string(versionBytes)
+			}, false,
+		},
 		{
 			"invalid order - UNORDERED",
 			func() {
@@ -140,7 +170,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 			func() {
 				// create a new channel and set it in state
 				ch := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, ibctesting.DefaultChannelVersion)
-				suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)
+				suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)
 
 				// set the active channelID in state
 				suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)

modules/apps/27-interchain-accounts/types/metadata.go

@@ -27,6 +27,21 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress,
 	}
 }
 
+// IsPreviousMetadataEqual compares a metadata to a previous version string set in a channel struct.
+// It ensures all fields are equal except the Address string
+func IsPreviousMetadataEqual(previousVersion string, metadata Metadata) bool {
+	var previousMetadata Metadata
+	if err := ModuleCdc.UnmarshalJSON([]byte(previousVersion), &previousMetadata); err != nil {
+		return false
+	}
+
+	return (previousMetadata.Version == metadata.Version &&
+		previousMetadata.ControllerConnectionId == metadata.ControllerConnectionId &&
+		previousMetadata.HostConnectionId == metadata.HostConnectionId &&
+		previousMetadata.Encoding == metadata.Encoding &&
+		previousMetadata.TxType == metadata.TxType)
+}
+
 // ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters
 func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error {
 	if !isSupportedEncoding(metadata.Encoding) {

modules/apps/27-interchain-accounts/types/metadata_test.go

@@ -5,6 +5,127 @@ import (
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 )
 
+// use TestVersion as metadata being compared against
+func (suite *TypesTestSuite) TestIsPreviousMetadataEqual() {
+
+	var (
+		metadata        types.Metadata
+		previousVersion string
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expEqual bool
+	}{
+		{
+			"success",
+			func() {
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			true,
+		},
+		{
+			"success with empty account address",
+			func() {
+				metadata.Address = ""
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			true,
+		},
+		{
+			"cannot decode previous version",
+			func() {
+				previousVersion = "invalid previous version"
+			},
+			false,
+		},
+		{
+			"unequal encoding format",
+			func() {
+				metadata.Encoding = "invalid-encoding-format"
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			false,
+		},
+		{
+			"unequal transaction type",
+			func() {
+				metadata.TxType = "invalid-tx-type"
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			false,
+		},
+		{
+			"unequal controller connection",
+			func() {
+				metadata.ControllerConnectionId = "connection-10"
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			false,
+		},
+		{
+			"unequal host connection",
+			func() {
+				metadata.HostConnectionId = "connection-10"
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			false,
+		},
+		{
+			"unequal version",
+			func() {
+				metadata.Version = "invalid version"
+
+				versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
+				suite.Require().NoError(err)
+				previousVersion = string(versionBytes)
+			},
+			false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+
+			path := ibctesting.NewPath(suite.chainA, suite.chainB)
+			suite.coordinator.SetupConnections(path)
+
+			expectedMetadata := types.NewMetadata(types.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestOwnerAddress, types.EncodingProtobuf, types.TxTypeSDKMultiMsg)
+			metadata = expectedMetadata // default success case
+
+			tc.malleate() // malleate mutates test data
+
+			equal := types.IsPreviousMetadataEqual(previousVersion, expectedMetadata)
+
+			if tc.expEqual {
+				suite.Require().True(equal)
+			} else {
+				suite.Require().False(equal)
+			}
+		})
+	}
+}
+
 func (suite *TypesTestSuite) TestValidateControllerMetadata() {
 
 	var metadata types.Metadata