From 84ca712f59dd574a5568b0801ec10c737d760b67 Mon Sep 17 00:00:00 2001 From: Ava Howell Date: Tue, 21 May 2024 14:31:29 -0700 Subject: [PATCH] ibc: audit fixes (#4386) ## Describe your changes This PR contains a set of fixes for the higher sev issues from the recent audit of the penumbra-ibc codebase, that are specific to the state machine side of things (as opposed to `ibc-types`, there will be a follow up PR for ibc-types and a new release for the relevant fixes there). ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: closes #4421 closes #4420 closes #4419 closes #4418 --- .../component/msg_handler/channel_open_ack.rs | 2 +- .../component/msg_handler/connection_open_ack.rs | 7 +++---- .../ibc/src/component/msg_handler/timeout.rs | 2 +- .../src/component/msg_handler/upgrade_client.rs | 2 +- .../ibc/src/component/proof_verification.rs | 16 ++++++++++++---- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs b/crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs index 9f1c88a94a..3e7accd673 100644 --- a/crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs +++ b/crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs @@ -110,7 +110,7 @@ impl MsgHandler for MsgChannelOpenAck { } fn channel_state_is_correct(channel: &ChannelEnd) -> anyhow::Result<()> { - if channel.state == ChannelState::Init || channel.state == ChannelState::TryOpen { + if channel.state == ChannelState::Init { Ok(()) } else { Err(anyhow::anyhow!("channel is not in the correct state")) diff --git a/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs b/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs index cd896d7bea..661d9feb6c 100644 --- a/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs +++ b/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs @@ -230,11 +230,10 @@ async fn verify_previous_connection( // see // https://github.com/cosmos/ibc/blob/master/spec/core/ics-003-connection-semantics/README.md + // // for this validation logic - let state_is_consistent = connection.state_matches(&State::Init) - && connection.versions.contains(&msg.version) - || connection.state_matches(&State::TryOpen) - && connection.versions.get(0).eq(&Some(&msg.version)); + let state_is_consistent = + connection.state_matches(&State::Init) && connection.versions.contains(&msg.version); if !state_is_consistent { anyhow::bail!("connection is not in the correct state"); diff --git a/crates/core/component/ibc/src/component/msg_handler/timeout.rs b/crates/core/component/ibc/src/component/msg_handler/timeout.rs index 687695364e..7951e8f5b0 100644 --- a/crates/core/component/ibc/src/component/msg_handler/timeout.rs +++ b/crates/core/component/ibc/src/component/msg_handler/timeout.rs @@ -83,7 +83,7 @@ impl MsgHandler for MsgTimeout { if channel.ordering == ChannelOrder::Ordered { // ordered channel: check that packet has not been received - if self.next_seq_recv_on_b != self.packet.sequence { + if self.next_seq_recv_on_b > self.packet.sequence { anyhow::bail!("packet sequence number does not match"); } diff --git a/crates/core/component/ibc/src/component/msg_handler/upgrade_client.rs b/crates/core/component/ibc/src/component/msg_handler/upgrade_client.rs index 5009298b33..c32031d61b 100644 --- a/crates/core/component/ibc/src/component/msg_handler/upgrade_client.rs +++ b/crates/core/component/ibc/src/component/msg_handler/upgrade_client.rs @@ -58,7 +58,7 @@ impl MsgHandler for MsgUpgradeClient { .context("couldn't decode proof for upgraded client state")?; state - .verify_client_upgrade_proof( + .verify_client_upgrade_proof::( &self.client_id, &proof_client_state, &proof_consensus_state, diff --git a/crates/core/component/ibc/src/component/proof_verification.rs b/crates/core/component/ibc/src/component/proof_verification.rs index ebc2d65bf2..013c847a1b 100644 --- a/crates/core/component/ibc/src/component/proof_verification.rs +++ b/crates/core/component/ibc/src/component/proof_verification.rs @@ -109,8 +109,8 @@ fn verify_merkle_proof( } #[async_trait] -pub trait ClientUpgradeProofVerifier: StateReadExt { - async fn verify_client_upgrade_proof( +pub trait ClientUpgradeProofVerifier: StateReadExt + Sized { + async fn verify_client_upgrade_proof( &self, client_id: &ClientId, client_state_proof: &MerkleProof, @@ -127,8 +127,8 @@ pub trait ClientUpgradeProofVerifier: StateReadExt { anyhow::bail!("upgrade path is not set"); }; - let upgrade_path_prefix = MerklePrefix::try_from(upgrade_path[0].clone().into_bytes()) - .map_err(|_| { + let upgrade_path_prefix = + MerklePrefix::try_from(upgrade_path.clone().concat().into_bytes()).map_err(|_| { anyhow::anyhow!("couldn't create commitment prefix from client upgrade path") })?; @@ -142,6 +142,14 @@ pub trait ClientUpgradeProofVerifier: StateReadExt { .get_verified_consensus_state(&trusted_client_state.latest_height(), client_id) .await?; + // check that the client is not expired + let now = HI::get_block_timestamp(&self).await?; + let time_elapsed = now.duration_since(trusted_consensus_state.timestamp)?; + + if trusted_client_state.expired(time_elapsed) { + anyhow::bail!("client is expired"); + } + verify_merkle_proof( &trusted_client_state.proof_specs, &upgrade_path_prefix,