diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 97fde1674..474aa32f5 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -296,7 +296,7 @@ impl TryFrom for ClientState { unbonding_period, max_clock_drift, latest_height, - raw.proof_specs.into(), + raw.proof_specs.try_into()?, raw.upgrade_path, frozen_height, allow_update, @@ -411,8 +411,6 @@ pub(crate) mod serde_tests { #[cfg(test)] mod tests { - use ibc_core_commitment_types::proto::ics23::ProofSpec as Ics23ProofSpec; - use super::*; #[derive(Clone, Debug, PartialEq)] @@ -556,28 +554,6 @@ mod tests { }, want_pass: false, }, - Test { - name: "Invalid (empty) proof specs".to_string(), - params: ClientStateParams { - 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, - }, ] .into_iter() .collect(); diff --git a/ibc-core/ics23-commitment/types/Cargo.toml b/ibc-core/ics23-commitment/types/Cargo.toml index 15cd37dd4..905689347 100644 --- a/ibc-core/ics23-commitment/types/Cargo.toml +++ b/ibc-core/ics23-commitment/types/Cargo.toml @@ -35,6 +35,9 @@ ics23 = { version = "0.11", default-features = false, features = ["hos parity-scale-codec = { workspace = true, optional = true } scale-info = { workspace = true, optional = true } +[dev-dependencies] +rstest = { workspace = true } + [features] default = ["std"] std = [ diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 0d9cf21b1..fdbdd24c7 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/ibc-core/ics23-commitment/types/src/error.rs @@ -29,6 +29,14 @@ pub enum CommitmentError { EncodingFailure(String), /// decoding commitment proof bytes failed: `{0}` DecodingFailure(String), + /// invalid prefix length range: `[{0}, {1}]` + InvalidPrefixLengthRange(i32, i32), + /// invalid child size: `{0}` + InvalidChildSize(i32), + /// invalid hash operation: `{0}` + InvalidHashOp(i32), + /// invalid length operation: `{0}` + InvalidLengthOp(i32), } #[cfg(feature = "std")] diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 3cd40ac84..46bf79a95 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -2,6 +2,7 @@ use ibc_primitives::prelude::*; use ibc_proto::ics23::{InnerSpec as RawInnerSpec, LeafOp as RawLeafOp, ProofSpec as RawProofSpec}; +use ics23::{HashOp, LengthOp}; use crate::error::CommitmentError; /// An array of proof specifications. @@ -19,7 +20,8 @@ impl ProofSpecs { ics23::iavl_spec(), // Format of proofs-iavl (iavl merkle proofs) ics23::tendermint_spec(), // Format of proofs-tendermint (crypto/ merkle SimpleProof) ] - .into() + .try_into() + .expect("should convert successfully") } pub fn is_empty(&self) -> bool { @@ -31,9 +33,15 @@ impl ProofSpecs { return Err(CommitmentError::EmptyProofSpecs); } for proof_spec in &self.0 { - if proof_spec.0.max_depth < proof_spec.0.min_depth + // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. + // For simplicity, negative values for `min_depth` and `max_depth` are not allowed + // and only `0` is used to indicate no limit. When `min_depth` and `max_depth` are both positive, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. + if proof_spec.0.max_depth < 0 || proof_spec.0.min_depth < 0 - || proof_spec.0.max_depth < 0 + || (0 < proof_spec.0.min_depth + && 0 < proof_spec.0.max_depth + && proof_spec.0.max_depth < proof_spec.0.min_depth) { return Err(CommitmentError::InvalidDepthRange( proof_spec.0.min_depth, @@ -45,9 +53,19 @@ impl ProofSpecs { } } -impl From> for ProofSpecs { - fn from(ics23_specs: Vec) -> Self { - Self(ics23_specs.into_iter().map(Into::into).collect()) +impl TryFrom> for ProofSpecs { + type Error = CommitmentError; + fn try_from(ics23_specs: Vec) -> Result { + // no proof specs provided + if ics23_specs.is_empty() { + return Err(CommitmentError::EmptyProofSpecs); + } + + ics23_specs + .into_iter() + .map(ProofSpec::try_from) + .collect::>() + .map(Self) } } @@ -61,28 +79,47 @@ impl From for Vec { #[derive(Clone, Debug, PartialEq)] struct ProofSpec(RawProofSpec); -impl From for ProofSpec { - fn from(spec: RawProofSpec) -> Self { - Self(RawProofSpec { - leaf_spec: spec.leaf_spec.map(|lop| LeafOp::from(lop).0), - inner_spec: spec.inner_spec.map(|ispec| InnerSpec::from(ispec).0), +impl TryFrom for ProofSpec { + type Error = CommitmentError; + fn try_from(spec: RawProofSpec) -> Result { + // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. + // For simplicity, negative values for `min_depth` and `max_depth` are not allowed + // and only `0` is used to indicate no limit. When `min_depth` and `max_depth` are both positive, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. + if spec.max_depth < 0 + || spec.min_depth < 0 + || (0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth) + { + return Err(CommitmentError::InvalidDepthRange( + spec.min_depth, + spec.max_depth, + )); + } + + let leaf_spec = spec + .leaf_spec + .map(LeafOp::try_from) + .transpose()? + .map(|lop| lop.0); + let inner_spec = spec + .inner_spec + .map(InnerSpec::try_from) + .transpose()? + .map(|ispec| ispec.0); + + Ok(Self(RawProofSpec { + leaf_spec, + inner_spec, max_depth: spec.max_depth, min_depth: spec.min_depth, prehash_key_before_comparison: spec.prehash_key_before_comparison, - }) + })) } } impl From for RawProofSpec { fn from(spec: ProofSpec) -> Self { - let spec = spec.0; - RawProofSpec { - leaf_spec: spec.leaf_spec.map(|lop| LeafOp(lop).into()), - inner_spec: spec.inner_spec.map(|ispec| InnerSpec(ispec).into()), - max_depth: spec.max_depth, - min_depth: spec.min_depth, - prehash_key_before_comparison: spec.prehash_key_before_comparison, - } + spec.0 } } @@ -90,28 +127,25 @@ impl From for RawProofSpec { #[derive(Clone, Debug, PartialEq)] struct LeafOp(RawLeafOp); -impl From for LeafOp { - fn from(leaf_op: RawLeafOp) -> Self { - Self(RawLeafOp { - hash: leaf_op.hash, - prehash_key: leaf_op.prehash_key, - prehash_value: leaf_op.prehash_value, - length: leaf_op.length, - prefix: leaf_op.prefix, - }) +impl TryFrom for LeafOp { + type Error = CommitmentError; + fn try_from(leaf_op: RawLeafOp) -> Result { + let _ = HashOp::try_from(leaf_op.hash) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.hash))?; + let _ = HashOp::try_from(leaf_op.prehash_key) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.prehash_key))?; + let _ = HashOp::try_from(leaf_op.prehash_value) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.prehash_value))?; + let _ = LengthOp::try_from(leaf_op.length) + .map_err(|_| CommitmentError::InvalidLengthOp(leaf_op.length))?; + + Ok(Self(leaf_op)) } } impl From for RawLeafOp { fn from(leaf_op: LeafOp) -> Self { - let leaf_op = leaf_op.0; - RawLeafOp { - hash: leaf_op.hash, - prehash_key: leaf_op.prehash_key, - prehash_value: leaf_op.prehash_value, - length: leaf_op.length, - prefix: leaf_op.prefix, - } + leaf_op.0 } } @@ -119,29 +153,132 @@ impl From for RawLeafOp { #[derive(Clone, Debug, PartialEq)] struct InnerSpec(RawInnerSpec); -impl From for InnerSpec { - fn from(inner_spec: RawInnerSpec) -> Self { - Self(RawInnerSpec { +impl TryFrom for InnerSpec { + type Error = CommitmentError; + fn try_from(inner_spec: RawInnerSpec) -> Result { + if inner_spec.child_size <= 0 { + return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); + } + + // Negative prefix lengths are not allowed and the maximum prefix length must + // be greater than or equal to the minimum prefix length. + if inner_spec.min_prefix_length < 0 + || inner_spec.max_prefix_length < 0 + || inner_spec.max_prefix_length < inner_spec.min_prefix_length + { + return Err(CommitmentError::InvalidPrefixLengthRange( + inner_spec.min_prefix_length, + inner_spec.max_prefix_length, + )); + } + + Ok(Self(RawInnerSpec { child_order: inner_spec.child_order, child_size: inner_spec.child_size, min_prefix_length: inner_spec.min_prefix_length, max_prefix_length: inner_spec.max_prefix_length, empty_child: inner_spec.empty_child, hash: inner_spec.hash, - }) + })) } } impl From for RawInnerSpec { fn from(inner_spec: InnerSpec) -> Self { - let inner_spec = inner_spec.0; - RawInnerSpec { - child_order: inner_spec.child_order, - child_size: inner_spec.child_size, - min_prefix_length: inner_spec.min_prefix_length, - max_prefix_length: inner_spec.max_prefix_length, - empty_child: inner_spec.empty_child, - hash: inner_spec.hash, - } + inner_spec.0 + } +} + +#[cfg(test)] +mod tests { + use ibc_proto::ics23::{InnerSpec as RawInnerSpec, ProofSpec as RawProofSpec}; + use rstest::rstest; + + use super::*; + + #[rstest] + #[case(0, 0)] + #[case(2, 2)] + #[case(5, 6)] + #[should_panic(expected = "InvalidDepthRange")] + #[case(-3,3)] + #[should_panic(expected = "InvalidDepthRange")] + #[case(2,-6)] + #[should_panic(expected = "InvalidDepthRange")] + #[case(-2,-6)] + #[should_panic(expected = "InvalidDepthRange")] + #[case(-6,-2)] + #[should_panic(expected = "InvalidDepthRange")] + #[case(5, 3)] + fn test_proof_specs_try_from(#[case] min_depth: i32, #[case] max_depth: i32) { + let raw_proof_spec = RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth, + min_depth, + prehash_key_before_comparison: false, + }; + ProofSpec::try_from(raw_proof_spec).unwrap(); + } + + #[rstest] + #[case(0, 0)] + #[case(1, 2)] + #[case(2, 2)] + #[should_panic(expected = "InvalidPrefixLengthRange")] + #[case(2, 1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] + #[case(-2,1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] + #[case(2,-1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] + #[case(-2,-1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] + #[case(-1,-2)] + fn test_inner_specs_try_from(#[case] min_prefix_length: i32, #[case] max_prefix_length: i32) { + let raw_inner_spec = RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length, + max_prefix_length, + empty_child: vec![], + hash: 1, + }; + InnerSpec::try_from(raw_inner_spec).unwrap(); + } + + #[rstest] + #[case(0, 0, 0, 0)] + #[case(9, 9, 9, 8)] + #[should_panic(expected = "InvalidHashOp")] + #[case(-1, 4, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(10, 4, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, -1, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 10, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 4, -1, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 4, 10, 4)] + #[should_panic(expected = "InvalidLengthOp")] + #[case(4, 4, 4, -1)] + #[should_panic(expected = "InvalidLengthOp")] + #[case(4, 4, 4, 9)] + fn test_leaf_op_try_from( + #[case] hash: i32, + #[case] prehash_key: i32, + #[case] prehash_value: i32, + #[case] length: i32, + ) { + let raw_leaf_op = RawLeafOp { + hash, + prehash_key, + prehash_value, + length, + prefix: vec![], + }; + LeafOp::try_from(raw_leaf_op).unwrap(); } }