Disallow empty CommitmentPrefix and CommitmentProofBytes

.changelog/unreleased/improvements/ibc/1761-disallow-empty-commitment-prefix-and-proof.md

@@ -0,0 +1,2 @@
+- Disallow empty `CommitmentPrefix` and `CommitmentProofBytes`
+  ([#1761](https://github.com/informalsystems/ibc-rs/issues/1761))

modules/src/clients/ics07_tendermint/client_def.rs

@@ -400,8 +400,7 @@ fn verify_membership(
     path: impl Into<Path>,
     value: Vec<u8>,
 ) -> Result<(), Ics02Error> {
-    let merkle_path =
-        apply_prefix(prefix, vec![path.into().to_string()]).map_err(Error::ics23_error)?;
+    let merkle_path = apply_prefix(prefix, vec![path.into().to_string()]);
     let merkle_proof: MerkleProof = RawMerkleProof::try_from(proof.clone())
         .map_err(Ics02Error::invalid_commitment_proof)?
         .into();
@@ -424,8 +423,7 @@ fn verify_non_membership(
     root: &CommitmentRoot,
     path: impl Into<Path>,
 ) -> Result<(), Ics02Error> {
-    let merkle_path =
-        apply_prefix(prefix, vec![path.into().to_string()]).map_err(Error::ics23_error)?;
+    let merkle_path = apply_prefix(prefix, vec![path.into().to_string()]);
     let merkle_proof: MerkleProof = RawMerkleProof::try_from(proof.clone())
         .map_err(Ics02Error::invalid_commitment_proof)?
         .into();

modules/src/core/ics02_client/error.rs

@@ -72,7 +72,6 @@ define_error! {
             | _ | { "the client state was not found" },
 
         EmptyPrefix
-            [ Ics23Error ]
             | _ | { "empty prefix" },
 
         UnknownConsensusStateType

modules/src/core/ics02_client/handler/upgrade_client.rs

@@ -85,7 +85,6 @@ mod tests {
     use crate::core::ics02_client::handler::ClientResult::Upgrade;
     use crate::core::ics02_client::msgs::upgrade_client::MsgUpgradeAnyClient;
     use crate::core::ics02_client::msgs::ClientMsg;
-    use crate::core::ics23_commitment::commitment::CommitmentProofBytes;
     use crate::core::ics24_host::identifier::ClientId;
     use crate::events::IbcEvent;
     use crate::handler::HandlerOutput;
@@ -94,7 +93,6 @@ mod tests {
     use crate::mock::header::MockHeader;
     use crate::test_utils::get_dummy_account_id;
     use crate::Height;
-    use ibc_proto::ibc::core::commitment::v1::MerkleProof;
 
     #[test]
     fn test_upgrade_client_ok() {
@@ -103,18 +101,12 @@ mod tests {
 
         let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42));
 
-        let buf: Vec<u8> = Vec::new();
-        let buf2: Vec<u8> = Vec::new();
-
-        let c_bytes = CommitmentProofBytes::from(buf);
-        let cs_bytes = CommitmentProofBytes::from(buf2);
-
         let msg = MsgUpgradeAnyClient {
             client_id: client_id.clone(),
             client_state: MockClientState::new(MockHeader::new(Height::new(1, 26))).into(),
             consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26))).into(),
-            proof_upgrade_client: MerkleProof::try_from(c_bytes).unwrap(),
-            proof_upgrade_consensus_state: MerkleProof::try_from(cs_bytes).unwrap(),
+            proof_upgrade_client: Default::default(),
+            proof_upgrade_consensus_state: Default::default(),
             signer,
         };
 
@@ -154,18 +146,12 @@ mod tests {
 
         let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42));
 
-        let buf: Vec<u8> = Vec::new();
-        let buf2: Vec<u8> = Vec::new();
-
-        let c_bytes = CommitmentProofBytes::from(buf);
-        let cs_bytes = CommitmentProofBytes::from(buf2);
-
         let msg = MsgUpgradeAnyClient {
             client_id: ClientId::from_str("nonexistingclient").unwrap(),
             client_state: MockClientState::new(MockHeader::new(Height::new(1, 26))).into(),
             consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26))).into(),
-            proof_upgrade_client: MerkleProof::try_from(c_bytes).unwrap(),
-            proof_upgrade_consensus_state: MerkleProof::try_from(cs_bytes).unwrap(),
+            proof_upgrade_client: Default::default(),
+            proof_upgrade_consensus_state: Default::default(),
             signer,
         };
 
@@ -188,18 +174,12 @@ mod tests {
 
         let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42));
 
-        let buf: Vec<u8> = Vec::new();
-        let buf2: Vec<u8> = Vec::new();
-
-        let c_bytes = CommitmentProofBytes::from(buf);
-        let cs_bytes = CommitmentProofBytes::from(buf2);
-
         let msg = MsgUpgradeAnyClient {
             client_id,
             client_state: MockClientState::new(MockHeader::new(Height::new(0, 26))).into(),
             consensus_state: MockConsensusState::new(MockHeader::new(Height::new(0, 26))).into(),
-            proof_upgrade_client: MerkleProof::try_from(c_bytes).unwrap(),
-            proof_upgrade_consensus_state: MerkleProof::try_from(cs_bytes).unwrap(),
+            proof_upgrade_client: Default::default(),
+            proof_upgrade_consensus_state: Default::default(),
             signer,
         };
 

modules/src/core/ics02_client/msgs/upgrade_client.rs

@@ -12,6 +12,7 @@ use crate::core::ics02_client::client_consensus::AnyConsensusState;
 use crate::core::ics02_client::client_state::AnyClientState;
 use crate::core::ics02_client::error::Error;
 use crate::core::ics23_commitment::commitment::CommitmentProofBytes;
+use crate::core::ics23_commitment::error::Error as Ics23Error;
 use crate::core::ics24_host::identifier::ClientId;
 use crate::signer::Signer;
 use crate::tx_msg::Msg;
@@ -65,15 +66,17 @@ impl Protobuf<RawMsgUpgradeClient> for MsgUpgradeAnyClient {}
 
 impl From<MsgUpgradeAnyClient> for RawMsgUpgradeClient {
     fn from(dm_msg: MsgUpgradeAnyClient) -> RawMsgUpgradeClient {
-        let c_bytes: CommitmentProofBytes = dm_msg.proof_upgrade_client.into();
-        let cs_bytes: CommitmentProofBytes = dm_msg.proof_upgrade_consensus_state.into();
+        let c_bytes = CommitmentProofBytes::try_from(dm_msg.proof_upgrade_client)
+            .map_or(vec![], |c| c.into());
+        let cs_bytes = CommitmentProofBytes::try_from(dm_msg.proof_upgrade_consensus_state)
+            .map_or(vec![], |c| c.into());
 
         RawMsgUpgradeClient {
             client_id: dm_msg.client_id.to_string(),
             client_state: Some(dm_msg.client_state.into()),
             consensus_state: Some(dm_msg.consensus_state.into()),
-            proof_upgrade_client: c_bytes.into(),
-            proof_upgrade_consensus_state: cs_bytes.into(),
+            proof_upgrade_client: c_bytes,
+            proof_upgrade_consensus_state: cs_bytes,
             signer: dm_msg.signer.to_string(),
         }
     }
@@ -91,8 +94,12 @@ impl TryFrom<RawMsgUpgradeClient> for MsgUpgradeAnyClient {
             .consensus_state
             .ok_or_else(Error::missing_raw_client_state)?;
 
-        let c_bytes = CommitmentProofBytes::from(proto_msg.proof_upgrade_client);
-        let cs_bytes = CommitmentProofBytes::from(proto_msg.proof_upgrade_consensus_state);
+        let c_bytes = CommitmentProofBytes::try_from(proto_msg.proof_upgrade_client)
+            .map_err(|_| Error::invalid_upgrade_client_proof(Ics23Error::empty_merkle_proof()))?;
+        let cs_bytes = CommitmentProofBytes::try_from(proto_msg.proof_upgrade_consensus_state)
+            .map_err(|_| {
+                Error::invalid_upgrade_consensus_state_proof(Ics23Error::empty_merkle_proof())
+            })?;
 
         Ok(MsgUpgradeAnyClient {
             client_id: ClientId::from_str(&proto_msg.client_id)

modules/src/core/ics03_connection/connection.rs

@@ -12,6 +12,7 @@ use ibc_proto::ibc::core::connection::v1::{
     IdentifiedConnection as RawIdentifiedConnection,
 };
 
+use crate::core::ics02_client::error::Error as ClientError;
 use crate::core::ics03_connection::error::Error;
 use crate::core::ics03_connection::version::Version;
 use crate::core::ics23_commitment::commitment::CommitmentPrefix;
@@ -262,7 +263,8 @@ impl TryFrom<RawCounterparty> for Counterparty {
                 .prefix
                 .ok_or_else(Error::missing_counterparty)?
                 .key_prefix
-                .into(),
+                .try_into()
+                .map_err(|_| Error::ics02_client(ClientError::empty_prefix()))?,
         ))
     }
 }

modules/src/core/ics03_connection/handler/conn_open_ack.rs

@@ -145,7 +145,7 @@ mod tests {
             Counterparty::new(
                 client_id.clone(),
                 Some(msg_ack.counterparty_connection_id().clone()),
-                CommitmentPrefix::from(b"ibc".to_vec()),
+                CommitmentPrefix::try_from(b"ibc".to_vec()).unwrap(),
             ),
             vec![msg_ack.version().clone()],
             ZERO_DURATION,
@@ -155,16 +155,6 @@ mod tests {
         let mut conn_end_open = default_conn_end.clone();
         conn_end_open.set_state(State::Open); // incorrect field
 
-        // A connection end with correct state, but incorrect prefix for the
-        // counterparty; will be part of the context to exercise unsuccessful path.
-        let mut conn_end_prefix = conn_end_open.clone();
-        conn_end_prefix.set_state(State::Init);
-        conn_end_prefix.set_counterparty(Counterparty::new(
-            client_id.clone(),
-            Some(msg_ack.counterparty_connection_id().clone()),
-            CommitmentPrefix::from(Vec::new()), // incorrect field
-        ));
-
         let tests: Vec<Test> = vec![
             Test {
                 name: "Successful processing of an Ack message".to_string(),
@@ -174,70 +164,46 @@ mod tests {
                     .with_connection(conn_id.clone(), default_conn_end),
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
                 want_pass: true,
-                match_error: Box::new(|_| {
-                    panic!("should not have error")
-                }),
+                match_error: Box::new(|_| panic!("should not have error")),
             },
             Test {
-                name: "Processing fails because the connection does not exist in the context".to_string(),
+                name: "Processing fails because the connection does not exist in the context"
+                    .to_string(),
                 ctx: default_context.clone(),
                 msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
                 want_pass: false,
                 match_error: {
                     let connection_id = conn_id.clone();
-                    Box::new(move |e| {
-                        match e.detail() {
-                            error::ErrorDetail::ConnectionNotFound(e) => {
-                                assert_eq!(e.connection_id, connection_id)
-                            }
-                            _ => {
-                                panic!("Expected ConnectionNotFound error");
-                            }
+                    Box::new(move |e| match e.detail() {
+                        error::ErrorDetail::ConnectionNotFound(e) => {
+                            assert_eq!(e.connection_id, connection_id)
+                        }
+                        _ => {
+                            panic!("Expected ConnectionNotFound error");
                         }
                     })
                 },
             },
             Test {
-                name: "Processing fails due to connections mismatch (incorrect 'open' state)".to_string(),
+                name: "Processing fails due to connections mismatch (incorrect 'open' state)"
+                    .to_string(),
                 ctx: default_context
-                    .clone()
                     .with_client(&client_id, proof_height)
                     .with_connection(conn_id.clone(), conn_end_open),
-                msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
+                msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)),
                 want_pass: false,
                 match_error: {
-                    let connection_id = conn_id.clone();
-                    Box::new(move |e| {
-                        match e.detail() {
-                            error::ErrorDetail::ConnectionMismatch(e) => {
-                                assert_eq!(e.connection_id, connection_id);
-                            }
-                            _ => {
-                                panic!("Expected ConnectionMismatch error");
-                            }
+                    let connection_id = conn_id;
+                    Box::new(move |e| match e.detail() {
+                        error::ErrorDetail::ConnectionMismatch(e) => {
+                            assert_eq!(e.connection_id, connection_id);
+                        }
+                        _ => {
+                            panic!("Expected ConnectionMismatch error");
                         }
                     })
                 },
             },
-            Test {
-                name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(),
-                ctx: default_context
-                    .with_client(&client_id, proof_height)
-                    .with_connection(conn_id, conn_end_prefix),
-                msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)),
-                want_pass: false,
-                match_error:
-                    Box::new(move |e| {
-                        match e.detail() {
-                            error::ErrorDetail::ConsensusStateVerificationFailure(e) => {
-                                assert_eq!(e.height, proof_height)
-                            }
-                            _ => {
-                                panic!("Expected ConsensusStateVerificationFailure error");
-                            }
-                        }
-                    }),
-            },
             /*
             Test {
                 name: "Processing fails due to MissingLocalConsensusState".to_string(),

modules/src/core/ics03_connection/handler/conn_open_confirm.rs

@@ -104,7 +104,7 @@ mod tests {
         let counterparty = Counterparty::new(
             client_id.clone(),
             Some(msg_confirm.connection_id().clone()),
-            CommitmentPrefix::from(Vec::new()),
+            CommitmentPrefix::try_from(b"ibc".to_vec()).unwrap(),
         );
 
         let context = MockContext::default();

modules/src/core/ics03_connection/msgs/conn_open_ack.rs

@@ -85,17 +85,21 @@ impl TryFrom<RawMsgConnectionOpenAck> for MsgConnectionOpenAck {
             .consensus_height
             .ok_or_else(Error::missing_consensus_height)?
             .into();
-        let consensus_proof_obj = ConsensusProof::new(msg.proof_consensus.into(), consensus_height)
-            .map_err(Error::invalid_proof)?;
+        let consensus_proof_obj = ConsensusProof::new(
+            msg.proof_consensus
+                .try_into()
+                .map_err(Error::invalid_proof)?,
+            consensus_height,
+        )
+        .map_err(Error::invalid_proof)?;
 
         let proof_height = msg
             .proof_height
             .ok_or_else(Error::missing_proof_height)?
             .into();
 
-        let client_proof = Some(msg.proof_client)
-            .filter(|x| !x.is_empty())
-            .map(CommitmentProofBytes::from);
+        let client_proof =
+            CommitmentProofBytes::try_from(msg.proof_client).map_err(Error::invalid_proof)?;
 
         Ok(Self {
             connection_id: msg
@@ -113,8 +117,8 @@ impl TryFrom<RawMsgConnectionOpenAck> for MsgConnectionOpenAck {
                 .map_err(Error::ics02_client)?,
             version: msg.version.ok_or_else(Error::empty_versions)?.try_into()?,
             proofs: Proofs::new(
-                msg.proof_try.into(),
-                client_proof,
+                msg.proof_try.try_into().map_err(Error::invalid_proof)?,
+                Some(client_proof),
                 Option::from(consensus_proof_obj),
                 None,
                 proof_height,
@@ -182,7 +186,7 @@ pub mod test_util {
                 revision_height: consensus_height,
             }),
             client_state: None,
-            proof_client: Vec::new(),
+            proof_client: get_dummy_proof(),
             version: Some(Version::default().into()),
             signer: get_dummy_bech32_account(),
         }

modules/src/core/ics03_connection/msgs/conn_open_confirm.rs

@@ -63,8 +63,14 @@ impl TryFrom<RawMsgConnectionOpenConfirm> for MsgConnectionOpenConfirm {
                 .connection_id
                 .parse()
                 .map_err(Error::invalid_identifier)?,
-            proofs: Proofs::new(msg.proof_ack.into(), None, None, None, proof_height)
-                .map_err(Error::invalid_proof)?,
+            proofs: Proofs::new(
+                msg.proof_ack.try_into().map_err(Error::invalid_proof)?,
+                None,
+                None,
+                None,
+                proof_height,
+            )
+            .map_err(Error::invalid_proof)?,
             signer: msg.signer.into(),
         })
     }