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(),
  })
  }