chore: add defensive check to ensure metadata does not change when re…

…opening an active channel (#847) (#887)

## Description

closes: #795

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 482b7ab)

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

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