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

* fix: disallow empty proof-specs and empty depth range in proof-specs

* chore: add changelog entry

* nits

* test: add unit test

* chore: use default option (none)

* avoid new var

* nit in changelog

* rm redundant clone

* propagate errors

* proof depth is unsigned

* use impl method

---------

Co-authored-by: Ranadeep Biswas <mail@rnbguy.at>
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems>

Author: gr4yha7

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

@@ -0,0 +1,2 @@
+- [ibc-client-tendermint-types] Check ics23 proof specs for empty depth range.
+  ([\#1100](https://github.com/cosmos/ibc-rs/issues/1100))

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

@@ -176,12 +176,8 @@ 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
+        self.proof_specs.validate()?;
 
         // `upgrade_path` itself may be empty, but if not then each key must be non-empty
         for (idx, key) in self.upgrade_path.iter().enumerate() {
@@ -565,7 +561,21 @@ mod tests {
             Test {
                 name: "Invalid (empty) proof specs".to_string(),
                 params: ClientStateParams {
-                    proof_specs: ProofSpecs::from(Vec::<Ics23ProofSpec>::new()),
+                    proof_specs: Vec::<Ics23ProofSpec>::new().into(),
+                    ..default_params.clone()
+                },
+                want_pass: false,
+            },
+            Test {
+                name: "Invalid (empty) proof specs depth range".to_string(),
+                params: ClientStateParams {
+                    proof_specs: vec![Ics23ProofSpec {
+                        leaf_spec: None,
+                        inner_spec: None,
+                        min_depth: 2,
+                        max_depth: 1,
+                        prehash_key_before_comparison: false,
+                    }].into(),
                     ..default_params
                 },
                 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
@@ -119,6 +122,12 @@ impl From<IdentifierError> for Error {
     }
 }
 
+impl From<CommitmentError> for Error {
+    fn from(e: CommitmentError) -> Self {
+        Self::InvalidProofSpec(e)
+    }
+}
+
 pub trait IntoResult<T, E> {
     fn into_result(self) -> Result<T, E>;
 }

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

@@ -13,6 +13,10 @@ pub enum CommitmentError {
     EmptyMerkleRoot,
     /// empty verified value
     EmptyVerifiedValue,
+    /// empty proof specs
+    EmptyProofSpecs,
+    /// invalid depth range: [{0}, {1}]
+    InvalidDepthRange(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,24 @@ impl ProofSpecs {
     pub fn is_empty(&self) -> bool {
         self.0.is_empty()
     }
+
+    pub fn validate(&self) -> Result<(), CommitmentError> {
+        if self.is_empty() {
+            return Err(CommitmentError::EmptyProofSpecs);
+        }
+        for proof_spec in &self.0 {
+            if proof_spec.0.max_depth < proof_spec.0.min_depth
+                || proof_spec.0.min_depth < 0
+                || proof_spec.0.max_depth < 0
+            {
+                return Err(CommitmentError::InvalidDepthRange(
+                    proof_spec.0.min_depth,
+                    proof_spec.0.max_depth,
+                ));
+            }
+        }
+        Ok(())
+    }
 }
 
 impl Default for ProofSpecs {