Misbehavior detection uses wrong height information

adizere commented 2 years ago

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:

https://github.com/informalsystems/ibc-rs/blob/1fda889b159c7706399704c4a1ea3a1dd4823b1d/relayer/src/foreign_client.rs#L1166-L1167

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 UpdateClient event

For Admin Use

[X] Not duplicate issue
[X] Appropriate labels applied
[X] Appropriate milestone (priority) applied
[X] Appropriate contributors tagged
[X] Contributor assigned/self-assigned

ancazamfir commented 2 years ago

ouch, i broke this in https://github.com/informalsystems/ibc-rs/pull/1512

ancazamfir commented 2 years ago

btw, do we check anywhere that the event consensus_height matches the header height?

adizere commented 2 years ago

btw, do we check anywhere that the event consensus_height matches the header height?

I don't think we check this. It would be good safety measure, or you have some specific scenario in mind?

ancazamfir commented 2 years ago

btw, do we check anywhere that the event consensus_height matches the header height?

I don't think we check this. It would be good safety measure, or you have some specific scenario in mind?

no, just safety for redundant/ duplicate data.

seanchen1991 commented 2 years ago

Does this code perform the check you're referring to? https://github.com/informalsystems/ibc-rs/blob/9411184a7666628e553419bf96dff90a6fb6ea18/relayer/src/foreign_client.rs#L1206-L1214

informalsystems / hermes