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.

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

merged 4 commits into from
May 21, 2024

Conversation

avahowell
Copy link
Contributor

@avahowell avahowell commented May 14, 2024

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

  • 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

@avahowell avahowell added security Issues or work related to security. consensus-breaking breaking change to execution of on-chain data labels May 14, 2024
@avahowell avahowell marked this pull request as draft May 14, 2024 22:45
@avahowell avahowell marked this pull request as ready for review May 20, 2024 19:40
@avahowell avahowell changed the title (wip) ibc: audit fixes ibc: audit fixes May 20, 2024
@cratelyn cratelyn added this to the Sprint 7 milestone May 20, 2024
@aubrika aubrika requested a review from cronokirby May 20, 2024 20:09
@cratelyn cratelyn added the A-IBC Area: IBC integration with Penumbra label May 20, 2024
@@ -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)

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Looks good, just have a few questions which could be good to answer here for posterity

@erwanor erwanor added the zellic-ibc-remediated Remediation for a Zellic IBC audit finding label May 21, 2024
@conorsch
Copy link
Contributor

We believe that this PR may resolve a lingering blocker for interchaintest, where timeouts were not being handled properly. Let's include this for Testnet 76 (#4402), so downstream can consume it, and press on with the testing integration. Before I mash the merge button, @avahowell could you respond to @cronokirby's comments above? It'd be helpful to have the narrative when we refer back to this changeset.

@conorsch conorsch merged commit 84ca712 into main May 21, 2024
13 checks passed
@conorsch conorsch deleted the ibc-audit-fixes branch May 21, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-IBC Area: IBC integration with Penumbra consensus-breaking breaking change to execution of on-chain data security Issues or work related to security. zellic-ibc-remediated Remediation for a Zellic IBC audit finding
Projects
Status: Done
5 participants