Skip to content

Commit

Permalink
ibc: audit fixes (#4386)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
avahowell authored May 21, 2024
1 parent 1999d65 commit 84ca712
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,10 @@ async fn verify_previous_connection<S: StateRead>(

// 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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::<HI>(
&self.client_id,
&proof_client_state,
&proof_consensus_state,
Expand Down
16 changes: 12 additions & 4 deletions crates/core/component/ibc/src/component/proof_verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<HI: HostInterface>(
&self,
client_id: &ClientId,
client_state_proof: &MerkleProof,
Expand All @@ -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")
})?;

Expand All @@ -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,
Expand Down

0 comments on commit 84ca712

Please sign in to comment.