fix: prevent expired client creation (#1294)

* fix: prevent expired client creation * chore: add changelog * chore: apply review comment Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Farhad Shabani <[email protected]> * imp: associate consensus_state_status signatures with ConsensusState * Fix a word in doc comment --------- Signed-off-by: Farhad Shabani <[email protected]> Co-authored-by: Sean Chen <[email protected]>
cosmos · Aug 7, 2024 · 7ea5cae · 7ea5cae
1 parent ccf6ce6
commit 7ea5cae
Show file tree

Hide file tree

Showing 15 changed files with 391 additions and 124 deletions.
diff --git a/.changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md b/.changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md
@@ -0,0 +1,3 @@
+- [ibc-core-client] Prevent expired client creations by ensuring that consensus
+  state's timestamp is within the trusting period.
+- ([\#1088](https://github.com/cosmos/ibc-rs/issues/1088))
diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs
@@ -1,8 +1,10 @@
+use core::time::Duration;
+
 use ibc_client_tendermint_types::{client_type as tm_client_type, ClientState as ClientStateType};
 use ibc_core_client::context::client_state::ClientStateCommon;
 use ibc_core_client::context::consensus_state::ConsensusState;
 use ibc_core_client::types::error::{ClientError, UpgradeClientError};
-use ibc_core_client::types::Height;
+use ibc_core_client::types::{Height, Status};
 use ibc_core_commitment_types::commitment::{
     CommitmentPrefix, CommitmentProofBytes, CommitmentRoot,
 };
@@ -14,14 +16,22 @@ use ibc_core_host::types::identifiers::ClientType;
 use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath};
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
-use ibc_primitives::ToVec;
+use ibc_primitives::{Timestamp, ToVec};
 
 use super::ClientState;
 use crate::consensus_state::ConsensusState as TmConsensusState;
 
 impl ClientStateCommon for ClientState {
-    fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError> {
-        verify_consensus_state(consensus_state)
+    fn verify_consensus_state(
+        &self,
+        consensus_state: Any,
+        host_timestamp: &Timestamp,
+    ) -> Result<(), ClientError> {
+        verify_consensus_state(
+            consensus_state,
+            host_timestamp,
+            self.inner().trusting_period,
+        )
     }
 
     fn client_type(&self) -> ClientType {
@@ -111,7 +121,11 @@ impl ClientStateCommon for ClientState {
 /// Note that this function is typically implemented as part of the
 /// [`ClientStateCommon`] trait, but has been made a standalone function
 /// in order to make the ClientState APIs more flexible.
-pub fn verify_consensus_state(consensus_state: Any) -> Result<(), ClientError> {
+pub fn verify_consensus_state(
+    consensus_state: Any,
+    host_timestamp: &Timestamp,
+    trusting_period: Duration,
+) -> Result<(), ClientError> {
     let tm_consensus_state = TmConsensusState::try_from(consensus_state)?;
 
     if tm_consensus_state.root().is_empty() {
@@ -120,9 +134,45 @@ pub fn verify_consensus_state(consensus_state: Any) -> Result<(), ClientError> {
         });
     };
 
+    if consensus_state_status(&tm_consensus_state, host_timestamp, trusting_period)?.is_expired() {
+        return Err(ClientError::ClientNotActive {
+            status: Status::Expired,
+        });
+    }
+
     Ok(())
 }
 
+/// Determines the `Status`, whether it is `Active` or `Expired`, of a consensus
+/// state, using its timestamp, the host's timestamp, and the trusting period.
+pub fn consensus_state_status<CS: ConsensusState>(
+    consensus_state: &CS,
+    host_timestamp: &Timestamp,
+    trusting_period: Duration,
+) -> Result<Status, ClientError> {
+    if !host_timestamp.is_set() {
+        return Err(ClientError::Other {
+            description: "host timestamp is none".into(),
+        });
+    }
+
+    // Note: if the `duration_since()` is `None`, indicating that the latest
+    // consensus state is in the future, then we don't consider the client
+    // to be expired.
+    if let Some(elapsed_since_latest_consensus_state) =
+        host_timestamp.duration_since(&consensus_state.timestamp())
+    {
+        // Note: The equality is considered as expired to stay consistent with
+        // the check in tendermint-rs, where a header at `trusted_header_time +
+        // trusting_period` is considered expired.
+        if elapsed_since_latest_consensus_state >= trusting_period {
+            return Ok(Status::Expired);
+        }
+    }
+
+    Ok(Status::Active)
+}
+
 /// Validate the given proof height against the client state's latest height, returning
 /// an error if the proof height is greater than the latest height of the client state.
 ///

diff --git a/ibc-clients/ics07-tendermint/src/client_state/validation.rs b/ibc-clients/ics07-tendermint/src/client_state/validation.rs
@@ -15,7 +15,10 @@ use tendermint::crypto::Sha256 as Sha256Trait;
 use tendermint::merkle::MerkleHash;
 use tendermint_light_client_verifier::{ProdVerifier, Verifier};
 
-use super::{check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update, ClientState};
+use super::{
+    check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update,
+    consensus_state_status, ClientState,
+};
 use crate::client_state::{verify_header, verify_misbehaviour};
 
 impl<V> ClientStateValidation<V> for ClientState
@@ -218,18 +221,13 @@ where
     // to be expired.
     let now = ctx.host_timestamp()?;
 
-    if let Some(elapsed_since_latest_consensus_state) =
-        now.duration_since(&latest_consensus_state.timestamp().into())
-    {
-        // Note: The equality is considered as expired to stay consistent with
-        // the check in tendermint-rs, where a header at `trusted_header_time +
-        // trusting_period` is considered expired.
-        if elapsed_since_latest_consensus_state >= client_state.trusting_period {
-            return Ok(Status::Expired);
-        }
-    }
+    let status = consensus_state_status(
+        &latest_consensus_state.into(),
+        &now,
+        client_state.trusting_period,
+    )?;
 
-    Ok(Status::Active)
+    Ok(status)
 }
 
 /// Check that the subject and substitute client states match as part of

diff --git a/ibc-core/ics02-client/context/src/client_state.rs b/ibc-core/ics02-client/context/src/client_state.rs
@@ -9,6 +9,7 @@ use ibc_core_host_types::identifiers::{ClientId, ClientType};
 use ibc_core_host_types::path::{Path, PathBytes};
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
+use ibc_primitives::Timestamp;
 
 use crate::context::{ClientExecutionContext, ClientValidationContext};
 use crate::Convertible;
@@ -22,7 +23,11 @@ pub trait ClientStateCommon: Convertible<Any> {
     ///
     /// Notably, an implementation should verify that it can properly
     /// deserialize the object into the expected format.
-    fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError>;
+    fn verify_consensus_state(
+        &self,
+        consensus_state: Any,
+        host_timestamp: &Timestamp,
+    ) -> Result<(), ClientError>;
 
     /// Type of client associated with this state (eg. Tendermint)
     fn client_type(&self) -> ClientType;

diff --git a/ibc-core/ics02-client/src/handler/create_client.rs b/ibc-core/ics02-client/src/handler/create_client.rs
@@ -41,7 +41,9 @@ where
         .into());
     };
 
-    client_state.verify_consensus_state(consensus_state)?;
+    let host_timestamp = ctx.host_timestamp()?;
+
+    client_state.verify_consensus_state(consensus_state, &host_timestamp)?;
 
     if client_val_ctx.client_state(&client_id).is_ok() {
         return Err(ClientError::ClientStateAlreadyExists { client_id }.into());

diff --git a/ibc-derive/src/client_state/traits/client_state_common.rs b/ibc-derive/src/client_state/traits/client_state_common.rs
@@ -14,7 +14,7 @@ pub(crate) fn impl_ClientStateCommon(
     let verify_consensus_state_impl = delegate_call_in_match(
         client_state_enum_name,
         enum_variants.iter(),
-        quote! { verify_consensus_state(cs, consensus_state) },
+        quote! { verify_consensus_state(cs, consensus_state, &host_timestamp) },
         imports,
     );
     let client_type_impl = delegate_call_in_match(
@@ -82,12 +82,13 @@ pub(crate) fn impl_ClientStateCommon(
     let ClientType = imports.client_type();
     let ClientError = imports.client_error();
     let Height = imports.height();
+    let Timestamp = imports.timestamp();
     let Path = imports.path();
     let PathBytes = imports.path_bytes();
 
     quote! {
         impl #ClientStateCommon for #HostClientState {
-            fn verify_consensus_state(&self, consensus_state: #Any) -> Result<(), #ClientError> {
+            fn verify_consensus_state(&self, consensus_state: #Any, host_timestamp: &#Timestamp) -> Result<(), #ClientError> {
                 match self {
                     #(#verify_consensus_state_impl),*
                 }

diff --git a/ibc-testkit/src/data/json/signed_header.json → .../src/data/json/expired_signed_header.json b/ibc-testkit/src/data/json/signed_header.json → .../src/data/json/expired_signed_header.json
diff --git a/ibc-testkit/src/data/json/valid_signed_header.json b/ibc-testkit/src/data/json/valid_signed_header.json
@@ -0,0 +1,64 @@
+{
+  "header": {
+    "version": {
+      "block": "0",
+      "app": "0"
+    },
+    "chain_id": "test-chain-1",
+    "height": "20",
+    "time": "2030-11-02T15:04:10Z",
+    "last_block_id": {
+      "hash": "15F15EF50BDE2018F4B129A827F90C18222C757770C8295EB8EE7BF50E761BC0",
+      "part_set_header": {
+        "total": 1,
+        "hash": "077E16D720F9AA656EBFD7F3FB31A4A35E1F2F4EBEBB123642BED45535D88AD5"
+      }
+    },
+    "last_commit_hash": "D5439DD65D45EF1E51412691BCF2F6741D48AC1325572E08D48BD72F80669E70",
+    "data_hash": "",
+    "validators_hash": "26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564",
+    "next_validators_hash": "26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564",
+    "consensus_hash": "048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F",
+    "app_hash": "6170705F68617368",
+    "last_results_hash": "",
+    "evidence_hash": "",
+    "proposer_address": "026CC7B6F3E62F789DBECEC59766888B5464737D"
+  },
+  "commit": {
+    "height": "20",
+    "round": 1,
+    "block_id": {
+      "hash": "5E87BD3A35C62D06138273453AF49C7728E4F8FB4CAFB0784F4816D5052AA349",
+      "part_set_header": {
+        "total": 1,
+        "hash": "DC797E9C450AE5FD0D8000E31672BE3EE97B6C0A3BD69239187F75C00C39D72B"
+      }
+    },
+    "signatures": [
+      {
+        "block_id_flag": 3,
+        "validator_address": "01F527D77D3FFCC4FCFF2DDC2952EEA5414F2A33",
+        "timestamp": "2019-11-02T15:04:15Z",
+        "signature": "SMBYJYr7yHbqmScg7jUhSGyis/gOMvwkO4mlEmKt9NKJ+aZgETFgIIFnExCju0VidMyy9mUwMQ/b+EoVL1NwAQ=="
+      },
+      {
+        "block_id_flag": 2,
+        "validator_address": "026CC7B6F3E62F789DBECEC59766888B5464737D",
+        "timestamp": "2019-11-02T15:04:15Z",
+        "signature": "Bn8cV8WlqGjezZcFgQey0ikBshIHw56jpuzWssO2/0HOpMC1Mr2mJ5mxEd4mpsQ4PwenpGtrLJv11PFK6aJLBw=="
+      },
+      {
+        "block_id_flag": 2,
+        "validator_address": "03A238BCAF7D1626DFE8A4AFB9448D00B7A3D2E2",
+        "timestamp": "2019-11-02T15:04:15Z",
+        "signature": "mhgrDo+fV5GaS34CZn//zgNx24H21Ila9io5HPgxeEWbG24eGLp0GAAlGPryT9bdv5LXPUq8wZpUHxAcyhvZAQ=="
+      },
+      {
+        "block_id_flag": 2,
+        "validator_address": "03EC0413849A3311A5341E7A69D6C544E9A30310",
+        "timestamp": "2019-11-02T15:04:15Z",
+        "signature": "+g5Rhm8Utshm0SN2gxKeRrqsJyUO32BxDqTVu0wHzaJz0cFJWZxA2JaYxaf6EzuQULkwp+oh6u3xboy/e5wHAg=="
+      }
+    ]
+  }
+}
diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs
@@ -112,12 +112,24 @@ impl ClientStateConfig {
 }
 
 #[cfg(feature = "serde")]
-pub fn dummy_tendermint_header() -> tendermint::block::Header {
+pub fn dummy_valid_tendermint_header() -> tendermint::block::Header {
     use tendermint::block::signed_header::SignedHeader;
 
     serde_json::from_str::<SignedHeader>(include_str!(concat!(
         env!("CARGO_MANIFEST_DIR"),
-        "/src/data/json/signed_header.json"
+        "/src/data/json/valid_signed_header.json"
+    )))
+    .expect("Never fails")
+    .header
+}
+
+#[cfg(feature = "serde")]
+pub fn dummy_expired_tendermint_header() -> tendermint::block::Header {
+    use tendermint::block::signed_header::SignedHeader;
+
+    serde_json::from_str::<SignedHeader>(include_str!(concat!(
+        env!("CARGO_MANIFEST_DIR"),
+        "/src/data/json/expired_signed_header.json"
     )))
     .expect("Never fails")
     .header
@@ -147,7 +159,7 @@ pub fn dummy_ics07_header() -> Header {
     // Build a SignedHeader from a JSON file.
     let shdr = serde_json::from_str::<SignedHeader>(include_str!(concat!(
         env!("CARGO_MANIFEST_DIR"),
-        "/src/data/json/signed_header.json"
+        "/src/data/json/valid_signed_header.json"
     )))
     .expect("Never fails");
 
@@ -222,7 +234,7 @@ mod tests {
     #[test]
     fn tm_client_state_from_header_healthy() {
         // check client state creation path from a tendermint header
-        let tm_header = dummy_tendermint_header();
+        let tm_header = dummy_valid_tendermint_header();
         let tm_client_state_from_header = dummy_tm_client_state_from_header(tm_header);
         let any_from_header = Any::from(tm_client_state_from_header.clone());
         let tm_client_state_from_any = ClientStateType::try_from(any_from_header);

diff --git a/ibc-testkit/src/fixtures/core/client/msg_create_client.rs b/ibc-testkit/src/fixtures/core/client/msg_create_client.rs
@@ -3,13 +3,13 @@ use ibc::core::client::types::proto::v1::MsgCreateClient as RawMsgCreateClient;
 use ibc::primitives::proto::Any;
 
 use crate::fixtures::clients::tendermint::{
-    dummy_tendermint_header, dummy_tm_client_state_from_header,
+    dummy_tm_client_state_from_header, dummy_valid_tendermint_header,
 };
 use crate::fixtures::core::signer::dummy_bech32_account;
 
 /// Returns a dummy `RawMsgCreateClient`, for testing purposes only!
 pub fn dummy_raw_msg_create_client() -> RawMsgCreateClient {
-    let tm_header = dummy_tendermint_header();
+    let tm_header = dummy_valid_tendermint_header();
 
     let tm_client_state = dummy_tm_client_state_from_header(tm_header.clone());
 

diff --git a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs
@@ -1,6 +1,7 @@
 use core::str::FromStr;
 use core::time::Duration;
 
+use ibc::clients::tendermint::client_state::consensus_state_status;
 use ibc::core::client::context::prelude::*;
 use ibc::core::client::types::error::{ClientError, UpgradeClientError};
 use ibc::core::client::types::{Height, Status};
@@ -160,8 +161,29 @@ pub trait MockClientContext {
 }
 
 impl ClientStateCommon for MockClientState {
-    fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError> {
-        let _mock_consensus_state = MockConsensusState::try_from(consensus_state)?;
+    fn verify_consensus_state(
+        &self,
+        consensus_state: Any,
+        host_timestamp: &Timestamp,
+    ) -> Result<(), ClientError> {
+        let mock_consensus_state = MockConsensusState::try_from(consensus_state)?;
+
+        let consensus_state_timestamp = mock_consensus_state.timestamp();
+
+        if !consensus_state_timestamp.is_set() {
+            return Err(ClientError::InvalidConsensusStateTimestamp {
+                time1: *host_timestamp,
+                time2: consensus_state_timestamp,
+            });
+        }
+
+        if consensus_state_status(&mock_consensus_state, host_timestamp, self.trusting_period)?
+            .is_expired()
+        {
+            return Err(ClientError::ClientNotActive {
+                status: Status::Expired,
+            });
+        }
 
         Ok(())
     }

diff --git a/ibc-testkit/src/testapp/ibc/core/types.rs b/ibc-testkit/src/testapp/ibc/core/types.rs
@@ -188,7 +188,10 @@ where
         ibc_store.store.commit().expect("no error");
         ibc_store.store_host_consensus_state(
             ibc_store.store.current_height(),
-            MockHeader::default().into_consensus_state().into(),
+            MockHeader::default()
+                .with_current_timestamp()
+                .into_consensus_state()
+                .into(),
         );
         ibc_store.store_ibc_commitment_proof(
             ibc_store.store.current_height(),