Fix for chain impersonation problem. Fix for incomplete channel create

[adizere]

Closes: #1038
Closes: #1064

For testing instructions for issue 1064, see the description within that issue for more details.
The basic command that is undergoing changes here is hermes create channel.

Testing for 1038

You will need two configuration files for Hermes.

1. save this into ~/.hermes/config.toml
2. save this into ~/.hermes/config_fake.toml

You will also need to use this for gm. Make sure that the binary option in that file points to the correct path to your hermes binary.

1. Make sure no gaia instance is running, i.e., killall gaiad and gm stop
   • if some gm chains were running before, gm reset them to clear their state
2. do ./scripts/dev-env ~/.hermes/config.toml ibc-0 ibc-1 and wait for it finish
   • the chain ibc-1 on grpc port 9091 will be our real chain where the ft-transfer should arrive
   • the config for this chain is consistent with that in file config.toml
3. gm start && gm hermes keys
   • this starts another chain, also called ibc-1, but this gaia is running on a different grpc port 27041
   • this will be our ibc-1 that is fake
   • the config for this chain is consistent with that in file config_fake.toml
4. hermes create channel ibc-0 ibc-1 --port-a transfer --port-b transfer
   • creates a channel: ibc-0:channel-0 ----> real ibc-1:channel-1
5. hermes -c ~/.hermes/config_fake.toml create channel ibc-0 ibc-1 --port-a transfer --port-b transfer
   • creates a channel: ibc-0:channel-1 ----> fake ibc-1:channel-1
   • the complete picture of the state of the three chains is as follows

ibc-1 (real)                                ibc-0
     (channel-0, connection-0, client-0) <-->   (channel-0, connection-0, client-0)

ibc-1 (fake)                                ibc-0
     (channel-0, connection-0, client-0) <-->   (channel-1, connection-1, client-1)

6. now submit an ft-transfer on ibc-0, on end ibc-0:channel-1, destined to arrive to the fake ibc-1 chain,

   • hermes -c ~/.hermes/config_fake.toml tx raw ft-transfer ibc-1 ibc-0 transfer channel-1 9999 -o 1000
   • this behavior is correct, so far

7. try to submit an ft-transfer on ibc-0, on end ibc-0:channel-1, destined to arrive to the real ibc-1 chain

   • we omit the -c option to use the config of the real ibc-1 chain
   • note that we use ibc-0:channel-1 to submit this transfer: this specific channel end on ibc-0 corresponds to a counterparty channel end ibc-1:channel-0, which in turn has counterparty ibc-0:channel-0; in other words, this command should fail, because the counterparty on the destination chain (namely, ibc-0:channel-0) does not match the source channel (ibc-0:channel-1):
   • hermes tx raw ft-transfer ibc-1 ibc-0 transfer channel-1 9999 -o 1000
   • with the changes in this PR, the following message is output:

Error: socket transfer/channel-0 on chain ibc-1 expected to have counterparty transfer/channel-1 (but instead has transfer/channel-0)

---

For contributor use:

• Updated the Unreleased section of CHANGELOG.md with the issue.
• If applicable: Unit tests written, added test to CI.
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments.
• Re-reviewed Files changed in the Github PR explorer.

[adizere]

@soareschen You recently refactored the code in relayer/src/channel.rs and I'm modifying that in this PR. Could you please have a look over my changes here? I'd like your input whether my modifications (in particular 29fba58) are in line with your refactoring. The problem I'm trying to solve is described in #1064. Any thoughts appreciated!

[soareschen]

I have not run the code yet, but here are some of my comments.

[ancazamfir]

When I follow the instructions I get:

$ hermes -c ~/.hermes/config_fake.toml create channel ibc-0 ibc-1 --port-a transfer --port-b transfer
...
Jun 14 09:20:30.733 ERROR ibc_relayer::foreign_client: [ibc-1 -> ibc-0:07-tendermint-0]  failed CreateClient: error raised while creating client: failed while building client state from src chain (ibc-1) with error: GRPC error: transport error: error trying to connect: received corrupt message
Error: error raised while creating client: Create client failed (ClientCreate("failed while building client state from src chain (ibc-1) with error: GRPC error: transport error: error trying to connect: received corrupt message"))```

[ancazamfir]

weird, if I do:

• ./scripts/dev-env ~/.hermes/config.toml ibc-0 ibc-1
• gm start && gm hermes keys
• hermes -c ~/.hermes/config_fake.toml create channel ibc-0 ibc-1 --port-a transfer --port-b transfer
• hermes -c ~/.hermes/config_fake.toml query channel end ibc-1 transfer channel-0

...
Success: ChannelEnd {
    state: Init,
    ordering: Unordered,
    remote: Counterparty {
        port_id: PortId(
            "transfer",
        ),
        channel_id: None,
    },
    connection_hops: [
        ConnectionId(
            "connection-0",
        ),
    ],
    version: "ics20-1",
}

• hermes query channel end ibc-1 transfer channel-0

Success: ChannelEnd {
    state: Open,
    ordering: Unordered,
    remote: Counterparty {
        port_id: PortId(
            "transfer",
        ),
        channel_id: Some(
            ChannelId(
                "channel-0",
            ),
        ),
    },
    connection_hops: [
        ConnectionId(
            "connection-0",
        ),
    ],
    version: "ics20-1",
}

[ancazamfir]

A few thoughts triggered by this:

• we initially discussed to start uni path workers on packet events and not for every open channel. I am now wondering, from the perspective of this issue/ PR, if it's not better to start them for open channels that have valid cross-referenced IDs and NOT start them for packet events anymore.
  • this would mean that every time a chanel is open-open (i.e. on confirm events) we start the two uni path workers if channel is valid (as per restore check in this PR)
  • we may end up with a lot of passive workers if the channels are not active but
  • the advantage is that we don't churn on packet events (create worker and immediately stop if the check fails)
• with the two chains with same name, hermes client worker detects misbehavior but submission fails as the two chains are not really forks (i.e. there is not common ancestor consensus state). The worker reports an error in the log but continues to run, skipping misbehaviour checking, for the purpose of client refresh which will fail of course.
  • the original design was that when the MisbehaviorEvent is received, the worker submits the evidence to the full node and exits which is fine, but
  • in this case though the evidence submission fails (so no event on the wire to let the client worker exit). We do not check the returned event (ChainError versus MisbehaviorEvent). It would probably be good to check and exit (so we don't try some update client later on refresh timeout)
  • if you try hermes create connection ibc-0 --client-a 07-tendermint-1 --client-b 07-tendermint-0 hermes retries a number of times with errors like this:
    Jun 14 15:55:34.525 ERROR ibc_relayer::connection: Failed ConnTry ConnectionSide { chain: ProdChainHandle { chain_id: ChainId { id: "ibc-1", version: 1 }, runtime_sender: Sender { .. } }, client_id: ClientId("07-tendermint-0"), connection_id: ConnectionId("connection-0") }: failed with underlying cause: tx response error: deliver_tx reports error: log=Log("failed to execute message; message index: 1: connection handshake open try failed: failed consensus state verification for client (07-tendermint-0): chained membership proof failed to verify membership of value: 0A2E2F6962632E6C69676874636C69656E74732E74656E6465726D696E742E76312E436F6E73656E737573537461746512540A0C088FC49E860610A8EF97CF0212220A20B23FEDC50B4967BEDDBCE52D9FB566BD95C390D04B63A8279D1830C4356CA10B1A200E65CB2CF370D4DA97FDAB1C81413C75D6E3655E4B87ABC172C1B52795325F7B in subroot 58703300193DCC0B2D296DBD5BD446C9569534E5AFEF33956A57206BD5C31A1B at index 0. Please ensure the path and value are both correct.: invalid proof")
  • This could be avoided if hermes would blacklist 07-tendermint-1 on ibc-0. Similarly it shouldn't try with frozen or expired clients...not sure this is checked.

[ancazamfir]

Great work @adizere! Looks good to me to merge. We may consider opening some issues in the future to reduce the effect of these types of attacks but definitely a very good start.

addressed

[adizere]

Captured Anca's thoughts in a discussion, so let's follow up there -- #1092