From 707d032dac76c3e4b2c70a9dd4c508d03d67d62a Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 12 Apr 2023 13:37:31 -0700 Subject: [PATCH] Disallow creation of new Tendermint client state instance with a frozen height (#602) --- ...isallow-frozen-tm-client-state-creation.md | 2 + .../clients/ics07_tendermint/client_state.rs | 133 ++++++++++++++---- .../ibc/src/clients/ics07_tendermint/error.rs | 2 + .../ics02_client/handler/create_client.rs | 31 +--- .../core/ics02_client/msgs/create_client.rs | 4 +- crates/ibc/src/mock/context.rs | 5 +- 6 files changed, 115 insertions(+), 62 deletions(-) create mode 100644 .changelog/unreleased/bug/178-disallow-frozen-tm-client-state-creation.md diff --git a/.changelog/unreleased/bug/178-disallow-frozen-tm-client-state-creation.md b/.changelog/unreleased/bug/178-disallow-frozen-tm-client-state-creation.md new file mode 100644 index 000000000..2697a05cf --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index 1febe6022..9dadd0306 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -82,7 +82,6 @@ impl ClientState { proof_specs: ProofSpecs, upgrade_path: Vec, allow_update: AllowUpdate, - frozen_height: Option, ) -> Result { 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 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 for ClientState { raw.proof_specs.into(), raw.upgrade_path, allow_update, - frozen_height, )?; Ok(client_state) @@ -951,16 +949,20 @@ impl From 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 { + 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, + } } } diff --git a/crates/ibc/src/clients/ics07_tendermint/error.rs b/crates/ibc/src/clients/ics07_tendermint/error.rs index 46a5689ca..c16bcfa74 100644 --- a/crates/ibc/src/clients/ics07_tendermint/error.rs +++ b/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, diff --git a/crates/ibc/src/core/ics02_client/handler/create_client.rs b/crates/ibc/src/core/ics02_client/handler/create_client.rs index 63db8a6d2..e255b8a5b 100644 --- a/crates/ibc/src/core/ics02_client/handler/create_client.rs +++ b/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(); diff --git a/crates/ibc/src/core/ics02_client/msgs/create_client.rs b/crates/ibc/src/core/ics02_client/msgs/create_client.rs index 3f5b8e238..0bfa2a7d1 100644 --- a/crates/ibc/src/core/ics02_client/msgs/create_client.rs +++ b/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, diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index c7abb332a..321977f9b 100644 --- a/crates/ibc/src/mock/context.rs +++ b/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())