Misbehavior detection uses wrong height information #2097
Description
Summary of Bug
Whenever Hermes finds an UpdateClient for a given consensus height h, it should perform misbehavior detection for the header in that event at that height. We call the height h a target height, i.e., this is the height that it aims to verify. Instead of using the consensus height field, Hermes mistakenly uses the event height as the target height.
Bug Description
Our definition of UpdateClient event has two notions of heights:
- an event height;
- and a consensus state height;
Both heights are fields of the Attributes type.
https://github.com/informalsystems/ibc-rs/blob/4ca8298270e8d629e2a99dbdf21a55a98f1e2dbe/modules/src/core/ics02_client/events.rs#L228-L229
As part of misbehavior detection, Hermes is not interested in the event height, but rather in the consensus state height. However, it seems like we mistakenly extract the event height to be used as target height, here:
Note: Found this bug while trying to answer a question for ibc-go devs (ref: https://github.com/cosmos/ibc-go/pull/1208/files#r851167308).
Version
hermes 0.13.0+37dbe8c4
Steps to Reproduce
Setup two chains ibc-1 and ibc-2 with a channel between them. Do hermes start with log level at trace and watch the logs. In a separate terminal, trigger an ft-transfer.
$ hermes tx raw ft-transfer ibc-2 ibc-1 transfer channel-1 9999 -o 1000 -n 1
In the running Hermes instance, we see the following:
2022-04-15T09:44:38.438365Z TRACE ThreadId(31) DetectMisbehaviorWorker{client=07-tendermint-1 src_chain=ibc-2 dst_chain=ibc-1}: received batch: EventBatch { chain_id: ChainId { id: "ibc-1", version: 1 }, height: Height { revision: 1, height: 16804 }, events: [UpdateClient(UpdateClient { common: Attributes { height: Height { revision: 1, height: 16804 }, client_id: ClientId("07-tendermint-1"), client_type: Tendermint, consensus_height: Height { revision: 2, height: 16807 } }, header: Some(Tendermint( Header {...})) })] }
2022-04-15T09:44:38.438656Z DEBUG ThreadId(31) DetectMisbehaviorWorker{client=07-tendermint-1 src_chain=ibc-2 dst_chain=ibc-1}: checking misbehavior for updated client
2022-04-15T09:44:38.551026Z TRACE ThreadId(31) DetectMisbehaviorWorker{client=07-tendermint-1 src_chain=ibc-2 dst_chain=ibc-1}: [ibc-2 -> ibc-1:07-tendermint-1] checking misbehaviour for consensus state heights (first 50 shown here): 1-16804, total: 1
The last line of this log says that Hermes is performing misbehavior checking for height 1-16804. The client that is concerned here is 07-tendermint-1 which is hosted on ibc-1 (the destination chain) and is verifying headers of ibc-2, the source chain. Since the source chain has revision_number 2, it doesn't make sense for Hermes to perform misbehavior checking on a header with height 1-16804, since this height cannot originate from ibc-2. Instead, Hermes should be performing misbehavior checking for Height { revision: 2, height: 16807 }.
Acceptance Criteria
- Misbehavior detection task should use as target height the appropriate field from the
UpdateClientevent
For Admin Use
- Not duplicate issue
- Appropriate labels applied
- Appropriate milestone (priority) applied
- Appropriate contributors tagged
- Contributor assigned/self-assigned