From 1983c70346140af760aa36cccb2c9e25d4473f6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 15 Nov 2021 13:15:17 +0100 Subject: [PATCH 1/7] add tests for testing wrong handshake flow Adds tests for each handshake test attempting to initialize the handshake using the wrong flow. Adds an additional portID check to OnChanOpenAck. --- .../interchain_accounts_test.go | 120 ++++++++++++++++++ .../keeper/handshake.go | 4 + .../keeper/handshake_test.go | 6 + 3 files changed, 130 insertions(+) create mode 100644 modules/apps/27-interchain-accounts/interchain_accounts_test.go diff --git a/modules/apps/27-interchain-accounts/interchain_accounts_test.go b/modules/apps/27-interchain-accounts/interchain_accounts_test.go new file mode 100644 index 00000000000..b207caacdfa --- /dev/null +++ b/modules/apps/27-interchain-accounts/interchain_accounts_test.go @@ -0,0 +1,120 @@ +package interchain_accounts_test + +import ( + "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" + channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v2/modules/core/24-host" + // ibctesting "github.com/cosmos/ibc-go/v2/testing" +) + +// Test initiating a ChanOpenInit using the host chain instead of the controller chain +// ChainA is the controller chain. ChainB is the host chain +func (suite *InterchainAccountsTestSuite) TestChanOpenInitWrongFlow() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + // use chainB (host) for ChanOpenInit + msg := channeltypes.NewMsgChannelOpenInit(path.EndpointB.ChannelConfig.PortID, types.VersionPrefix, channeltypes.ORDERED, []string{path.EndpointB.ConnectionID}, path.EndpointA.ChannelConfig.PortID, types.ModuleName) + handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) + _, err := handler(suite.chainB.GetContext(), msg) + + suite.Require().Error(err) +} + +// Test initiating a ChanOpenTry using the controller chain instead of the host chain +// ChainA is the controller chain. ChainB creates a controller port as well, +// attempting to trick chainA. +func (suite *InterchainAccountsTestSuite) TestChanOpenTryWrongFlow() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + // chainB also creates a controller port + err = InitInterchainAccount(path.EndpointB, TestOwnerAddress) + suite.Require().NoError(err) + + path.EndpointA.UpdateClient() + channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + proofInit, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) + + // use chainA (controller) for ChanOpenTry + msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, types.VersionPrefix, proofInit, proofHeight, types.ModuleName) + handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainA.GetContext(), msg) + + suite.Require().Error(err) +} + +// Test initiating a ChanOpenAck using the host chain instead of the controller chain +// ChainA is the controller chain. ChainB is the host chain +func (suite *InterchainAccountsTestSuite) TestChanOpenAckWrongFlow() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + // chainA maliciously sets channel to TRYOPEN + channel := channeltypes.NewChannel(channeltypes.TRYOPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion) + suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel) + + // commit state changes so proof can be created + suite.chainA.App.Commit() + suite.chainA.NextBlock() + + path.EndpointB.UpdateClient() + + // query proof from ChainA + channelKey := host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + proofTry, proofHeight := path.EndpointA.Chain.QueryProof(channelKey) + + // use chainB (host) for ChanOpenAck + msg := channeltypes.NewMsgChannelOpenAck(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelID, TestVersion, proofTry, proofHeight, types.ModuleName) + handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainB.GetContext(), msg) + + suite.Require().Error(err) +} + +// Test initiating a ChanOpenConfirm using the controller chain instead of the host chain +// ChainA is the controller chain. ChainB is the host chain +func (suite *InterchainAccountsTestSuite) TestChanOpenConfirmWrongFlow() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + // chainB maliciously sets channel to OPEN + channel := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointB.ConnectionID}, TestVersion) + suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, channel) + + // commit state changes so proof can be created + suite.chainB.App.Commit() + suite.chainB.NextBlock() + + path.EndpointA.UpdateClient() + + // query proof from ChainB + channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + proofAck, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) + + // use chainA (controller) for ChanOpenConfirm + msg := channeltypes.NewMsgChannelOpenConfirm(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, proofAck, proofHeight, types.ModuleName) + handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainA.GetContext(), msg) + + suite.Require().Error(err) +} diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index a6c69509013..b1806ca475d 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -143,6 +143,10 @@ func (k Keeper) OnChanOpenAck( channelID string, counterpartyVersion string, ) error { + if portID == types.PortID { + return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "portID cannot be host chain port ID: %s", types.PortID) + } + if err := types.ValidateVersion(counterpartyVersion); err != nil { return sdkerrors.Wrap(err, "counterparty version validation failed") } diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index cf27023831c..f9dd13f31ea 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -334,6 +334,12 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() { counterpartyVersion = "version" }, false, }, + { + "invalid portID", func() { + path.EndpointA.ChannelConfig.PortID = types.PortID + expectedChannelID = "" + }, false, + }, } for _, tc := range testCases { From 20938d57b6cab6db2bfdb18170edd9a01b39dd28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 15 Nov 2021 13:19:31 +0100 Subject: [PATCH 2/7] remove unnecessary comment --- modules/apps/27-interchain-accounts/interchain_accounts_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/interchain_accounts_test.go b/modules/apps/27-interchain-accounts/interchain_accounts_test.go index b207caacdfa..144a2fd66db 100644 --- a/modules/apps/27-interchain-accounts/interchain_accounts_test.go +++ b/modules/apps/27-interchain-accounts/interchain_accounts_test.go @@ -4,7 +4,6 @@ import ( "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v2/modules/core/24-host" - // ibctesting "github.com/cosmos/ibc-go/v2/testing" ) // Test initiating a ChanOpenInit using the host chain instead of the controller chain From d3fb62a0c00b921d636749b62824e2e758763ea8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 19 Nov 2021 12:16:50 +0100 Subject: [PATCH 3/7] readjust tests based on new layout --- .../controller/ibc_module_test.go | 99 +++++++++++++++ .../controller/keeper/handshake.go | 4 + .../controller/keeper/handshake_test.go | 6 + .../host/ibc_module_test.go | 50 ++++++++ .../interchain_accounts_test.go | 119 ------------------ 5 files changed, 159 insertions(+), 119 deletions(-) delete mode 100644 modules/apps/27-interchain-accounts/interchain_accounts_test.go diff --git a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go index 7cec17d38da..b3ccbd4dd24 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go @@ -189,6 +189,54 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { } } +// Test initiating a ChanOpenTry using the controller chain instead of the host chain +// ChainA is the controller chain. ChainB creates a controller port as well, +// attempting to trick chainA. +// Sending a MsgChanOpenTry will never reach the application callback due to +// core IBC checks not passing, so a call to the application callback is also +// done directly. +func (suite *InterchainAccountsTestSuite) TestChanOpenTry() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + // chainB also creates a controller port + err = InitInterchainAccount(path.EndpointB, TestOwnerAddress) + suite.Require().NoError(err) + + path.EndpointA.UpdateClient() + channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + proofInit, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) + + // use chainA (controller) for ChanOpenTry + msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, types.VersionPrefix, proofInit, proofHeight, types.ModuleName) + handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainA.GetContext(), msg) + + suite.Require().Error(err) + + // call application callback directly + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + chanCap, found := suite.chainA.App.GetScopedIBCKeeper().GetCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + suite.Require().True(found) + + err = cbs.OnChanOpenTry( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.Order, []string{path.EndpointA.ConnectionID}, + path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, + counterparty, path.EndpointA.ChannelConfig.Version, path.EndpointB.ChannelConfig.Version, + ) + suite.Require().Error(err) +} + func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { var ( path *ibctesting.Path @@ -253,3 +301,54 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { } } + +// Test initiating a ChanOpenConfirm using the controller chain instead of the host chain +// ChainA is the controller chain. ChainB is the host chain +// Sending a MsgChanOpenConfirm will never reach the application callback due to +// core IBC checks not passing, so a call to the application callback is also +// done directly. +func (suite *InterchainAccountsTestSuite) TestChanOpenConfirm() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + // chainB maliciously sets channel to OPEN + channel := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointB.ConnectionID}, TestVersion) + suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, channel) + + // commit state changes so proof can be created + suite.chainB.App.Commit() + suite.chainB.NextBlock() + + path.EndpointA.UpdateClient() + + // query proof from ChainB + channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + proofAck, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) + + // use chainA (controller) for ChanOpenConfirm + msg := channeltypes.NewMsgChannelOpenConfirm(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, proofAck, proofHeight, types.ModuleName) + handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainA.GetContext(), msg) + + suite.Require().Error(err) + + // call application callback directly + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanOpenConfirm( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + ) + suite.Require().Error(err) + +} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go index 6145c867109..f88f8f1fadc 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go @@ -70,6 +70,10 @@ func (k Keeper) OnChanOpenAck( channelID string, counterpartyVersion string, ) error { + if portID == types.PortID { + return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "portID cannot be host chain port ID: %s", types.PortID) + } + if err := types.ValidateVersion(counterpartyVersion); err != nil { return sdkerrors.Wrap(err, "counterparty version validation failed") } diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go index a4bd9622eab..f0e634fd362 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go @@ -174,6 +174,12 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() { }, false, }, + { + "invalid portID", func() { + path.EndpointA.ChannelConfig.PortID = types.PortID + expectedChannelID = "" + }, false, + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 92203c25ea8..cc19b609378 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -106,6 +106,21 @@ func SetupICAPath(path *ibctesting.Path, owner string) error { return nil } +// Test initiating a ChanOpenInit using the host chain instead of the controller chain +// ChainA is the controller chain. ChainB is the host chain +func (suite *InterchainAccountsTestSuite) TestChanOpenInit() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + // use chainB (host) for ChanOpenInit + msg := channeltypes.NewMsgChannelOpenInit(path.EndpointB.ChannelConfig.PortID, types.VersionPrefix, channeltypes.ORDERED, []string{path.EndpointB.ConnectionID}, path.EndpointA.ChannelConfig.PortID, types.ModuleName) + handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) + _, err := handler(suite.chainB.GetContext(), msg) + + suite.Require().Error(err) +} + func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { var ( path *ibctesting.Path @@ -192,6 +207,41 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() { } +// Test initiating a ChanOpenAck using the host chain instead of the controller chain +// ChainA is the controller chain. ChainB is the host chain +func (suite *InterchainAccountsTestSuite) TestChanOpenAck() { + suite.SetupTest() // reset + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + err = path.EndpointB.ChanOpenTry() + suite.Require().NoError(err) + + // chainA maliciously sets channel to TRYOPEN + channel := channeltypes.NewChannel(channeltypes.TRYOPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion) + suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel) + + // commit state changes so proof can be created + suite.chainA.App.Commit() + suite.chainA.NextBlock() + + path.EndpointB.UpdateClient() + + // query proof from ChainA + channelKey := host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + proofTry, proofHeight := path.EndpointA.Chain.QueryProof(channelKey) + + // use chainB (host) for ChanOpenAck + msg := channeltypes.NewMsgChannelOpenAck(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelID, TestVersion, proofTry, proofHeight, types.ModuleName) + handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) + _, err = handler(suite.chainB.GetContext(), msg) + + suite.Require().Error(err) +} + func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { testCases := []struct { name string diff --git a/modules/apps/27-interchain-accounts/interchain_accounts_test.go b/modules/apps/27-interchain-accounts/interchain_accounts_test.go deleted file mode 100644 index 144a2fd66db..00000000000 --- a/modules/apps/27-interchain-accounts/interchain_accounts_test.go +++ /dev/null @@ -1,119 +0,0 @@ -package interchain_accounts_test - -import ( - "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" - channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" - host "github.com/cosmos/ibc-go/v2/modules/core/24-host" -) - -// Test initiating a ChanOpenInit using the host chain instead of the controller chain -// ChainA is the controller chain. ChainB is the host chain -func (suite *InterchainAccountsTestSuite) TestChanOpenInitWrongFlow() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - // use chainB (host) for ChanOpenInit - msg := channeltypes.NewMsgChannelOpenInit(path.EndpointB.ChannelConfig.PortID, types.VersionPrefix, channeltypes.ORDERED, []string{path.EndpointB.ConnectionID}, path.EndpointA.ChannelConfig.PortID, types.ModuleName) - handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) - _, err := handler(suite.chainB.GetContext(), msg) - - suite.Require().Error(err) -} - -// Test initiating a ChanOpenTry using the controller chain instead of the host chain -// ChainA is the controller chain. ChainB creates a controller port as well, -// attempting to trick chainA. -func (suite *InterchainAccountsTestSuite) TestChanOpenTryWrongFlow() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) - - // chainB also creates a controller port - err = InitInterchainAccount(path.EndpointB, TestOwnerAddress) - suite.Require().NoError(err) - - path.EndpointA.UpdateClient() - channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - proofInit, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) - - // use chainA (controller) for ChanOpenTry - msg := channeltypes.NewMsgChannelOpenTry(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, TestVersion, channeltypes.ORDERED, []string{path.EndpointA.ConnectionID}, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, types.VersionPrefix, proofInit, proofHeight, types.ModuleName) - handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) - _, err = handler(suite.chainA.GetContext(), msg) - - suite.Require().Error(err) -} - -// Test initiating a ChanOpenAck using the host chain instead of the controller chain -// ChainA is the controller chain. ChainB is the host chain -func (suite *InterchainAccountsTestSuite) TestChanOpenAckWrongFlow() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) - - err = path.EndpointB.ChanOpenTry() - suite.Require().NoError(err) - - // chainA maliciously sets channel to TRYOPEN - channel := channeltypes.NewChannel(channeltypes.TRYOPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion) - suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, channel) - - // commit state changes so proof can be created - suite.chainA.App.Commit() - suite.chainA.NextBlock() - - path.EndpointB.UpdateClient() - - // query proof from ChainA - channelKey := host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - proofTry, proofHeight := path.EndpointA.Chain.QueryProof(channelKey) - - // use chainB (host) for ChanOpenAck - msg := channeltypes.NewMsgChannelOpenAck(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelID, TestVersion, proofTry, proofHeight, types.ModuleName) - handler := suite.chainB.GetSimApp().MsgServiceRouter().Handler(msg) - _, err = handler(suite.chainB.GetContext(), msg) - - suite.Require().Error(err) -} - -// Test initiating a ChanOpenConfirm using the controller chain instead of the host chain -// ChainA is the controller chain. ChainB is the host chain -func (suite *InterchainAccountsTestSuite) TestChanOpenConfirmWrongFlow() { - suite.SetupTest() // reset - path := NewICAPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) - - err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) - suite.Require().NoError(err) - - err = path.EndpointB.ChanOpenTry() - suite.Require().NoError(err) - - // chainB maliciously sets channel to OPEN - channel := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointB.ConnectionID}, TestVersion) - suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, channel) - - // commit state changes so proof can be created - suite.chainB.App.Commit() - suite.chainB.NextBlock() - - path.EndpointA.UpdateClient() - - // query proof from ChainB - channelKey := host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - proofAck, proofHeight := path.EndpointB.Chain.QueryProof(channelKey) - - // use chainA (controller) for ChanOpenConfirm - msg := channeltypes.NewMsgChannelOpenConfirm(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, proofAck, proofHeight, types.ModuleName) - handler := suite.chainA.GetSimApp().MsgServiceRouter().Handler(msg) - _, err = handler(suite.chainA.GetContext(), msg) - - suite.Require().Error(err) -} From 3250f37c611ea4f224c733d735558b73bc77604f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 19 Nov 2021 12:18:13 +0100 Subject: [PATCH 4/7] Add tests provided by Damian --- .../host/ibc_module_test.go | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index cc19b609378..f0378a08f16 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -387,6 +387,114 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { } +func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() { + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "ICA OnAcknowledgementPacket fails with ErrInvalidChannelFlow", func() {}, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + packet := channeltypes.NewPacket( + []byte("empty packet data"), + suite.chainA.SenderAccount.GetSequence(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + clienttypes.NewHeight(0, 100), + 0, + ) + + err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, []byte("ackBytes"), TestAccAddress) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} + +func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "ICA OnTimeoutPacket fails with ErrInvalidChannelFlow", func() {}, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + packet := channeltypes.NewPacket( + []byte("empty packet data"), + suite.chainA.SenderAccount.GetSequence(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + clienttypes.NewHeight(0, 100), + 0, + ) + + err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, TestAccAddress) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} + func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { var ( proposedVersion string From 247f32252cbf64ee9572fac895d8e6bbdaa81de6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 19 Nov 2021 12:29:09 +0100 Subject: [PATCH 5/7] add tests for OnChanCloseInit and OnChanCloseConfirm on host side --- .../host/ibc_module_test.go | 73 ++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index f0378a08f16..393e748279b 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -45,7 +45,7 @@ func TestICATestSuite(t *testing.T) { } func (suite *InterchainAccountsTestSuite) SetupTest() { - suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3) + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0)) suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1)) } @@ -303,6 +303,77 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() { } +// OnChanCloseInit on host (chainB) +func (suite *InterchainAccountsTestSuite) TestOnChanCloseInit() { + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanCloseInit( + suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, + ) + + suite.Require().Error(err) +} + +func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() { + var ( + path *ibctesting.Path + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path = NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanCloseConfirm( + suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + + activeChannelID, found := suite.chainB.GetSimApp().ICAHostKeeper.GetActiveChannelID(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + + if tc.expPass { + suite.Require().NoError(err) + suite.Require().False(found) + suite.Require().Empty(activeChannelID) + } else { + suite.Require().Error(err) + } + + }) + } +} + func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { var ( packetData []byte From 901bd2b81277c0d0eb92fe3c47a3c39fe12acbb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 19 Nov 2021 12:39:11 +0100 Subject: [PATCH 6/7] add OnChanCloseInit/Confirm and NegotiateAppVersion tests to controller side --- .../controller/ibc_module_test.go | 177 ++++++++++++++++++ .../host/ibc_module_test.go | 2 +- 2 files changed, 178 insertions(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go index b3ccbd4dd24..54f07bee76c 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_module_test.go @@ -10,6 +10,7 @@ import ( "github.com/tendermint/tendermint/crypto" "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types" + clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v2/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v2/testing" @@ -352,3 +353,179 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenConfirm() { suite.Require().Error(err) } + +// OnChanCloseInit on controller (chainA) +func (suite *InterchainAccountsTestSuite) TestOnChanCloseInit() { + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanCloseInit( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + ) + + suite.Require().Error(err) +} + +func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() { + var ( + path *ibctesting.Path + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + + { + "success", func() {}, true, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path = NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + err = cbs.OnChanCloseConfirm( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + activeChannelID, found := suite.chainA.GetSimApp().ICAHostKeeper.GetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + + if tc.expPass { + suite.Require().NoError(err) + suite.Require().False(found) + suite.Require().Empty(activeChannelID) + } else { + suite.Require().Error(err) + } + + }) + } +} + +func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() { + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "ICA OnRecvPacket fails with ErrInvalidChannelFlow", func() {}, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + packet := channeltypes.NewPacket( + []byte("empty packet data"), + suite.chainA.SenderAccount.GetSequence(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + clienttypes.NewHeight(0, 100), + 0, + ) + + ack := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, TestAccAddress) + suite.Require().Equal(tc.expPass, ack.Success()) + }) + } +} + +func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { + var ( + proposedVersion string + ) + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "ICA OnRecvPacket fails with ErrInvalidChannelFlow", func() {}, false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() + path := NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + + counterpartyPortID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) + suite.Require().NoError(err) + + counterparty := channeltypes.Counterparty{ + PortId: counterpartyPortID, + ChannelId: path.EndpointB.ChannelID, + } + + proposedVersion = types.VersionPrefix + + tc.malleate() + + version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, path.EndpointA.ChannelConfig.PortID, counterparty, proposedVersion) + if tc.expPass { + suite.Require().NoError(err) + suite.Require().NoError(types.ValidateVersion(version)) + suite.Require().Equal(TestVersion, version) + } else { + suite.Require().Error(err) + suite.Require().Empty(version) + } + }) + } +} diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 393e748279b..e63d9ac60d6 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -484,7 +484,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() { tc.malleate() // malleate mutates test data - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID) + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) suite.Require().NoError(err) cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) From b203bd251f9bbc75f3490b6619864a2ddec1014e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 19 Nov 2021 14:54:34 +0100 Subject: [PATCH 7/7] fix failing test --- modules/apps/27-interchain-accounts/host/ibc_module_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index e63d9ac60d6..b3362d9faaa 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -484,10 +484,10 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() { tc.malleate() // malleate mutates test data - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) suite.Require().NoError(err) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + cbs, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) packet := channeltypes.NewPacket( @@ -501,7 +501,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() { 0, ) - err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, []byte("ackBytes"), TestAccAddress) + err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, []byte("ackBytes"), TestAccAddress) if tc.expPass { suite.Require().NoError(err)