fix(ibc-clients/tendermint): disallow empty proof-specs and empty depth range in proof-specs

.changelog/unreleased/bug-fixes/1100-fix-tendermint-empty-proof-specs-check.md

@@ -0,0 +1,2 @@
+- [ibc-client-tendermint-types] Validate the tendermint `ClientState` against empty proof-specs and empty proof-specs depth range.
+  ([\#1100](https://github.com/cosmos/ibc-rs/issues/1100))

ibc-clients/ics07-tendermint/types/src/client_state.rs

@@ -180,11 +180,9 @@ impl ClientState {
             });
         }
 
-        // Disallow empty proof-specs
-        if self.proof_specs.is_empty() {
-            return Err(Error::Validation {
-                reason: "ClientState proof-specs cannot be empty".to_string(),
-            });
+        // Sanity checks on client proof specs
+        if let Err(e) = self.proof_specs.validate() {
+            return Err(Error::InvalidProofSpec(e));
         }
 
         // `upgrade_path` itself may be empty, but if not then each key must be non-empty
@@ -421,7 +419,9 @@ pub(crate) mod serde_tests {
 
 #[cfg(test)]
 mod tests {
-    use ibc_core_commitment_types::proto::ics23::ProofSpec as Ics23ProofSpec;
+    use ibc_core_commitment_types::proto::ics23::{
+        HashOp, InnerSpec, LeafOp, LengthOp, ProofSpec as Ics23ProofSpec,
+    };
 
     use super::*;
 
@@ -462,6 +462,30 @@ mod tests {
             want_pass: bool,
         }
 
+        let leaf = LeafOp {
+            hash: HashOp::Sha256.into(),
+            prehash_key: 0,
+            prehash_value: HashOp::Sha256.into(),
+            length: LengthOp::VarProto.into(),
+            prefix: vec![0_u8],
+        };
+        let inner = InnerSpec {
+            child_order: vec![0, 1],
+            min_prefix_length: 1,
+            max_prefix_length: 1,
+            child_size: 32,
+            empty_child: vec![],
+            hash: HashOp::Sha256.into(),
+        };
+
+        let empty_depth_range_proof_specs: Vec<Ics23ProofSpec> = vec![Ics23ProofSpec {
+            leaf_spec: Some(leaf),
+            inner_spec: Some(inner),
+            min_depth: 2,
+            max_depth: 1,
+            prehash_key_before_comparison: false,
+        }];
+
         let tests: Vec<Test> = vec![
             Test {
                 name: "Valid parameters".to_string(),
@@ -570,7 +594,15 @@ mod tests {
                 name: "Invalid (empty) proof specs".to_string(),
                 params: ClientStateParams {
                     proof_specs: ProofSpecs::from(Vec::<Ics23ProofSpec>::new()),
-                    ..default_params
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid (empty) proof specs depth range".to_string(),
+                params: ClientStateParams {
+                    proof_specs: ProofSpecs::from(empty_depth_range_proof_specs),
+                    ..default_params.clone()
                 },
                 want_pass: false,
             },

ibc-clients/ics07-tendermint/types/src/error.rs

@@ -5,6 +5,7 @@ use core::time::Duration;
 use displaydoc::Display;
 use ibc_core_client_types::error::ClientError;
 use ibc_core_client_types::Height;
+use ibc_core_commitment_types::error::CommitmentError;
 use ibc_core_host_types::error::IdentifierError;
 use ibc_core_host_types::identifiers::ClientId;
 use ibc_primitives::prelude::*;
@@ -35,6 +36,8 @@ pub enum Error {
     MissingSignedHeader,
     /// invalid header, failed basic validation: `{reason}`
     Validation { reason: String },
+    /// invalid client proof specs: `{0}`
+    InvalidProofSpec(CommitmentError),
     /// invalid raw client state: `{reason}`
     InvalidRawClientState { reason: String },
     /// missing validator set

ibc-core/ics23-commitment/types/src/error.rs

@@ -13,6 +13,10 @@ pub enum CommitmentError {
     EmptyMerkleRoot,
     /// empty verified value
     EmptyVerifiedValue,
+    /// empty proof specs
+    EmptyProofSpecs,
+    /// empty depth range: [{0}, {1}]
+    EmptyDepthRange(i32, i32),
     /// mismatch between the number of proofs with that of specs
     NumberOfSpecsMismatch,
     /// mismatch between the number of proofs with that of keys

ibc-core/ics23-commitment/types/src/specs.rs

@@ -2,6 +2,8 @@
 
 use ibc_primitives::prelude::*;
 use ibc_proto::ics23::{InnerSpec as RawInnerSpec, LeafOp as RawLeafOp, ProofSpec as RawProofSpec};
+
+use crate::error::CommitmentError;
 /// An array of proof specifications.
 ///
 /// This type encapsulates different types of proof specifications, mostly predefined, e.g., for
@@ -23,6 +25,21 @@ impl ProofSpecs {
     pub fn is_empty(&self) -> bool {
         self.0.is_empty()
     }
+
+    pub fn validate(&self) -> Result<(), CommitmentError> {
+        if self.0.is_empty() {
+            return Err(CommitmentError::EmptyProofSpecs);
+        }
+        for proof_spec in &self.0 {
+            if proof_spec.0.min_depth > proof_spec.0.max_depth {
+                return Err(CommitmentError::EmptyDepthRange(
+                    proof_spec.0.min_depth,
+                    proof_spec.0.max_depth,
+                ));
+            }
+        }
+        Ok(())
+    }
 }
 
 impl Default for ProofSpecs {