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

staking: validator definitions with conflicting consensus keys #3858

Closed
erwanor opened this issue Feb 21, 2024 · 7 comments
Closed

staking: validator definitions with conflicting consensus keys #3858

erwanor opened this issue Feb 21, 2024 · 7 comments
Assignees
Labels
A-staking Area: Design and implementation of staking and delegation liveness _P-high High priority zellic-component-needs-remediation Bug reports by zellic
Milestone

Comments

@erwanor
Copy link
Member

erwanor commented Feb 21, 2024

Is your feature request related to a problem? Please describe.
Similar to #3855, Zellic found that it is possible to craft a transaction with two actions that refer to distinct identity keys but conflicting consensus keys. This is a high-priority fix, because CometBFT will halt if presented with duplicate consensus identities.

@erwanor erwanor added A-staking Area: Design and implementation of staking and delegation zellic-component-needs-remediation Bug reports by zellic labels Feb 21, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Feb 21, 2024
@erwanor erwanor added liveness _P-V1 Priority: slated for V1 release _P-high High priority and removed _P-V1 Priority: slated for V1 release labels Feb 21, 2024
@aubrika aubrika added this to the Sprint 1 milestone Feb 26, 2024
@hdevalence
Copy link
Member

We already have logic to prevent duplicate consensus keys. Did it not work here?

@erwanor
Copy link
Member Author

erwanor commented Feb 26, 2024

@hdevalence This is another TOCTOU hazard, where it's possible to have two actions that use different identity keys (= different definitions) but colliding consensus keys.

@zbuc
Copy link
Member

zbuc commented Feb 26, 2024

We check the consensus key isn't a duplicate in the verify_stateful method for the ValidatorDefinition action so this is a little surprising. I'll try to replicate

@hdevalence
Copy link
Member

verify_stateful is precisely where the TOCTOU comes from — its parallelized

@hdevalence
Copy link
Member

see eg the comments in the actionhandler impl in #3883

@hdevalence
Copy link
Member

maybe we should rename the method later

@cronokirby cronokirby moved this from 🗄️ Backlog to Todo in Penumbra Feb 27, 2024
@hdevalence
Copy link
Member

We will do #3960 instead.

@hdevalence hdevalence closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Penumbra Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staking Area: Design and implementation of staking and delegation liveness _P-high High priority zellic-component-needs-remediation Bug reports by zellic
Projects
Archived in project
Development

No branches or pull requests

5 participants