From 65d84464842b3620f0bd66a07af30705bfa37761 Mon Sep 17 00:00:00 2001 From: Emmanuel Ekpenyong <39572181+gr4yha7@users.noreply.github.com> Date: Fri, 1 Mar 2024 17:56:08 +0100 Subject: [PATCH] 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 Co-authored-by: Rano | Ranadeep --- ...-fix-tendermint-empty-proof-specs-check.md | 2 ++ .../types/src/client_state.rs | 24 +++++++++++++------ .../ics07-tendermint/types/src/error.rs | 9 +++++++ ibc-core/ics23-commitment/types/src/error.rs | 4 ++++ ibc-core/ics23-commitment/types/src/specs.rs | 20 ++++++++++++++++ 5 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/1100-fix-tendermint-empty-proof-specs-check.md diff --git a/.changelog/unreleased/bug-fixes/1100-fix-tendermint-empty-proof-specs-check.md b/.changelog/unreleased/bug-fixes/1100-fix-tendermint-empty-proof-specs-check.md new file mode 100644 index 000000000..b4a3aa542 --- /dev/null +++ b/.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)) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 5fbb53189..98ed61132 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/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::::new()), + proof_specs: Vec::::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, diff --git a/ibc-clients/ics07-tendermint/types/src/error.rs b/ibc-clients/ics07-tendermint/types/src/error.rs index 556d5e6fe..3e2a51112 100644 --- a/ibc-clients/ics07-tendermint/types/src/error.rs +++ b/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 for Error { } } +impl From for Error { + fn from(e: CommitmentError) -> Self { + Self::InvalidProofSpec(e) + } +} + pub trait IntoResult { fn into_result(self) -> Result; } diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 88625226f..0d9cf21b1 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/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 diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 3c2b1df75..2e18094a0 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/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 {