Merge pull request #27 from datachainlab/fix-height-validation

Fix height and misbehaviour validation

Signed-off-by: Jun Kimura <[email protected]>

crates/ibc/src/client_state.rs

@@ -33,6 +33,11 @@ use ibc_proto::protobuf::Protobuf;
 use prost::Message;
 use serde::{Deserialize, Serialize};
 
+/// The revision number for the Ethereum light client is always 0.
+///
+/// Therefore, in ethereum, the revision number is not used to determine the hard fork.
+/// The current fork is determined by the client state's fork parameters.
+pub const ETHEREUM_CLIENT_REVISION_NUMBER: u64 = 0;
 pub const ETHEREUM_CLIENT_STATE_TYPE_URL: &str = "/ibc.lightclients.ethereum.v1.ClientState";
 pub const ETHEREUM_ACCOUNT_STORAGE_ROOT_INDEX: usize = 2;
 
@@ -163,15 +168,18 @@ impl<const SYNC_COMMITTEE_SIZE: usize> ClientState<SYNC_COMMITTEE_SIZE> {
 
     pub fn verify_membership(
         &self,
+        proof_height: ibc::Height,
         _counterparty_prefix: &ibc::core::ics23_commitment::commitment::CommitmentPrefix,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         path: impl Into<Path>,
         value: Vec<u8>,
     ) -> Result<(), ClientError> {
+        self.verify_height(proof_height)?;
         let proof = decode_eip1184_rlp_proof(proof.clone().into())?;
         let path = path.into();
         let root = H256::from_slice(root.as_bytes());
+        // if root is zero, the IBC contract has not been initialized yet
         if root.is_zero() {
             return Err(ClientError::ClientSpecific {
                 description: format!(
@@ -199,14 +207,17 @@ impl<const SYNC_COMMITTEE_SIZE: usize> ClientState<SYNC_COMMITTEE_SIZE> {
 
     pub fn verify_non_membership(
         &self,
+        proof_height: ibc::Height,
         _counterparty_prefix: &ibc::core::ics23_commitment::commitment::CommitmentPrefix,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         path: impl Into<Path>,
     ) -> Result<(), ibc::core::ics02_client::error::ClientError> {
+        self.verify_height(proof_height)?;
         let proof = decode_eip1184_rlp_proof(proof.clone().into())?;
         let path = path.into();
         let root = H256::from_slice(root.as_bytes());
+        // if root is zero, the IBC contract has not been initialized yet
         if root.is_zero() {
             return Err(ClientError::ClientSpecific {
                 description: format!(
@@ -227,21 +238,27 @@ impl<const SYNC_COMMITTEE_SIZE: usize> ClientState<SYNC_COMMITTEE_SIZE> {
         Ok(())
     }
 
-    /// Verify that the client is at a sufficient height and unfrozen at the given height
+    /// Verify that the client is at a sufficient height and unfrozen
     pub fn verify_height(&self, height: Height) -> Result<(), Error> {
+        if height.revision_number() != ETHEREUM_CLIENT_REVISION_NUMBER {
+            return Err(Error::UnexpectedHeightRevisionNumber {
+                expected: ETHEREUM_CLIENT_REVISION_NUMBER,
+                got: height.revision_number(),
+            });
+        }
+        if self.is_frozen() {
+            return Err(Error::ClientFrozen {
+                frozen_height: self.frozen_height.unwrap(),
+                target_height: height,
+            });
+        }
         if self.latest_height() < height {
             return Err(Error::InsufficientHeight {
                 latest_height: self.latest_height(),
                 target_height: height,
             });
         }
-        match self.frozen_height {
-            Some(frozen_height) if frozen_height <= height => Err(Error::ClientFrozen {
-                frozen_height,
-                target_height: height,
-            }),
-            _ => Ok(()),
-        }
+        Ok(())
     }
 
     pub fn validate(&self) -> Result<(), Error> {
@@ -289,7 +306,11 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
     }
 
     fn latest_height(&self) -> Height {
-        Height::new(0, self.latest_execution_block_number.into()).unwrap()
+        Height::new(
+            ETHEREUM_CLIENT_REVISION_NUMBER,
+            self.latest_execution_block_number.into(),
+        )
+        .unwrap()
     }
 
     fn frozen_height(&self) -> Option<Height> {
@@ -315,6 +336,9 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         client_id: ClientId,
         header: Any,
     ) -> Result<UpdatedState, ClientError> {
+        if self.is_frozen() {
+            return Err(ClientError::ClientFrozen { client_id });
+        }
         let cc = self.build_context(ctx);
         let header = Header::<SYNC_COMMITTEE_SIZE>::try_from(header)?;
         header.validate(&cc)?;
@@ -395,7 +419,11 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         client_id: ClientId,
         misbehaviour: Any,
     ) -> Result<alloc::boxed::Box<dyn Ics2ClientState>, ibc::core::ContextError> {
+        if self.is_frozen() {
+            return Err(ClientError::ClientFrozen { client_id }.into());
+        }
         let misbehaviour = Misbehaviour::<SYNC_COMMITTEE_SIZE>::try_from(misbehaviour)?;
+        misbehaviour.validate()?;
         let consensus_state = match maybe_consensus_state(
             ctx,
             &ClientConsensusStatePath::new(&client_id, &misbehaviour.trusted_sync_committee.height),
@@ -448,13 +476,11 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         client_cons_state_path: &ibc::core::ics24_host::path::ClientConsensusStatePath,
         expected_consensus_state: &dyn ibc::core::ics02_client::consensus_state::ConsensusState,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(proof_height)?;
-
         let value = expected_consensus_state
             .encode_vec()
             .map_err(ClientError::InvalidAnyConsensusState)?;
         self.verify_membership(
+            proof_height,
             counterparty_prefix,
             proof,
             root,
@@ -472,13 +498,11 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         counterparty_conn_path: &ibc::core::ics24_host::path::ConnectionPath,
         expected_counterparty_connection_end: &ibc::core::ics03_connection::connection::ConnectionEnd,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(proof_height)?;
-
         let value = expected_counterparty_connection_end
             .encode_vec()
             .map_err(ClientError::InvalidConnectionEnd)?;
         self.verify_membership(
+            proof_height,
             counterparty_prefix,
             proof,
             root,
@@ -496,14 +520,12 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         counterparty_chan_end_path: &ibc::core::ics24_host::path::ChannelEndPath,
         expected_counterparty_channel_end: &ibc::core::ics04_channel::channel::ChannelEnd,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(proof_height)?;
-
         let value = expected_counterparty_channel_end
             .encode_vec()
             .map_err(ClientError::InvalidChannelEnd)?;
 
         self.verify_membership(
+            proof_height,
             counterparty_prefix,
             proof,
             root,
@@ -521,12 +543,10 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
         client_state_path: &ibc::core::ics24_host::path::ClientStatePath,
         expected_client_state: Any,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(proof_height)?;
-
         let value = expected_client_state.encode_to_vec();
 
         self.verify_membership(
+            proof_height,
             counterparty_prefix,
             proof,
             root,
@@ -538,17 +558,15 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
     fn verify_packet_data(
         &self,
         _ctx: &dyn ibc::core::ValidationContext,
-        height: ibc::Height,
+        proof_height: ibc::Height,
         connection_end: &ibc::core::ics03_connection::connection::ConnectionEnd,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         commitment_path: &ibc::core::ics24_host::path::CommitmentPath,
         commitment: ibc::core::ics04_channel::commitment::PacketCommitment,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(height)?;
-
         self.verify_membership(
+            proof_height,
             connection_end.counterparty().prefix(),
             proof,
             root,
@@ -560,17 +578,15 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
     fn verify_packet_acknowledgement(
         &self,
         _ctx: &dyn ibc::core::ValidationContext,
-        height: ibc::Height,
+        proof_height: ibc::Height,
         connection_end: &ibc::core::ics03_connection::connection::ConnectionEnd,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         ack_path: &ibc::core::ics24_host::path::AckPath,
         ack: ibc::core::ics04_channel::commitment::AcknowledgementCommitment,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(height)?;
-
         self.verify_membership(
+            proof_height,
             connection_end.counterparty().prefix(),
             proof,
             root,
@@ -582,22 +598,20 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
     fn verify_next_sequence_recv(
         &self,
         _ctx: &dyn ibc::core::ValidationContext,
-        height: ibc::Height,
+        proof_height: ibc::Height,
         connection_end: &ibc::core::ics03_connection::connection::ConnectionEnd,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         seq_recv_path: &ibc::core::ics24_host::path::SeqRecvPath,
         sequence: ibc::core::ics04_channel::packet::Sequence,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(height)?;
-
         let mut seq_bytes = Vec::new();
         u64::from(sequence)
             .encode(&mut seq_bytes)
             .expect("buffer size too small");
 
         self.verify_membership(
+            proof_height,
             connection_end.counterparty().prefix(),
             proof,
             root,
@@ -609,16 +623,14 @@ impl<const SYNC_COMMITTEE_SIZE: usize> Ics2ClientState for ClientState<SYNC_COMM
     fn verify_packet_receipt_absence(
         &self,
         _ctx: &dyn ibc::core::ValidationContext,
-        height: ibc::Height,
+        proof_height: ibc::Height,
         connection_end: &ibc::core::ics03_connection::connection::ConnectionEnd,
         proof: &ibc::core::ics23_commitment::commitment::CommitmentProofBytes,
         root: &ibc::core::ics23_commitment::commitment::CommitmentRoot,
         receipt_path: &ibc::core::ics24_host::path::ReceiptPath,
     ) -> Result<(), ClientError> {
-        let client_state = downcast_eth_client_state::<SYNC_COMMITTEE_SIZE>(self)?;
-        client_state.verify_height(height)?;
-
         self.verify_non_membership(
+            proof_height,
             connection_end.counterparty().prefix(),
             proof,
             root,
@@ -884,16 +896,6 @@ impl<const SYNC_COMMITTEE_SIZE: usize> From<ClientState<SYNC_COMMITTEE_SIZE>> fo
     }
 }
 
-fn downcast_eth_client_state<const SYNC_COMMITTEE_SIZE: usize>(
-    cs: &dyn Ics2ClientState,
-) -> Result<&ClientState<SYNC_COMMITTEE_SIZE>, ClientError> {
-    cs.as_any()
-        .downcast_ref::<ClientState<SYNC_COMMITTEE_SIZE>>()
-        .ok_or_else(|| ClientError::ClientArgsTypeMismatch {
-            client_type: eth_client_type(),
-        })
-}
-
 fn downcast_eth_consensus_state(
     cs: &dyn Ics02ConsensusState,
 ) -> Result<ConsensusState, ClientError> {

crates/ibc/src/errors.rs

@@ -57,6 +57,8 @@ pub enum Error {
         latest_height: Height,
         target_height: Height,
     },
+    /// the height's revision number is unexpected: expected=`{expected}` got=`{got}`
+    UnexpectedHeightRevisionNumber { expected: u64, got: u64 },
     /// unexpected timestamp: expected={0} got={1}
     UnexpectedTimestamp(i128, i128),
     /// missing trusting period

crates/ibc/src/header.rs

@@ -77,7 +77,7 @@ pub fn decode_header<const SYNC_COMMITTEE_SIZE: usize, B: Buf>(
 
 impl<const SYNC_COMMITTEE_SIZE: usize> Header<SYNC_COMMITTEE_SIZE> {
     pub fn validate<C: ChainContext>(&self, ctx: &C) -> Result<(), Error> {
-        self.trusted_sync_committee.sync_committee.validate()?;
+        self.trusted_sync_committee.validate()?;
         if self.timestamp.into_tm_time().is_none() {
             return Err(Error::ZeroTimestampError);
         }
@@ -198,6 +198,7 @@ impl<const SYNC_COMMITTEE_SIZE: usize> From<Header<SYNC_COMMITTEE_SIZE>> for IBC
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::client_state::ETHEREUM_CLIENT_REVISION_NUMBER;
     use ethereum_consensus::context::ChainContext;
     use ethereum_consensus::{config, types::U64};
     use ethereum_light_client_verifier::{
@@ -246,7 +247,7 @@ mod tests {
             let update = to_consensus_update_info(update);
             let header = Header {
                 trusted_sync_committee: TrustedSyncCommittee {
-                    height: ibc::Height::new(1, 1).unwrap(),
+                    height: ibc::Height::new(ETHEREUM_CLIENT_REVISION_NUMBER, 1).unwrap(),
                     sync_committee: current_sync_committee.to_committee().clone(),
                     is_next: true,
                 },
@@ -267,7 +268,7 @@ mod tests {
 
             let header = Header {
                 trusted_sync_committee: TrustedSyncCommittee {
-                    height: ibc::Height::new(1, 1).unwrap(),
+                    height: ibc::Height::new(ETHEREUM_CLIENT_REVISION_NUMBER, 1).unwrap(),
                     sync_committee: current_sync_committee.to_committee().clone(),
                     is_next: true,
                 },
@@ -296,7 +297,7 @@ mod tests {
         let update = to_consensus_update_info(update);
         let header = Header {
             trusted_sync_committee: TrustedSyncCommittee {
-                height: ibc::Height::new(1, 1).unwrap(),
+                height: ibc::Height::new(ETHEREUM_CLIENT_REVISION_NUMBER, 1).unwrap(),
                 sync_committee: current_sync_committee.to_committee().clone(),
                 is_next: true,
             },

crates/ibc/src/misbehaviour.rs

@@ -36,6 +36,13 @@ pub struct Misbehaviour<const SYNC_COMMITTEE_SIZE: usize> {
     pub data: MisbehaviourData<SYNC_COMMITTEE_SIZE, ConsensusUpdateInfo<SYNC_COMMITTEE_SIZE>>,
 }
 
+impl<const SYNC_COMMITTEE_SIZE: usize> Misbehaviour<SYNC_COMMITTEE_SIZE> {
+    pub fn validate(&self) -> Result<(), Error> {
+        self.trusted_sync_committee.validate()?;
+        Ok(())
+    }
+}
+
 impl<const SYNC_COMMITTEE_SIZE: usize> Ics02Misbehaviour for Misbehaviour<SYNC_COMMITTEE_SIZE> {
     fn client_id(&self) -> &ibc::core::ics24_host::identifier::ClientId {
         &self.client_id

crates/ibc/src/types.rs

@@ -1,3 +1,4 @@
+use crate::client_state::ETHEREUM_CLIENT_REVISION_NUMBER;
 use crate::commitment::decode_eip1184_rlp_proof;
 use crate::errors::Error;
 use crate::internal_prelude::*;
@@ -110,6 +111,19 @@ pub struct TrustedSyncCommittee<const SYNC_COMMITTEE_SIZE: usize> {
     pub is_next: bool,
 }
 
+impl<const SYNC_COMMITTEE_SIZE: usize> TrustedSyncCommittee<SYNC_COMMITTEE_SIZE> {
+    pub fn validate(&self) -> Result<(), Error> {
+        if self.height.revision_number() != ETHEREUM_CLIENT_REVISION_NUMBER {
+            return Err(Error::UnexpectedHeightRevisionNumber {
+                expected: ETHEREUM_CLIENT_REVISION_NUMBER,
+                got: self.height.revision_number(),
+            });
+        }
+        self.sync_committee.validate()?;
+        Ok(())
+    }
+}
+
 impl<const SYNC_COMMITTEE_SIZE: usize> TryFrom<ProtoTrustedSyncCommittee>
     for TrustedSyncCommittee<SYNC_COMMITTEE_SIZE>
 {