diff --git a/CHANGELOG.md b/CHANGELOG.md index c385daac19..09e802582a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/modules/src/ics24_host/identifier.rs b/modules/src/ics24_host/identifier.rs index cf6b969c29..cdc991427f 100644 --- a/modules/src/ics24_host/identifier.rs +++ b/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) + } +} diff --git a/relayer-cli/src/commands/tx/transfer.rs b/relayer-cli/src/commands/tx/transfer.rs index 65569dc175..5b1525d234 100644 --- a/relayer-cli/src/commands/tx/transfer.rs +++ b/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, 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 { diff --git a/relayer/src/chain/counterparty.rs b/relayer/src/chain/counterparty.rs index 958bc9b2ff..4f2338d43b 100644 --- a/relayer/src/chain/counterparty.rs +++ b/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, + 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(()) +} diff --git a/relayer/src/channel.rs b/relayer/src/channel.rs index 75bff12ad9..66f38079b4 100644 --- a/relayer/src/channel.rs +++ b/relayer/src/channel.rs @@ -1,8 +1,10 @@ #![allow(clippy::borrowed_box)] + +use std::time::Duration; + use anomaly::BoxError; use prost_types::Any; use serde::Serialize; -use std::time::Duration; use thiserror::Error; use tracing::{debug, error, info}; @@ -14,24 +16,30 @@ use ibc::ics04_channel::msgs::chan_open_ack::MsgChannelOpenAck; use ibc::ics04_channel::msgs::chan_open_confirm::MsgChannelOpenConfirm; use ibc::ics04_channel::msgs::chan_open_init::MsgChannelOpenInit; use ibc::ics04_channel::msgs::chan_open_try::MsgChannelOpenTry; -use ibc::ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId}; +use ibc::ics24_host::identifier::{ + ChainId, ChannelId, ClientId, ConnectionId, PortChannelId, PortId, +}; use ibc::tx_msg::Msg; use ibc::Height; +use ibc_proto::ibc::core::channel::v1::QueryConnectionChannelsRequest; +use crate::chain::counterparty::{channel_connection_client, channel_state_on_destination}; use crate::chain::handle::ChainHandle; use crate::connection::Connection; use crate::error::Error; use crate::foreign_client::{ForeignClient, ForeignClientError}; use crate::object::Channel as WorkerChannelObject; use crate::supervisor::Error as WorkerChannelError; - +use crate::util::retry::RetryResult; use crate::util::retry::{retry_count, retry_with_index}; mod retry_strategy { - use crate::util::retry::clamp_total; - use retry::delay::Fibonacci; use std::time::Duration; + use retry::delay::Fibonacci; + + use crate::util::retry::clamp_total; + // Default parameters for the retrying mechanism const MAX_DELAY: Duration = Duration::from_secs(60); // 1 minute const MAX_TOTAL_DELAY: Duration = Duration::from_secs(10 * 60); // 10 minutes @@ -42,10 +50,6 @@ mod retry_strategy { } } -use crate::chain::counterparty::{channel_connection_client, channel_state_on_destination}; -use crate::util::retry::RetryResult; -use ibc_proto::ibc::core::channel::v1::QueryConnectionChannelsRequest; - #[derive(Debug, Error)] pub enum ChannelError { #[error("failed with underlying cause: {0}")] @@ -70,6 +74,15 @@ pub enum ChannelError { "failed during a transaction submission step to chain id {0} with underlying error: {1}" )] SubmitError(ChainId, Error), + + #[error("the channel is partially open ({0}, {1})")] + PartialOpenHandshake(State, State), + + #[error("channel {0} on chain {1} has no counterparty channel id")] + IncompleteChannelState(PortChannelId, ChainId), + + #[error("channel {0} on chain {1} expected to have counterparty {2} (but instead has {3})")] + MismatchingChannelEnds(PortChannelId, ChainId, PortChannelId, PortChannelId), } #[derive(Clone, Debug, Serialize)] @@ -407,7 +420,20 @@ impl Channel { Ok(()) } - fn do_channel_handshake(&mut self) -> Result<(), ChannelError> { + /// Does a single step towards finalizing the channel open handshake. + /// Covers only a step of type either Ack or Confirm. + /// (Assumes that the channel open handshake was previously + /// initialized with Init & Try steps.) + /// + /// Returns `Ok` when both channel ends are in state `Open`. + /// Also returns `Ok` if the channel is undergoing a closing handshake. + /// + /// An `Err` can signal two cases: + /// - the attempted handshake step has failed, + /// - the attempted step may have finished successfully, but further + /// steps are necessary to finalize the channel open handshake. + /// In both `Err` cases, there should be retry calling this method. + fn do_chan_open_ack_confirm_step(&self) -> Result<(), ChannelError> { let src_channel_id = self .src_channel_id() .ok_or(ChannelError::MissingLocalChannelId)?; @@ -420,63 +446,119 @@ impl Channel { let a_channel = self .src_chain() .query_channel(&self.src_port_id(), src_channel_id, Height::zero()) - .map_err(|_| ChannelError::Failed("Failed to query source chain".into()))?; + .map_err(|_| { + ChannelError::Failed(format!( + "failed to query source chain {}", + self.src_chain().id() + )) + })?; let b_channel = self .dst_chain() .query_channel(&self.dst_port_id(), dst_channel_id, Height::zero()) - .map_err(|_| ChannelError::Failed("Failed to query destination chain".into()))?; + .map_err(|_| { + ChannelError::Failed(format!( + "failed to query destination chain {}", + self.dst_chain().id() + )) + })?; match (a_channel.state(), b_channel.state()) { + // Handle sending the Ack message to the source chain (State::Init, State::TryOpen) | (State::TryOpen, State::TryOpen) => { let event = self.flipped().build_chan_open_ack_and_send().map_err(|e| { - error!("Failed ChanAck {:?}: {}", self.a_side, e); + error!("failed ChanAck {:?}: {}", self.a_side, e); e })?; - info!("done {} => {:#?}\n", self.src_chain().id(), event); + info!( + "done with ChanAck step {} => {:#?}\n", + self.src_chain().id(), + event + ); + // One more step (confirm) left. + // Returning error signals that the caller should retry. + Err(ChannelError::PartialOpenHandshake( + *a_channel.state(), + *b_channel.state(), + )) } + + // Handle sending the Ack message to the destination chain + (State::TryOpen, State::Init) => { + let event = self.build_chan_open_ack_and_send().map_err(|e| { + error!("failed ChanAck {:?}: {}", self.b_side, e); + e + })?; + + info!( + "done with ChanAck step {} => {:#?}\n", + self.dst_chain().id(), + event + ); + // One more step (confirm) left. + // Returning error signals that the caller should retry. + Err(ChannelError::PartialOpenHandshake( + *a_channel.state(), + *b_channel.state(), + )) + } + + // Handle sending the Confirm message to the destination chain (State::Open, State::TryOpen) => { let event = self.build_chan_open_confirm_and_send().map_err(|e| { - error!("Failed ChanConfirm {:?}: {}", self.b_side, e); + error!("failed ChanConfirm {:?}: {}", self.b_side, e); e })?; info!("done {} => {:#?}\n", self.dst_chain().id(), event); + Ok(()) } + + // Send Confirm to the source chain (State::TryOpen, State::Open) => { - // Confirm to a_chain let event = self .flipped() .build_chan_open_confirm_and_send() .map_err(|e| { - error!("Failed ChanConfirm {:?}: {}", self.a_side, e); + error!("failed ChanConfirm {:?}: {}", self.a_side, e); e })?; - info!("done {} => {:#?}\n", self.src_chain().id(), event) + info!( + "finalized channel open handshake {} => {:#?}\n", + self.src_chain().id(), + event + ); + Ok(()) } (State::Open, State::Open) => { - info!("Channel handshake finished for {:#?}\n", self); + info!("channel handshake already finished for {:#?}\n", self); + Ok(()) } - _ => { /* TODO channel close */ } + // In all other conditions, return Ok, since the channel open handshake does not apply. + _ => Ok(()), } - - info!("successfully opened channel"); - Ok(()) } - fn do_channel_handshake_with_retry(&mut self) -> Result<(), ChannelError> { - retry_with_index(retry_strategy::default(), |_| self.do_channel_handshake()).map_err( - |err| { - error!("failed to open channel after {} retries", err); - ChannelError::Failed(format!( - "Failed to finish channel handshake in {} iterations for {:?}", - retry_count(&err), - self - )) - }, - )?; + /// Takes a partially open channel and finalizes the open handshake protocol. + /// + /// Pre-condition: the channel identifiers are already established on both ends + /// (i.e., the OpenInit and OpenTry steps have been fulfilled for this channel). + /// + /// Post-condition: the channel state is `Open` on both ends if successful. + fn do_chan_open_finalize_with_retry(&self) -> Result<(), ChannelError> { + retry_with_index(retry_strategy::default(), |_| { + self.do_chan_open_ack_confirm_step() + }) + .map_err(|err| { + error!("failed to open channel after {} retries", err); + ChannelError::Failed(format!( + "Failed to finish channel handshake in {} iterations for {:?}", + retry_count(&err), + self + )) + })?; Ok(()) } @@ -485,7 +567,7 @@ impl Channel { fn handshake(&mut self) -> Result<(), ChannelError> { self.do_chan_open_init_and_send_with_retry()?; self.do_chan_open_try_and_send_with_retry()?; - self.do_channel_handshake_with_retry() + self.do_chan_open_finalize_with_retry() } pub fn counterparty_state(&self) -> Result { diff --git a/relayer/src/link.rs b/relayer/src/link.rs index 685451c7c1..0c80fae822 100644 --- a/relayer/src/link.rs +++ b/relayer/src/link.rs @@ -22,19 +22,19 @@ use ibc::{ }, packet::{Packet, PacketMsgType, Sequence}, }, - ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId}, + ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortChannelId, PortId}, query::QueryTxRequest, signer::Signer, timestamp::ZERO_DURATION, tx_msg::Msg, Height, }; - use ibc_proto::ibc::core::channel::v1::{ QueryNextSequenceReceiveRequest, QueryPacketAcknowledgementsRequest, QueryPacketCommitmentsRequest, QueryUnreceivedAcksRequest, QueryUnreceivedPacketsRequest, }; +use crate::chain::counterparty::check_channel_counterparty; use crate::chain::handle::ChainHandle; use crate::channel::{Channel, ChannelError, ChannelSide}; use crate::connection::ConnectionError; @@ -53,6 +53,9 @@ pub enum LinkError { #[error("failed with underlying error: {0}")] Generic(#[from] Error), + #[error("link initialization failed during channel counterparty verification: {0}")] + Initialization(ChannelError), + #[error("failed to construct packet proofs for chain {0} with error: {1}")] PacketProofsConstructor(ChainId, Error), @@ -1634,6 +1637,21 @@ impl Link { ))); } + // Check that the counterparty details on the destination chain matches the source chain + check_channel_counterparty( + b_chain.clone(), + &PortChannelId { + channel_id: b_channel_id.clone(), + port_id: a_channel.counterparty().port_id.clone(), + }, + &PortChannelId { + channel_id: a_channel_id.clone(), + port_id: opts.src_port_id.clone(), + }, + ) + .map_err(LinkError::Initialization)?; + + // Check the underlying connection let a_connection_id = a_channel.connection_hops()[0].clone(); let a_connection = a_chain.query_connection(&a_connection_id, Height::zero())?; diff --git a/relayer/src/transfer.rs b/relayer/src/transfer.rs index 87f1f9beb1..8d1830d65c 100644 --- a/relayer/src/transfer.rs +++ b/relayer/src/transfer.rs @@ -1,17 +1,18 @@ +use std::time::Duration; + use thiserror::Error; use tracing::error; use ibc::application::ics20_fungible_token_transfer::msgs::transfer::MsgTransfer; use ibc::events::IbcEvent; use ibc::ics24_host::identifier::{ChainId, ChannelId, PortId}; +use ibc::timestamp::Timestamp; use ibc::tx_msg::Msg; +use ibc::Height; -use crate::chain::{Chain, CosmosSdkChain}; +use crate::chain::handle::ChainHandle; use crate::config::ChainConfig; use crate::error::Error; -use ibc::timestamp::Timestamp; -use ibc::Height; -use std::time::Duration; #[derive(Debug, Error)] pub enum PacketError { @@ -45,8 +46,8 @@ pub struct TransferOptions { } pub fn build_and_send_transfer_messages( - mut packet_src_chain: CosmosSdkChain, // the chain whose account is debited - mut packet_dst_chain: CosmosSdkChain, // the chain where the transfer is sent + packet_src_chain: Box, // the chain whose account is debited + packet_dst_chain: Box, // the chain whose account eventually gets credited opts: TransferOptions, ) -> Result, PacketError> { let receiver = match &opts.receiver { @@ -90,7 +91,7 @@ pub fn build_and_send_transfer_messages( let events = packet_src_chain .send_msgs(msgs) - .map_err(|e| PacketError::Submit(packet_src_chain.id().clone(), e))?; + .map_err(|e| PacketError::Submit(packet_src_chain.id(), e))?; // Check if the chain rejected the transaction let result = events