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

Check that IBC handlers don't short-circuit misbehavior handling #4209

Closed
hdevalence opened this issue Apr 15, 2024 · 3 comments
Closed

Check that IBC handlers don't short-circuit misbehavior handling #4209

hdevalence opened this issue Apr 15, 2024 · 3 comments
Assignees
Labels
A-IBC Area: IBC integration with Penumbra needs-refinement unclear, incomplete, or stub issue that needs work _P-high High priority _P-V1 Priority: slated for V1 release security Issues or work related to security.

Comments

@hdevalence
Copy link
Member

Describe the bug

While discussing cosmos/ibc-go#6159 , I mentioned our IBC implementation's short-circuit behavior, and @srdtrk pointed out that there's a possibility that this causes us to not handle misbehavior correctly:

The reason why we don’t quit so early like penumbra is because we check if UpdateMsg actually is evidence of misbehavior. (The check should be done imo)

We should audit the code and ensure that our short-circuiting behavior doesn't prevent this.

A fix for this issue is either:

  • An explanation of why we handle misbehavior correctly despite our short-circuiting (if this is the case)
  • A fix to handle misbehavior correctly (if it was not correct)
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra Apr 15, 2024
@hdevalence hdevalence added the security Issues or work related to security. label Apr 15, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Apr 15, 2024
@cratelyn cratelyn added the A-IBC Area: IBC integration with Penumbra label Apr 15, 2024
@srdtrk
Copy link

srdtrk commented Apr 16, 2024

In ibc-go, we have a legacy message server for misbehavior that is deprecated. We now use MsgUpdateClient to check for misbehavior when there is a duplicate client update with conflicting headers.

I see that penumbra has a non-legacy misbehavior handler. Nevertheless, it is better to do this check in update client so that misbehavior is handled even if the relayer doesn't know that there is a misbehavior.

@aubrika aubrika added _P-V1 Priority: slated for V1 release _P-high High priority labels Apr 17, 2024
@avahowell
Copy link
Contributor

We do perform this check for misbehavior (a duplicate header with different state) in UpdateClient:

// if we have a stored consensus state for this height that conflicts, we need to freeze

@avahowell
Copy link
Contributor

avahowell commented Apr 17, 2024

The short circuiting behavior for client updates will only short circuit in the event where we already have a header for the provided height, and the consensus state for the stored header is identical to the one contained in the update:

Ok(stored_tm_consensus_state == untrusted_consensus_state)

So, if a relayer provides an equivocation to the update handler, update_is_already_committed will return false, and the rest of the update handler will continue to next_tendermint_state which will set the next state for the client as frozen due to the equivocation.

Closing this issue for now since I think this addresses the concern raised

@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra Apr 17, 2024
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 needs-refinement unclear, incomplete, or stub issue that needs work _P-high High priority _P-V1 Priority: slated for V1 release security Issues or work related to security.
Projects
Archived in project
Development

No branches or pull requests

5 participants