Disallow creation of new Tendermint client state instance with a froz…

…en height (#602)

.changelog/unreleased/bug/178-disallow-frozen-tm-client-state-creation.md

@@ -0,0 +1,2 @@
+- Disallow creation of new Tendermint client state instance with a frozen height
+ ([#178](https://github.com/cosmos/ibc-rs/issues/178))

crates/ibc/src/clients/ics07_tendermint/client_state.rs

@@ -82,7 +82,6 @@ impl ClientState {
  proof_specs: ProofSpecs,
  upgrade_path: Vec<String>,
  allow_update: AllowUpdate,
- frozen_height: Option<Height>,
  ) -> Result<ClientState, Error> {
  if chain_id.as_str().len() > MaxChainIdLen {
  return Err(Error::ChainIdTooLong {
@@ -172,7 +171,7 @@ impl ClientState {
  proof_specs,
  upgrade_path,
  allow_update,
- frozen_height,
+ frozen_height: None,
  verifier: ProdVerifier::default(),
  })
  }
@@ -704,9 +703,6 @@ impl Ics2ClientState for ClientState {
  let upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?;
  let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?;
 
- // Frozen height is set to None fo the new client state
- let new_frozen_height = None;
-
  // Construct new client state and consensus state relayer chosen client
  // parameters are ignored. All chain-chosen parameters come from
  // committed client, all client-chosen parameters come from current
@@ -721,7 +717,6 @@ impl Ics2ClientState for ClientState {
  upgraded_tm_client_state.proof_specs,
  upgraded_tm_client_state.upgrade_path,
  self.allow_update,
- new_frozen_height,
  )?;
 
  // The new consensus state is merely used as a trusted kernel against
@@ -860,9 +855,13 @@ impl TryFrom<RawTmClientState> for ClientState {
  // In `RawClientState`, a `frozen_height` of `0` means "not frozen".
  // See:
  // https://github.com/cosmos/ibc-go/blob/8422d0c4c35ef970539466c5bdec1cd27369bab3/modules/light-clients/07-tendermint/types/client_state.go#L74
- let frozen_height = raw
+ if raw
  .frozen_height
- .and_then(|raw_height| raw_height.try_into().ok());
+ .and_then(|h| Height::try_from(h).ok())
+ .is_some()
+ {
+ return Err(Error::FrozenHeightNotAllowed);
+ }
 
  // We use set this deprecated field just so that we can properly convert
  // it back in its raw form
@@ -882,7 +881,6 @@ impl TryFrom<RawTmClientState> for ClientState {
  raw.proof_specs.into(),
  raw.upgrade_path,
  allow_update,
- frozen_height,
  )?;
 
  Ok(client_state)
@@ -951,16 +949,20 @@ impl From<ClientState> for Any {
 
 #[cfg(test)]
 mod tests {
+ use crate::clients::ics07_tendermint::header::test_util::get_dummy_tendermint_header;
  use crate::prelude::*;
  use crate::Height;
  use core::time::Duration;
  use test_log::test;
 
+ use ibc_proto::google::protobuf::Any;
+ use ibc_proto::ibc::core::client::v1::Height as RawHeight;
  use ibc_proto::ics23::ProofSpec as Ics23ProofSpec;
 
  use crate::clients::ics07_tendermint::client_state::{
  AllowUpdate, ClientState as TmClientState,
  };
+ use crate::clients::ics07_tendermint::error::Error;
  use crate::core::ics02_client::client_state::ClientState;
  use crate::core::ics02_client::trust_threshold::TrustThreshold;
  use crate::core::ics23_commitment::specs::ProofSpecs;
@@ -1133,7 +1135,6 @@ mod tests {
  p.proof_specs,
  p.upgrade_path,
  p.allow_update,
- None,
  );
 
  assert_eq!(
@@ -1199,7 +1200,6 @@ mod tests {
  p.proof_specs,
  p.upgrade_path,
  p.allow_update,
- None,
  )
  .unwrap();
  let client_state = match test.setup {
@@ -1218,6 +1218,48 @@ mod tests {
  );
  }
  }
+
+ #[test]
+ fn tm_client_state_conversions_healthy() {
+ // check client state creation path from a proto type
+ let tm_client_state_from_raw = TmClientState::new_dummy_from_raw(RawHeight {
+ revision_number: 0,
+ revision_height: 0,
+ });
+ assert!(tm_client_state_from_raw.is_ok());
+
+ let any_from_tm_client_state =
+ Any::from(tm_client_state_from_raw.as_ref().unwrap().clone());
+ let tm_client_state_from_any = TmClientState::try_from(any_from_tm_client_state);
+ assert!(tm_client_state_from_any.is_ok());
+ assert_eq!(
+ tm_client_state_from_raw.unwrap(),
+ tm_client_state_from_any.unwrap()
+ );
+
+ // check client state creation path from a tendermint header
+ let tm_header = get_dummy_tendermint_header();
+ let tm_client_state_from_header = TmClientState::new_dummy_from_header(tm_header);
+ let any_from_header = Any::from(tm_client_state_from_header.clone());
+ let tm_client_state_from_any = TmClientState::try_from(any_from_header);
+ assert!(tm_client_state_from_any.is_ok());
+ assert_eq!(
+ tm_client_state_from_header,
+ tm_client_state_from_any.unwrap()
+ );
+ }
+
+ #[test]
+ fn tm_client_state_malformed_with_frozen_height() {
+ let tm_client_state_from_raw = TmClientState::new_dummy_from_raw(RawHeight {
+ revision_number: 0,
+ revision_height: 10,
+ });
+ match tm_client_state_from_raw {
+ Err(Error::FrozenHeightNotAllowed) => {}
+ _ => panic!("Expected to fail with FrozenHeightNotAllowed error"),
+ }
+ }
 }
 
 #[cfg(all(test, feature = "serde"))]
@@ -1249,29 +1291,58 @@ pub mod test_util {
  use tendermint::block::Header;
 
  use crate::clients::ics07_tendermint::client_state::{AllowUpdate, ClientState};
+ use crate::clients::ics07_tendermint::error::Error;
  use crate::core::ics02_client::height::Height;
+ use crate::core::ics23_commitment::specs::ProofSpecs;
  use crate::core::ics24_host::identifier::ChainId;
+ use ibc_proto::ibc::core::client::v1::Height as RawHeight;
+ use ibc_proto::ibc::lightclients::tendermint::v1::{ClientState as RawTmClientState, Fraction};
 
- pub fn get_dummy_tendermint_client_state(tm_header: Header) -> ClientState {
- ClientState::new(
- ChainId::from(tm_header.chain_id.clone()),
- Default::default(),
- Duration::from_secs(64000),
- Duration::from_secs(128000),
- Duration::from_millis(3000),
- Height::new(
- ChainId::chain_version(tm_header.chain_id.as_str()),
- u64::from(tm_header.height),
+ impl ClientState {
+ pub fn new_dummy_from_raw(frozen_height: RawHeight) -> Result<Self, Error> {
+ ClientState::try_from(get_dummy_raw_tm_client_state(frozen_height))
+ }
+
+ pub fn new_dummy_from_header(tm_header: Header) -> ClientState {
+ ClientState::new(
+ tm_header.chain_id.clone().into(),
+ Default::default(),
+ Duration::from_secs(64000),
+ Duration::from_secs(128000),
+ Duration::from_millis(3000),
+ Height::new(
+ ChainId::chain_version(tm_header.chain_id.as_str()),
+ u64::from(tm_header.height),
+ )
+ .unwrap(),
+ Default::default(),
+ Default::default(),
+ AllowUpdate {
+ after_expiry: false,
+ after_misbehaviour: false,
+ },
  )
- .unwrap(),
- Default::default(),
- Default::default(),
- AllowUpdate {
- after_expiry: false,
- after_misbehaviour: false,
- },
- None,
- )
- .unwrap()
+ .unwrap()
+ }
+ }
+
+ pub fn get_dummy_raw_tm_client_state(frozen_height: RawHeight) -> RawTmClientState {
+ #[allow(deprecated)]
+ RawTmClientState {
+ chain_id: ChainId::new("ibc".to_string(), 0).to_string(),
+ trust_level: Some(Fraction {
+ numerator: 1,
+ denominator: 3,
+ }),
+ trusting_period: Some(Duration::from_secs(64000).into()),
+ unbonding_period: Some(Duration::from_secs(128000).into()),
+ max_clock_drift: Some(Duration::from_millis(3000).into()),
+ latest_height: Some(Height::new(0, 10).unwrap().into()),
+ proof_specs: ProofSpecs::default().into(),
+ upgrade_path: Default::default(),
+ frozen_height: Some(frozen_height),
+ allow_update_after_expiry: false,
+ allow_update_after_misbehaviour: false,
+ }
  }
 }

crates/ibc/src/clients/ics07_tendermint/error.rs

@@ -67,6 +67,8 @@ pub enum Error {
  HeaderTimestampTooLow { actual: String, min: String },
  /// header revision height = `{height}` is invalid
  InvalidHeaderHeight { height: u64 },
+ /// Disallowed to create a new client with a frozen height
+ FrozenHeightNotAllowed,
  /// the header's current/trusted revision number (`{current_revision}`) and the update's revision number (`{update_revision}`) should be the same
  MismatchedRevisions {
  current_revision: u64,

crates/ibc/src/core/ics02_client/handler/create_client.rs

@@ -113,23 +113,18 @@ where
 
 #[cfg(test)]
 mod tests {
- use crate::clients::ics07_tendermint::client_type as tm_client_type;
- use crate::core::ics02_client::handler::create_client::{execute, validate};
- use crate::core::ValidationContext;
  use crate::prelude::*;
 
- use core::time::Duration;
  use test_log::test;
 
- use crate::clients::ics07_tendermint::client_state::{
- AllowUpdate, ClientState as TmClientState,
- };
+ use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState;
+ use crate::clients::ics07_tendermint::client_type as tm_client_type;
  use crate::clients::ics07_tendermint::consensus_state::ConsensusState as TmConsensusState;
  use crate::clients::ics07_tendermint::header::test_util::get_dummy_tendermint_header;
+ use crate::core::ics02_client::handler::create_client::{execute, validate};
  use crate::core::ics02_client::msgs::create_client::MsgCreateClient;
- use crate::core::ics02_client::trust_threshold::TrustThreshold;
- use crate::core::ics23_commitment::specs::ProofSpecs;
  use crate::core::ics24_host::identifier::ClientId;
+ use crate::core::ValidationContext;
  use crate::mock::client_state::{client_type as mock_client_type, MockClientState};
  use crate::mock::consensus_state::MockConsensusState;
  use crate::mock::context::MockContext;
@@ -177,23 +172,7 @@ mod tests {
 
  let tm_header = get_dummy_tendermint_header();
 
- let tm_client_state = TmClientState::new(
- tm_header.chain_id.clone().into(),
- TrustThreshold::ONE_THIRD,
- Duration::from_secs(64000),
- Duration::from_secs(128000),
- Duration::from_millis(3000),
- Height::new(0, u64::from(tm_header.height)).unwrap(),
- ProofSpecs::default(),
- vec![],
- AllowUpdate {
- after_expiry: false,
- after_misbehaviour: false,
- },
- None,
- )
- .unwrap()
- .into();
+ let tm_client_state = TmClientState::new_dummy_from_header(tm_header.clone()).into();
 
  let client_type = tm_client_type();
 

crates/ibc/src/core/ics02_client/msgs/create_client.rs

@@ -75,7 +75,7 @@ mod tests {
 
  use ibc_proto::ibc::core::client::v1::MsgCreateClient as RawMsgCreateClient;
 
- use crate::clients::ics07_tendermint::client_state::test_util::get_dummy_tendermint_client_state;
+ use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState;
  use crate::clients::ics07_tendermint::consensus_state::ConsensusState as TmConsensusState;
  use crate::clients::ics07_tendermint::header::test_util::get_dummy_tendermint_header;
  use crate::core::ics02_client::msgs::create_client::MsgCreateClient;
@@ -86,7 +86,7 @@ mod tests {
  let signer = get_dummy_account_id();
 
  let tm_header = get_dummy_tendermint_header();
- let tm_client_state = get_dummy_tendermint_client_state(tm_header.clone()).into();
+ let tm_client_state = TmClientState::new_dummy_from_header(tm_header.clone()).into();
 
  let msg = MsgCreateClient::new(
  tm_client_state,

crates/ibc/src/mock/context.rs

@@ -25,7 +25,6 @@ use subtle_encoding::bech32;
 use ibc_proto::google::protobuf::Any;
 use tracing::debug;
 
-use crate::clients::ics07_tendermint::client_state::test_util::get_dummy_tendermint_client_state;
 use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState;
 use crate::core::context::ContextError;
 use crate::core::context::Router;
@@ -245,7 +244,7 @@ impl MockContext {
  );
 
  let client_state =
- get_dummy_tendermint_client_state(light_block.header().clone()).into_box();
+ TmClientState::new_dummy_from_header(light_block.header().clone()).into_box();
 
  // Return the tuple.
  (Some(client_state), light_block.into())
@@ -312,7 +311,7 @@ impl MockContext {
  HostBlock::generate_tm_block(client_chain_id, cs_height.revision_height(), now);
 
  let client_state =
- get_dummy_tendermint_client_state(light_block.header().clone()).into_box();
+ TmClientState::new_dummy_from_header(light_block.header().clone()).into_box();
 
  // Return the tuple.
  (Some(client_state), light_block.into())