Add missing validation checks for all IBC message types

.changelog/unreleased/breaking-changes/233-Missing-message-validation-checks.md

@@ -0,0 +1,2 @@
+- Add missing validation checks for all the IBC message types
+  ([#233](https://github.com/cosmos/ibc-rs/issues/233))

crates/ibc/src/applications/transfer/error.rs

@@ -14,25 +14,13 @@ use crate::prelude::*;
 pub enum TokenTransferError {
     /// context error: `{0}`
     ContextError(ContextError),
+    /// invalid identifier: `{0}`
+    InvalidIdentifier(ValidationError),
     /// destination channel not found in the counterparty of port_id `{port_id}` and channel_id `{channel_id}`
     DestinationChannelNotFound {
         port_id: PortId,
         channel_id: ChannelId,
     },
-    /// invalid port identifier `{context}`, validation error: `{validation_error}`
-    InvalidPortId {
-        context: String,
-        validation_error: ValidationError,
-    },
-    /// invalid channel identifier `{context}`, validation error: `{validation_error}`
-    InvalidChannelId {
-        context: String,
-        validation_error: ValidationError,
-    },
-    /// invalid packet timeout height value `{context}`
-    InvalidPacketTimeoutHeight { context: String },
-    /// invalid packet timeout timestamp value `{timestamp}`
-    InvalidPacketTimeoutTimestamp { timestamp: u64 },
     /// base denomination is empty
     EmptyBaseDenom,
     /// invalid prot id n trace at position: `{pos}`, validation error: `{validation_error}`
@@ -88,14 +76,7 @@ impl std::error::Error for TokenTransferError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self {
             Self::ContextError(e) => Some(e),
-            Self::InvalidPortId {
-                validation_error: e,
-                ..
-            } => Some(e),
-            Self::InvalidChannelId {
-                validation_error: e,
-                ..
-            } => Some(e),
+            Self::InvalidIdentifier(e) => Some(e),
             Self::InvalidTracePortId {
                 validation_error: e,
                 ..
@@ -123,3 +104,9 @@ impl From<ContextError> for TokenTransferError {
         Self::ContextError(err)
     }
 }
+
+impl From<ValidationError> for TokenTransferError {
+    fn from(err: ValidationError) -> TokenTransferError {
+        Self::InvalidIdentifier(err)
+    }
+}

crates/ibc/src/applications/transfer/msgs/transfer.rs

@@ -1,15 +1,17 @@
 //! This is the definition of a transfer messages that an application submits to a chain.
 
-use crate::applications::transfer::packet::PacketData;
 use crate::prelude::*;
 
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::applications::transfer::v1::MsgTransfer as RawMsgTransfer;
 use ibc_proto::protobuf::Protobuf;
 
 use crate::applications::transfer::error::TokenTransferError;
+use crate::applications::transfer::packet::PacketData;
+use crate::core::ics04_channel::error::PacketError;
 use crate::core::ics04_channel::timeout::TimeoutHeight;
 use crate::core::ics24_host::identifier::{ChannelId, PortId};
+use crate::core::ContextError;
 use crate::timestamp::Timestamp;
 use crate::tx_msg::Msg;
 
@@ -51,30 +53,22 @@ impl TryFrom<RawMsgTransfer> for MsgTransfer {
 
     fn try_from(raw_msg: RawMsgTransfer) -> Result<Self, Self::Error> {
         let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp)
-            .map_err(|_| TokenTransferError::InvalidPacketTimeoutTimestamp {
-                timestamp: raw_msg.timeout_timestamp,
-            })?;
-
-        let timeout_height_on_b: TimeoutHeight =
-            raw_msg.timeout_height.try_into().map_err(|e| {
-                TokenTransferError::InvalidPacketTimeoutHeight {
-                    context: format!("invalid timeout height {e}"),
-                }
-            })?;
+            .map_err(PacketError::InvalidPacketTimestamp)
+            .map_err(ContextError::from)?;
+
+        let timeout_height_on_b: TimeoutHeight = raw_msg
+            .timeout_height
+            .try_into()
+            .map_err(ContextError::from)?;
+
+        // Packet timeout height and packet timeout timestamp cannot both be unset.
+        if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() {
+            return Err(PacketError::MissingTimeout).map_err(ContextError::from)?;
+        }
 
         Ok(MsgTransfer {
-            port_id_on_a: raw_msg.source_port.parse().map_err(|e| {
-                TokenTransferError::InvalidPortId {
-                    context: raw_msg.source_port.clone(),
-                    validation_error: e,
-                }
-            })?,
-            chan_id_on_a: raw_msg.source_channel.parse().map_err(|e| {
-                TokenTransferError::InvalidChannelId {
-                    context: raw_msg.source_channel.clone(),
-                    validation_error: e,
-                }
-            })?,
+            port_id_on_a: raw_msg.source_port.parse()?,
+            chan_id_on_a: raw_msg.source_channel.parse()?,
             packet_data: PacketData {
                 token: raw_msg
                     .token

crates/ibc/src/applications/transfer/relay/send_transfer.rs

@@ -29,9 +29,7 @@ where
     ctx_a.can_send_coins()?;
 
     let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &msg.chan_id_on_a);
-    let chan_end_on_a = ctx_a
-        .channel_end(&chan_end_path_on_a)
-        .map_err(TokenTransferError::ContextError)?;
+    let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?;
 
     let port_id_on_b = chan_end_on_a.counterparty().port_id().clone();
     let chan_id_on_b = chan_end_on_a
@@ -44,9 +42,7 @@ where
         .clone();
 
     let seq_send_path_on_a = SeqSendPath::new(&msg.port_id_on_a, &msg.chan_id_on_a);
-    let sequence = ctx_a
-        .get_next_sequence_send(&seq_send_path_on_a)
-        .map_err(TokenTransferError::ContextError)?;
+    let sequence = ctx_a.get_next_sequence_send(&seq_send_path_on_a)?;
 
     let token = &msg.packet_data.token;
 
@@ -84,7 +80,7 @@ where
         }
     };
 
-    send_packet_validate(ctx_a, &packet).map_err(TokenTransferError::ContextError)?;
+    send_packet_validate(ctx_a, &packet)?;
 
     Ok(())
 }
@@ -97,9 +93,7 @@ where
     Ctx: TokenTransferExecutionContext,
 {
     let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &msg.chan_id_on_a);
-    let chan_end_on_a = ctx_a
-        .channel_end(&chan_end_path_on_a)
-        .map_err(TokenTransferError::ContextError)?;
+    let chan_end_on_a = ctx_a.channel_end(&chan_end_path_on_a)?;
 
     let port_on_b = chan_end_on_a.counterparty().port_id().clone();
     let chan_on_b = chan_end_on_a
@@ -113,9 +107,7 @@ where
 
     // get the next sequence
     let seq_send_path_on_a = SeqSendPath::new(&msg.port_id_on_a, &msg.chan_id_on_a);
-    let sequence = ctx_a
-        .get_next_sequence_send(&seq_send_path_on_a)
-        .map_err(TokenTransferError::ContextError)?;
+    let sequence = ctx_a.get_next_sequence_send(&seq_send_path_on_a)?;
 
     let token = &msg.packet_data.token;
 
@@ -155,7 +147,7 @@ where
         }
     };
 
-    send_packet_execute(ctx_a, packet).map_err(TokenTransferError::ContextError)?;
+    send_packet_execute(ctx_a, packet)?;
 
     {
         ctx_a.log_message(format!(

crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs

@@ -23,6 +23,8 @@ impl ClientState {
         client_id: &ClientId,
         misbehaviour: TmMisbehaviour,
     ) -> Result<(), ClientError> {
+        misbehaviour.validate_basic()?;
+
         let header_1 = misbehaviour.header1();
         let trusted_consensus_state_1 = {
             let consensus_state_path =
@@ -41,39 +43,11 @@ impl ClientState {
             downcast_tm_consensus_state(consensus_state.as_ref())
         }?;
 
-        self.check_misbehaviour_headers_consistency(header_1, header_2)?;
-
         let current_timestamp = ctx.host_timestamp()?;
         self.verify_misbehaviour_header(header_1, &trusted_consensus_state_1, current_timestamp)?;
         self.verify_misbehaviour_header(header_2, &trusted_consensus_state_2, current_timestamp)
     }
 
-    pub fn check_misbehaviour_headers_consistency(
-        &self,
-        header_1: &TmHeader,
-        header_2: &TmHeader,
-    ) -> Result<(), ClientError> {
-        if header_1.signed_header.header.chain_id != header_2.signed_header.header.chain_id {
-            return Err(Error::InvalidRawMisbehaviour {
-                reason: "headers must have identical chain_ids".to_owned(),
-            }
-            .into());
-        }
-
-        if header_1.height() < header_2.height() {
-            return Err(Error::InvalidRawMisbehaviour {
-                reason: format!(
-                    "headers1 height is less than header2 height ({} < {})",
-                    header_1.height(),
-                    header_2.height()
-                ),
-            }
-            .into());
-        }
-
-        Ok(())
-    }
-
     pub fn verify_misbehaviour_header(
         &self,
         header: &TmHeader,

crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs

@@ -20,31 +20,12 @@ impl ClientState {
         client_id: &ClientId,
         header: TmHeader,
     ) -> Result<(), ClientError> {
+        // Checks that the header fields are valid.
+        header.validate_basic()?;
+
         // The tendermint-light-client crate though works on heights that are assumed
         // to have the same revision number. We ensure this here.
-        if header.height().revision_number() != self.chain_id().version() {
-            return Err(ClientError::ClientSpecific {
-                description: Error::MismatchedRevisions {
-                    current_revision: self.chain_id().version(),
-                    update_revision: header.height().revision_number(),
-                }
-                .to_string(),
-            });
-        }
-
-        // We also need to ensure that the trusted height (representing the
-        // height of the header already on chain for which this client update is
-        // based on) must be smaller than height of the new header that we're
-        // installing.
-        if header.height() <= header.trusted_height {
-            return Err(ClientError::HeaderVerificationFailure {
-                reason: format!(
-                    "header height <= header trusted height ({} <= {})",
-                    header.height(),
-                    header.trusted_height
-                ),
-            });
-        }
+        header.verify_chain_id_version_matches_height(&self.chain_id())?;
 
         // Delegate to tendermint-light-client, which contains the required checks
         // of the new header against the trusted consensus state.

crates/ibc/src/clients/ics07_tendermint/consensus_state.rs

@@ -49,6 +49,18 @@ impl TryFrom<RawConsensusState> for ConsensusState {
     type Error = Error;
 
     fn try_from(raw: RawConsensusState) -> Result<Self, Self::Error> {
+        let proto_root = raw
+            .root
+            .ok_or(Error::InvalidRawClientState {
+                reason: "missing commitment root".into(),
+            })?
+            .hash;
+        if proto_root.is_empty() {
+            return Err(Error::InvalidRawClientState {
+                reason: "empty commitment root".into(),
+            });
+        };
+
         let ibc_proto::google::protobuf::Timestamp { seconds, nanos } =
             raw.timestamp.ok_or(Error::InvalidRawClientState {
                 reason: "missing timestamp".into(),
@@ -62,19 +74,15 @@ impl TryFrom<RawConsensusState> for ConsensusState {
                 reason: format!("invalid timestamp: {e}"),
             })?;
 
+        let next_validators_hash = Hash::from_bytes(Algorithm::Sha256, &raw.next_validators_hash)
+            .map_err(|e| Error::InvalidRawClientState {
+            reason: e.to_string(),
+        })?;
+
         Ok(Self {
-            root: raw
-                .root
-                .ok_or(Error::InvalidRawClientState {
-                    reason: "missing commitment root".into(),
-                })?
-                .hash
-                .into(),
+            root: proto_root.into(),
             timestamp,
-            next_validators_hash: Hash::from_bytes(Algorithm::Sha256, &raw.next_validators_hash)
-                .map_err(|e| Error::InvalidRawClientState {
-                    reason: e.to_string(),
-                })?,
+            next_validators_hash,
         })
     }
 }

crates/ibc/src/clients/ics07_tendermint/error.rs

@@ -11,7 +11,6 @@ use tendermint::account::Id;
 use tendermint::{Error as TendermintError, Hash};
 use tendermint_light_client_verifier::errors::VerificationErrorDetail as LightClientErrorDetail;
 use tendermint_light_client_verifier::operations::VotingPowerTally;
-use tendermint_light_client_verifier::types::Validator;
 use tendermint_light_client_verifier::Verdict;
 
 #[derive(Debug, Display)]
@@ -69,11 +68,13 @@ pub enum Error {
     InvalidHeaderHeight { height: u64 },
     /// Disallowed to create a new client with a frozen height
     FrozenHeightNotAllowed,
-    /// the header's current/trusted revision number (`{current_revision}`) and the update's revision number (`{update_revision}`) should be the same
-    MismatchedRevisions {
-        current_revision: u64,
-        update_revision: u64,
+    /// the header's trusted revision number (`{trusted_revision}`) and the update's revision number (`{header_revision}`) should be the same
+    MismatchHeightRevisions {
+        trusted_revision: u64,
+        header_revision: u64,
     },
+    /// the given chain-id (`{given}`) does not match the chain-id of the client (`{expected}`)
+    MismatchHeaderChainId { given: String, expected: String },
     /// not enough trust because insufficient validators overlap: `{reason}`
     NotEnoughTrustedValsSigned { reason: VotingPowerTally },
     /// verification failed: `{detail}`
@@ -82,11 +83,10 @@ pub enum Error {
     ProcessedTimeNotFound { client_id: ClientId, height: Height },
     /// Processed height for the client `{client_id}` at height `{height}` not found
     ProcessedHeightNotFound { client_id: ClientId, height: Height },
-    /// trusted validators `{trusted_validator_set:?}`, does not hash to latest trusted validators. Expected: `{next_validators_hash}`, got: `{trusted_val_hash}`
-    MisbehaviourTrustedValidatorHashMismatch {
-        trusted_validator_set: Vec<Validator>,
-        next_validators_hash: Hash,
-        trusted_val_hash: Hash,
+    /// The given hash of the validators does not matches the given hash in the signed header. Expected: `{signed_header_validators_hash}`, got: `{validators_hash}`
+    MismatchValidatorsHashes {
+        validators_hash: Hash,
+        signed_header_validators_hash: Hash,
     },
     /// current timestamp minus the latest consensus state timestamp is greater than or equal to the trusting period (`{duration_since_consensus_state:?}` >= `{trusting_period:?}`)
     ConsensusStateTimestampGteTrustingPeriod {
@@ -97,11 +97,6 @@ pub enum Error {
     MisbehaviourHeadersBlockHashesEqual,
     /// headers are not at same height and are monotonically increasing
     MisbehaviourHeadersNotAtSameHeight,
-    /// header chain-id `{header_chain_id}` does not match the light client's chain-id `{chain_id}`)
-    MisbehaviourHeadersChainIdMismatch {
-        header_chain_id: String,
-        chain_id: String,
-    },
     /// invalid raw client id: `{client_id}`
     InvalidRawClientId { client_id: String },
 }