Hermes: Make sure hermes cannot be slowed down by packets to fake chains

[gamarin2]

If I'm not mistaken, when Hermes catches an IBC transfer to relay, it does the following:

1. Figure out source channel
2. Using source channel, query the source chain to get the counterparty's chain-id
3. Create packets and submit them to counterparty chain

The (potential) issue is that chain-ids are not unique, meaning it is possible for someone to create an IBC connection with a chain-id that is exactly the same as the chain-id of a "real chain". This could potentially be a problem is hermes retries multiple times if header verification fails. Consider the following scenario:

1. An IBC connection between Cosmos Hub and Akash is open. On Cosmos Hub, the client ID of Akash is client-1 and the chain-id is akash-1.
2. An attacker creates a fake Akash chain using starport and opens an IBC connection with the Hub. The attacker uses the same chain-id akash-1, only the client-id is different, client-2.
3. The attacker then sends a bunch of IBC transfers from Cosmos Hub to the fake Akash chain.
4. Hermes picks up the packets. It reads the counterparty chain-id for the channel, which gives akash-1.
5. Hermes then tries to relay the packet to the real Akash chain, but the relaying fails during header verification. Hermes then retries (how many times?).

My questions is: do we have a problem? Can this slow down Hermes significantly?

cc @ancazamfir @romac @andynog

[ancazamfir]

Thanks for the clarification, I understood something different in our discussion.

So I think we can detect this and drop the send packet event early. In step 4 we find the channel end on real akash-1. Then we verify that the counterparty points back to the source_channel_id in the packet event. I looked at the code and I don't think we catch this anywhere. If there is some check it's definitely not where it should be.

I think this should work as (port ID, channel ID) -s must be unique per chain. In fact on cosmos chains channel IDs alone are unique per chain. Same for connection IDs.
So on the hub the channel ID to the fake akash-1 should be different than the channel ID to the real chain.

Here is an example of the channel, connection and client IDs on the two akash chains

akash-1:
     (channel-10, connection-10, client-10) --> (channel-1, connection-1, client-1)
fake akash-1:  
     (channel-10, connection-10, client-10) --> (channel-2, connection-2, client-2)

• relayer gets the send_packet event from the hub with source_channel_id = channel-2
• queries channel-2 and gets the counterparty channel ID, channel-10
• gets the hub connection then client from which it figures the counterparty is akash-1
• queries the real akash-1 for channel-10
• checks the counterparty information of channel-10 against the event source_channel_id.
  • In this case the check should fail as counterparty of channel-10 on real akash-1 should be channel-1 which does not match the packet's channel-2
• hermes should drop the event if the check above fails

Note also that the client updates will not fail in this case as the headers are valid hub headers for a client ID that exists on the real akash-1. With today's code we will retry < 10 times.

[gamarin2]

I didn't know channel-id was unique. Is this something implementation-related, or something specified in the IBC spec. If it's the former, maybe using client-id would be more robust? In any case, the general idea of what you proposed @ancazamfir makes sense. That's what we've implemented in our own backend to verify IBC paths.

[ancazamfir]

Is this something implementation-related, or something specified in the IBC spec.

It is in the ICS24 IBC spec. There is a table that lists the keys that identify the different objects (e.g. clientState, connectionEnd, channelEnd).

We will add the check I mentioned above and will also look into doing the similar check at connectionEnd and clientState level to see if they add to the security. My concern is that these queries take time. Need to check if we can make use of the little information we cache, otherwise we would have to do more queries for each event/ packet or make a small design change.

[ancazamfir]

BTW, there is a gRPC query implemented in SDK that, given a channel and port ID, retrieves the client data (includes the client ID and state). So it's faster if you don't need connection information.

[adizere]

I can confirm this is a problem.

The conditions I recreated are not exactly the same as in the above description of this issue, but suffice to say that Hermes can become confused and relay packets to a different chain that the original chain that was intended to receive them,

> hermes -c ~/.hermes/config_fake.toml tx raw packet-recv ibc-1 ibc-0 transfer channel-0
... 
Jun 08 14:44:49.478 DEBUG ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] packets that still have commitments on ibc-0: [1]
Jun 08 14:44:49.482 DEBUG ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] recv packets to send out to ibc-1 of the ones with commitments on source ibc-0: [Sequence(1)]
Jun 08 14:44:49.485 DEBUG ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] received from query_txs [Sequence(1)]
Jun 08 14:44:49.485  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] generate messages from batch with 1 events
Jun 08 14:44:49.488 DEBUG ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] ibc-0 => SendPacketEv(SendPacket - h:0-3506, seq:1, path:channel-0/transfer->channel-0/transfer, toh:1-1287, tos:Timestamp(NoTimestamp)))
Jun 08 14:44:49.496 TRACE ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] built recv_packet msg seq:1, path:channel-0/transfer->channel-0/transfer, toh:1-1287, tos:Timestamp(NoTimestamp)), proofs at height 0-3507
Jun 08 14:44:49.496 DEBUG ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] ibc-1 <= /ibc.core.channel.v1.MsgRecvPacket from SendPacketEv(SendPacket - h:0-3506, seq:1, path:channel-0/transfer->channel-0/transfer, toh:1-1287, tos:Timestamp(NoTimestamp)))
Jun 08 14:44:49.496  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] scheduling op. data with 1 msg(s) for Destination chain (height 0-3507)
Jun 08 14:44:49.496  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] ready to fetch a scheduled op. data with batch of size 1 targeting Destination
Jun 08 14:44:49.496  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] relay op. data to Destination, proofs height 0-3506, (delayed by: 47.319µs) [try 1/5]
Jun 08 14:44:49.496  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] prepending Destination client update @ height 0-3507
Jun 08 14:44:50.543 DEBUG ibc_relayer::foreign_client: [ibc-0 -> ibc-1:07-tendermint-0] MsgUpdateAnyClient for target height 0-3507 and trusted height 0-3368
Jun 08 14:44:50.543  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] assembled batch of 2 message(s)
Jun 08 14:44:56.321  INFO ibc_relayer::link: [ibc-0:transfer/channel-0 -> ibc-1] result events:
	ChainErrorEv(deliver_tx reports error: log=Log("failed to execute message; message index: 1: receive packet verification failed: packet source channel doesn\'t match the counterparty\'s channel (channel-0 ≠ channel-1): invalid packet"))

As can be seen in the last log line I pasted above, the destination chain, fake ibc-1, rejects the packet on grounds of mismatching source channel (which is the correct behavior to be expected from a chain). The actual destination was another chain..

The command hermes -c ~/.hermes/config_fake.toml start encounters the same problem.