Cosmos-sdk: ConnOpenTry() fails if a connection exists with uninitialized counterparty connection id

[ancazamfir]

Summary of Bug

If a previous connection exists with uninitialized counterparty connection id, ConnOpenTry fails here:
https://github.com/cosmos/cosmos-sdk/blob/fe8a891f11dcefe708e43d53549beec808f98687/x/ibc/core/03-connection/keeper/handshake.go#L118

Haven't tested the channel case but looks like the same check is there:
https://github.com/cosmos/cosmos-sdk/blob/fe8a891f11dcefe708e43d53549beec808f98687/x/ibc/core/04-channel/keeper/handshake.go#L121

Version

v0.40.0-rc0 but looks the same on master

Steps to Reproduce

Testing with raw commands from the Rust relayer from a development branch. The only difference with the Go relayer being that we allow building OpenInit with unspecified counterparty connection id.

---

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[colin-axner]

cc @cwgoes

[cwgoes]

What is an "uninitialized counterparty connection id"? How would a connection ever be created with one?

Do you mean empty string?

[ancazamfir]

Do you mean empty string?

yes

[cwgoes]

Right, OK, this needs a spec change.

[ebuchman]

Is this just a spec change or a bug? I think I came across a similar issue during Informal Systems audit of crossing hellos just now, and it seems it's not just about the ConnectionID, it expects other parameters of the existing connection to match as well, eg. the clientID. So if ChainB has two clients for ChainA, call them client1 and client2, and then a crossing hello's happens with ConnOpenInit being called on both chains, but on ChainA it's called with reference to client1 (ie. counterparty.ClientID = client1) and on ChainB it's called with reference to client2 (ie. clientID = client2), but they use the correct/expected connection ID, then I think the connection will fail to ever open because this previousConnection.ClientId == clientID check will fail?

https://github.com/cosmos/cosmos-sdk/blob/fe8a891f11dcefe708e43d53549beec808f98687/x/ibc/core/03-connection/keeper/handshake.go#L116-L122

I didn't see a test for crossing connection hellos - are there any? I tried to write one for this scenario, but not sure I got it all right. This does seem to test crossing hellos and passes/fails in the expected spot :

func (suite *KeeperTestSuite) TestConnOpen2Try() {
    var (
        clientA            string
        clientB, clientB2  string
        versions           []exported.Version
        consensusHeight    exported.Height
        counterpartyClient exported.ClientState
    )

    f := func() {

        // create clients on both chain
        clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, ibctesting.Tendermint)

        // create another client for chain A on chainB
        var err error
        clientB2, err = suite.coordinator.CreateClient(suite.chainB, suite.chainA, ibctesting.Tendermint)
        suite.Require().NoError(err)

        // call OpenInit on chain A with reference to clientB
        connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB)
        suite.Require().NoError(err)
        fmt.Println("connA", connA)
        fmt.Println("connB", connB)

        // call OpenInit (crossing hello) on chain B with reference to clientB2 but with desired connectionID.
        // this will cause the test to fail.
        // Note if we call ConnOpenInit here with clientB instead of clientB2, the test passes
        counterparty := types.NewCounterparty(clientA, connA.ID, suite.chainA.GetPrefix())
        err = suite.chainB.App.IBCKeeper.ConnectionKeeper.ConnOpenInit(suite.chainB.GetContext(), connB.ID, clientB2, counterparty, nil)
        suite.Require().NoError(err)

        err = suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
        suite.Require().NoError(err)

        err = suite.coordinator.UpdateClient(suite.chainB, suite.chainA, clientB, ibctesting.Tendermint)
        suite.Require().NoError(err)

        // retrieve client state of chainA to pass as counterpartyClient
        counterpartyClient = suite.chainA.GetClientState(clientA)
    }

    suite.Run("crossing", func() {
        suite.SetupTest()                          // reset
        consensusHeight = clienttypes.ZeroHeight() // must be explicitly changed in malleate
        versions = types.GetCompatibleVersions()   // must be explicitly changed in malleate

        f()

        connA := suite.chainA.GetFirstTestConnection(clientA, clientB)
        connB := suite.chainB.GetFirstTestConnection(clientB, clientA)
        counterparty := types.NewCounterparty(clientA, connA.ID, suite.chainA.GetPrefix())
        fmt.Println("connA, connB", connA, connB)

        // get counterpartyChosenConnectionID
        var counterpartyChosenConnectionID string
        connection, found := suite.chainA.App.IBCKeeper.ConnectionKeeper.GetConnection(suite.chainA.GetContext(), connA.ID)
        if found {
            counterpartyChosenConnectionID = connection.Counterparty.ConnectionId
        }

        connectionKey := host.ConnectionKey(connA.ID)
        proofInit, proofHeight := suite.chainA.QueryProof(connectionKey)

        if consensusHeight.IsZero() {
            // retrieve consensus state height to provide proof for
            consensusHeight = counterpartyClient.GetLatestHeight()
        }
        consensusKey := host.FullConsensusStateKey(clientA, consensusHeight)
        proofConsensus, _ := suite.chainA.QueryProof(consensusKey)

        // retrieve proof of counterparty clientstate on chainA
        clientKey := host.FullClientStateKey(clientA)
        proofClient, _ := suite.chainA.QueryProof(clientKey)

        fmt.Println("OPEN TRY")
        err := suite.chainB.App.IBCKeeper.ConnectionKeeper.ConnOpenTry(
            suite.chainB.GetContext(), connB.ID, counterpartyChosenConnectionID, counterparty, clientB, counterpartyClient,
            versions, proofInit, proofClient, proofConsensus,
            proofHeight, consensusHeight,
        )

        suite.Require().NoError(err)
    })
}

[colin-axner]

I didn't see a test for crossing connection hellos - are there any?

There are a couple doing basic unit testing in connection/channel handshake but definitely not enough. We have an issue tracking it #7603.

The test looks correct to me. On chainA it creates connection from clientA to clientB and on chainB it creates a connection from clientB2 to clientA, all with the same connectionID. Since the clients are different but the same connection IDs are used, with the current construction it is expected to fail

[ancazamfir]

Is this just a spec change or a bug?

I believe it's both. With empty counterparty connection id for existing connection we expect the check to pass but it fails. Probably this check (both in code and spec) was just overlooked when flexible connection id was introduced.

I think I came across a similar issue during review of crossing hellos just now, and it seems it's not just about the ConnectionID, it expects other parameters of the existing connection to match as well, eg. the clientID.

All the other parameters must be specified (i.e. non-empty strings) in the existing connection. For these the check fails, as expected, if they don't match the ones in OpenTry.

[ebuchman]

Since the clients are different but the same connection IDs are used, with the current construction it is expected to fail

But can't a malicious relayer race to do this every time it sees a ConnOpenInit and thus attempt to block anyone from ever opening a connection?

[ancazamfir]

But can't a malicious relayer race to do this every time it sees a ConnOpenInit and thus attempt to block anyone from ever opening a connection?

Yes but I opened this issue just to fix quickly the case where we only deal with correct relayers.

re: malicious relayer -> I think there should be another issue where we discuss this since it will probably be a longer discussion.

Talking a few days ago with @milosevic and Ilina about this:
A connection or channel will never reach Open state if there is a mismatch in the identifiers. It is very simple to write a relayer that watches the OpenInit events from A and immediately send a MsgOpenInit to B with mismatched connection ids, blocking forever the handshake on the connection/ channel.

I think the “correct” relayer that would guarantee that the handshake finishes would have to do something like:

• always send MsgOpenInit without specifying the counterparty connection id (this is allowed now in order to support flexible id selection)
• specify the counterpary connection id in OpenTry (i.e. tie the two connection ends with a message that carries proofs)

So maybe the solution is to not allow the MsgOpenInit with counterparty connection_id specified.

[cwgoes]

But can't a malicious relayer race to do this every time it sees a ConnOpenInit and thus attempt to block anyone from ever opening a connection?

No, unless the initiator of the handshake specified a particular counterparty connection ID, in which case the relayer can race anyways.

For the original issue outlined above I think we just need to check that either the counterparty connection ID matches or that our previously stored counterparty connection ID was unspecified (empty string) indicating that the counterparty should be allowed to pick it.

[cwgoes]

@ancazamfir I've made the spec changes in https://github.com/cosmos/ics/pull/493 if you want to take a look.