Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ibc: audit fixes #4386

Merged
merged 4 commits into from
May 21, 2024
Merged

ibc: audit fixes #4386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
269ab19
accept timeout proofs for any past packet
avahowell May 14, 2024
9f4abd0
check client expiry in verify_client_upgrade_proof
avahowell May 14, 2024
c4e51bc
fix connection+channel ack: only acccept acks on INIT states
avahowell May 14, 2024
3c38d2a
fix 4420: Upgrade-path length hardcoded to two
avahowell May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs
View file
Open in desktop
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
7 changes: 3 additions & 4 deletions crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs
View file
Open in desktop
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);
cronokirby marked this conversation as resolved.
Show resolved Hide resolved

if !state_is_consistent {
anyhow::bail!("connection is not in the correct state");
Expand Down
2 changes: 1 addition & 1 deletion crates/core/component/ibc/src/component/msg_handler/timeout.rs
View file
Open in desktop
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 {
cronokirby marked this conversation as resolved.
Show resolved Hide resolved
anyhow::bail!("packet sequence number does not match");
}

Expand Down
2 changes: 1 addition & 1 deletion crates/core/component/ibc/src/component/msg_handler/upgrade_client.rs
View file
Open in desktop
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
View file
Open in desktop
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?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, something to flag is that i noticed the other host chain interface methods are not used (or rust-analyzer can't seem to find them)

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
Loading