Fix for chain impersonation problem. Fix for incomplete `channel crea…

…te` (#1066)

* Countermeasure that fixes #1038

* Tentative fix for #1064

* Better fix for the impersonation problem

* Better log message. Handle (TryOpen, Init) case

* Changelog

* Better error, added doc comment, less clones.

* Clarify the intent of method do_chan_open_ack_confirm_step

* Removed some unnecessary clones

* Moved check_channel_counterparty to chain::counterparty

* Fully specified error in case of failure in check_channel_counterparty

* Used port+channel instead of socket

* Fix small typo in error variant

* Removed chan counterparty verification in ft-transfer.

Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>

CHANGELOG.md

@@ -19,11 +19,20 @@
   - Update the on-chain IBC client with supporting headers when light client verification
     does bisection when verifying a header for a client update or a misbehaviour detection ([#673])
 
+### BUG FIXES
+
+- [ibc-relayer-cli]
+  - Fix for chain impersonation bug. ([#1038])
+  - Fix for partially open handshake bug of `channel create` CLI. ([#1064])
+
 ### BREAKING CHANGES
 
 - [ibc-relayer-cli]
   - Removed `--coin-type` option from `keys restore` command. Use `--hd-path` instead. ([#1049])
-
+
+[#1038]: https://github.com/informalsystems/ibc-rs/issues/1038
+[#1049]: https://github.com/informalsystems/ibc-rs/issues/1049
+[#1064]: https://github.com/informalsystems/ibc-rs/issues/1064
 [#673]: https://github.com/informalsystems/ibc-rs/issues/673
 [#868]: https://github.com/informalsystems/ibc-rs/issues/1049
 [#877]: https://github.com/informalsystems/ibc-rs/issues/877

modules/src/ics24_host/identifier.rs

@@ -391,3 +391,9 @@ pub struct PortChannelId {
     pub channel_id: ChannelId,
     pub port_id: PortId,
 }
+
+impl std::fmt::Display for PortChannelId {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
+        write!(f, "{}/{}", self.port_id, self.channel_id)
+    }
+}

relayer-cli/src/commands/tx/transfer.rs

@@ -1,18 +1,18 @@
-use std::sync::Arc;
-
 use abscissa_core::{config::Override, Command, FrameworkErrorKind, Options, Runnable};
 use anomaly::BoxError;
-use tokio::runtime::Runtime as TokioRuntime;
 
-use ibc::events::IbcEvent;
-use ibc::ics02_client::height::Height;
-use ibc::ics24_host::identifier::{ChainId, ChannelId, PortId};
+use ibc::{
+    events::IbcEvent,
+    ics02_client::client_state::ClientState,
+    ics02_client::height::Height,
+    ics24_host::identifier::{ChainId, ChannelId, PortId},
+};
 use ibc_relayer::{
-    chain::{Chain, CosmosSdkChain},
     config::Config,
     transfer::{build_and_send_transfer_messages, TransferOptions},
 };
 
+use crate::cli_utils::ChainHandlePair;
 use crate::conclude::{exit_with_unrecoverable_error, Output};
 use crate::error::{Error, Kind};
 use crate::prelude::*;
@@ -132,48 +132,33 @@ impl Runnable for TxIcs20MsgTransferCmd {
 
         debug!("Message: {:?}", opts);
 
-        let rt = Arc::new(TokioRuntime::new().unwrap());
-
-        let src_chain_res =
-            CosmosSdkChain::bootstrap(opts.packet_src_chain_config.clone(), rt.clone())
-                .map_err(|e| Kind::Runtime.context(e));
-
-        let src_chain = match src_chain_res {
-            Ok(chain) => chain,
-            Err(e) => return Output::error(format!("{}", e)).exit(),
-        };
-
-        let dst_chain_res = CosmosSdkChain::bootstrap(opts.packet_dst_chain_config.clone(), rt)
-            .map_err(|e| Kind::Runtime.context(e));
-
-        let dst_chain = match dst_chain_res {
-            Ok(chain) => chain,
-            Err(e) => return Output::error(format!("{}", e)).exit(),
-        };
+        let chains = ChainHandlePair::spawn(&config, &self.src_chain_id, &self.dst_chain_id)
+            .unwrap_or_else(exit_with_unrecoverable_error);
 
         // Double check that channels and chain identifiers match.
         // To do this, fetch from the source chain the channel end, then the associated connection
         // end, and then the underlying client state; finally, check that this client is verifying
         // headers for the destination chain.
-        let channel_end = src_chain
+        let channel_end_src = chains
+            .src
             .query_channel(
                 &opts.packet_src_port_id,
                 &opts.packet_src_channel_id,
                 Height::zero(),
             )
             .unwrap_or_else(exit_with_unrecoverable_error);
-        if !channel_end.is_open() {
+        if !channel_end_src.is_open() {
             return Output::error(format!(
                 "the requested port/channel ('{}'/'{}') on chain id '{}' is in state '{}'; expected 'open' state",
                 opts.packet_src_port_id,
                 opts.packet_src_channel_id,
                 self.src_chain_id,
-                channel_end.state
+                channel_end_src.state
             ))
             .exit();
         }
 
-        let conn_id = match channel_end.connection_hops.first() {
+        let conn_id = match channel_end_src.connection_hops.first() {
             None => {
                 return Output::error(format!(
                     "could not retrieve the connection hop underlying port/channel '{}'/'{}' on chain '{}'",
@@ -184,13 +169,15 @@ impl Runnable for TxIcs20MsgTransferCmd {
             Some(cid) => cid,
         };
 
-        let conn_end = src_chain
+        let conn_end = chains
+            .src
             .query_connection(conn_id, Height::zero())
             .unwrap_or_else(exit_with_unrecoverable_error);
 
         debug!("connection hop underlying the channel: {:?}", conn_end);
 
-        let src_chain_client_state = src_chain
+        let src_chain_client_state = chains
+            .src
             .query_client_state(conn_end.client_id(), Height::zero())
             .unwrap_or_else(exit_with_unrecoverable_error);
 
@@ -199,18 +186,18 @@ impl Runnable for TxIcs20MsgTransferCmd {
             src_chain_client_state
         );
 
-        if src_chain_client_state.chain_id != self.dst_chain_id {
+        if src_chain_client_state.chain_id() != self.dst_chain_id {
             return Output::error(
                 format!("the requested port/channel ('{}'/'{}') provides a path from chain '{}' to \
                  chain '{}' (not to the destination chain '{}'). Bailing due to mismatching arguments.",
                         opts.packet_src_port_id, opts.packet_src_channel_id,
                         self.src_chain_id,
-                        src_chain_client_state.chain_id, self.dst_chain_id)).exit();
+                        src_chain_client_state.chain_id(), self.dst_chain_id)).exit();
         }
 
         // Checks pass, build and send the tx
         let res: Result<Vec<IbcEvent>, Error> =
-            build_and_send_transfer_messages(src_chain, dst_chain, opts)
+            build_and_send_transfer_messages(chains.src, chains.dst, opts)
                 .map_err(|e| Kind::Tx.context(e).into());
 
         match res {

relayer/src/chain/counterparty.rs

@@ -1,16 +1,16 @@
 use serde::{Deserialize, Serialize};
-use tracing::trace;
+use tracing::{error, trace};
 
 use ibc::{
     ics02_client::client_state::{ClientState, IdentifiedAnyClientState},
     ics03_connection::connection::IdentifiedConnectionEnd,
     ics04_channel::channel::{ChannelEnd, IdentifiedChannelEnd, State},
-    ics24_host::identifier::ConnectionId,
-    ics24_host::identifier::{ChainId, ChannelId, PortId},
+    ics24_host::identifier::{ChainId, ChannelId, ConnectionId, PortChannelId, PortId},
     Height,
 };
 use ibc_proto::ibc::core::channel::v1::QueryConnectionChannelsRequest;
 
+use crate::channel::ChannelError;
 use crate::supervisor::Error;
 
 use super::handle::ChainHandle;
@@ -158,3 +158,52 @@ pub fn channel_state_on_destination(
         };
     Ok(counterparty_state)
 }
+
+/// Queries a channel end on a [`ChainHandle`], and verifies
+/// that the counterparty field on that channel end matches an
+/// expected counterparty.
+/// Returns `Ok` if the counterparty matches, and `Err` otherwise.
+pub fn check_channel_counterparty(
+    target_chain: Box<dyn ChainHandle>,
+    target_pchan: &PortChannelId,
+    expected: &PortChannelId,
+) -> Result<(), ChannelError> {
+    let channel_end_dst = target_chain
+        .query_channel(
+            &target_pchan.port_id,
+            &target_pchan.channel_id,
+            Height::zero(),
+        )
+        .map_err(|e| ChannelError::QueryError(target_chain.id(), e))?;
+
+    let counterparty = channel_end_dst.remote;
+    match counterparty.channel_id {
+        Some(actual_channel_id) => {
+            let actual = PortChannelId {
+                channel_id: actual_channel_id,
+                port_id: counterparty.port_id,
+            };
+            if &actual != expected {
+                return Err(ChannelError::MismatchingChannelEnds(
+                    target_pchan.clone(),
+                    target_chain.id(),
+                    expected.clone(),
+                    actual,
+                ));
+            }
+        }
+        None => {
+            error!(
+                "channel {} on chain {} has no counterparty channel id ",
+                target_pchan,
+                target_chain.id()
+            );
+            return Err(ChannelError::IncompleteChannelState(
+                target_pchan.clone(),
+                target_chain.id(),
+            ));
+        }
+    }
+
+    Ok(())
+}