Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use explicit checks in ica controller upgrade handlers (backport #5472) #5518

Merged
merged 1 commit into from
Jan 5, 2024
Merged

refactor: use explicit checks in ica controller upgrade handlers (backport #5472) #5518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 71 additions & 24 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,17 +142,42 @@ func (Keeper) OnChanCloseConfirm(
}

// OnChanUpgradeInit performs the upgrade init step of the channel upgrade handshake.
// The upgrade init callback must verify the proposed changes to the order, connectionHops, and version.
// Within the version we have the tx type, encoding, interchain account address, host/controller connectionID's
// and the ICS27 protocol version.
//
// The following may be changed:
// - tx type (must be supported)
// - encoding (must be supported)
//
// The following may not be changed:
// - order
// - connectionHops (and subsequently host/controller connectionIDs)
// - interchain account address
// - ICS27 protocol version
func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
if strings.TrimSpace(version) == "" {
return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
}

// verify order has not changed
// support for unordered ICA channels is not implemented yet
if order != channeltypes.ORDERED {
return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
}

metadata, err := icatypes.MetadataFromVersion(version)
// verify connection hops has not changed
connectionID, err := k.GetConnectionID(ctx, portID, channelID)
if err != nil {
return "", err
}

if len(connectionHops) != 1 || connectionHops[0] != connectionID {
return "", errorsmod.Wrapf(channeltypes.ErrInvalidUpgrade, "expected connection hops %s, got %s", []string{connectionID}, connectionHops)
}

// verify proposed version only modifies tx type or encoding
if strings.TrimSpace(version) == "" {
return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
}

proposedMetadata, err := icatypes.MetadataFromVersion(version)
if err != nil {
return "", err
}
Expand All @@ -162,30 +187,49 @@ func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, ord
return "", err
}

if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
return "", errorsmod.Wrap(err, "invalid metadata")
// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
// tx type and encoding are supported
if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, proposedMetadata); err != nil {
return "", errorsmod.Wrap(err, "invalid upgrade metadata")
}

// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != metadata.Address {
if currentMetadata.Address != proposedMetadata.Address {
return "", errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be changed")
}

if currentMetadata.ControllerConnectionId != connectionHops[0] {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed connection hop must not change")
if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
}

if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
}

return version, nil
}

// OnChanUpgradeAck implements the ack setup of the channel upgrade handshake.
// The upgrade ack callback must verify the proposed changes to the channel version.
// Within the channel version we have the tx type, encoding, interchain account address, host/controller connectionID's
// and the ICS27 protocol version.
//
// The following may be changed:
// - tx type (must be supported)
// - encoding (must be supported)
//
// The following may not be changed:
// - controller connectionID
// - host connectionID
// - interchain account address
// - ICS27 protocol version
func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
if strings.TrimSpace(counterpartyVersion) == "" {
return errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
}

metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
proposedMetadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
if err != nil {
return err
}
Expand All @@ -195,27 +239,30 @@ func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpart
return err
}

// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != metadata.Address {
return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
}

if currentMetadata.ControllerConnectionId != metadata.ControllerConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed controller connection ID must not change")
// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
// tx type and encoding are supported. Note, we pass in the current channel connection hops. The upgrade init
// step will verify that the proposed connection hops will not change.
if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, proposedMetadata); err != nil {
return errorsmod.Wrap(err, "invalid upgrade metadata")
}

if currentMetadata.HostConnectionId != metadata.HostConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed host connection ID must not change")
// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != proposedMetadata.Address {
return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
}

channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
}

if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil {
return err
if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
}

return nil
Expand Down
154 changes: 127 additions & 27 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
)

Expand Down Expand Up @@ -478,12 +479,26 @@ func (suite *KeeperTestSuite) TestOnChanCloseConfirm() {
}

func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
const (
invalidVersion = "invalid-version"
)

var (
path *ibctesting.Path
metadata icatypes.Metadata
version string
order channeltypes.Order
)

// updateMetadata is a helper function which modifies the metadata stored in the channel version
// and marshals it into a string to pass to OnChanUpgradeInit as the version string.
updateMetadata := func(modificationFn func(*icatypes.Metadata)) {
metadata, err := icatypes.MetadataFromVersion(path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version)
suite.Require().NoError(err)
modificationFn(&metadata)
version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
}

testCases := []struct {
name string
malleate func()
Expand All @@ -495,48 +510,104 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
nil,
},
{
name: "failure: change ICA address",
name: "failure: invalid order",
malleate: func() {
metadata.Address = TestOwnerAddress
order = channeltypes.UNORDERED
},
expError: icatypes.ErrInvalidAccountAddress,
expError: channeltypes.ErrInvalidChannelOrdering,
},
{
name: "failure: change controller connection id",
name: "failure: connectionID not found",
malleate: func() {
metadata.ControllerConnectionId = differentConnectionID
// channelID is provided via the endpoint channelID
path.EndpointA.ChannelID = "invalid channel"
},
expError: connectiontypes.ErrInvalidConnection,
expError: channeltypes.ErrChannelNotFound,
},
{
name: "failure: change host connection id",
name: "failure: invalid proposed connectionHops",
malleate: func() {
metadata.HostConnectionId = differentConnectionID
// connection hops is provided via endpoint connectionID
path.EndpointA.ConnectionID = differentConnectionID
},
expError: connectiontypes.ErrInvalidConnection,
expError: channeltypes.ErrInvalidUpgrade,
},
{
name: "failure: empty version",
malleate: func() {
version = ""
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: cannot decode version string",
malleate: func() {
channel := path.EndpointA.GetChannel()
channel.Version = "invalid-metadata-string"
path.EndpointA.SetChannel(channel)
version = invalidVersion
},
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid connection hops",
name: "failure: cannot decode self version string",
malleate: func() {
path.EndpointA.ConnectionID = differentConnectionID
ch := path.EndpointA.GetChannel()
ch.Version = invalidVersion
path.EndpointA.SetChannel(ch)
},
expError: connectiontypes.ErrConnectionNotFound,
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid order",
name: "failure: failed controller metadata validation, invalid encoding",
malleate: func() {
order = channeltypes.UNORDERED
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Encoding = "invalid-encoding"
})
},
expError: channeltypes.ErrInvalidChannelOrdering,
expError: icatypes.ErrInvalidCodec,
},
{
name: "failure: failed controller metadata validation, invalid tx type",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.TxType = "invalid-tx-type"
})
},
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: failed controller metadata validation, invalid interchain account version",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Version = "invalid-interchain-account-version"
})
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: interchain account address changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Address = TestOwnerAddress // use valid address
})
},
expError: icatypes.ErrInvalidAccountAddress,
},
{
name: "failure: controller connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.ControllerConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller connection identifier are unreachable
},
{
name: "failure: host connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.HostConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host connection identifier are unreachable
},
}

Expand All @@ -563,9 +634,12 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
// this is the actual change to the version.
metadata.Encoding = icatypes.EncodingProto3JSON

tc.malleate() // malleate mutates test data
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))

version := string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
version = path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version

tc.malleate() // malleate mutates test data

upgradeVersion, err := path.EndpointA.Chain.GetSimApp().ICAControllerKeeper.OnChanUpgradeInit(
path.EndpointA.Chain.GetContext(),
Expand Down Expand Up @@ -642,7 +716,24 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid tx type",
name: "failure: channel not found",
malleate: func() {
// channelID is provided via the endpoint channelID
path.EndpointA.ChannelID = "invalid channel"
},
expError: ibcerrors.ErrNotFound, // GetChannel error is unreachable
},
{
name: "failure: failed controller metadata validation, invalid encoding",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Encoding = "invalid-encoding"
})
},
expError: icatypes.ErrInvalidCodec,
},
{
name: "failure: failed controller metadata validation, invalid tx type",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.TxType = "invalid-tx-type"
Expand All @@ -651,10 +742,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: interchain account address has changed",
name: "failure: failed controller metadata validation, invalid interchain account version",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Version = "invalid-interchain-account-version"
})
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: interchain account address changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Address = "different-address"
metadata.Address = TestOwnerAddress // use valid address
})
},
expError: icatypes.ErrInvalidAccountAddress,
Expand All @@ -663,19 +763,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
name: "failure: controller connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.ControllerConnectionId = "different-connection-id"
metadata.ControllerConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnectionIdentifier,
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller identifier are unreachable
},
{
name: "failure: host connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.HostConnectionId = "different-host-id"
metadata.HostConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnectionIdentifier,
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host identifier are unreachable
},
}

Expand Down
Loading