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

Apply proposer boost to first block in case of equivocation #3352

Merged
merged 4 commits into from Sep 12, 2023

Conversation

michaelsproul
Copy link
Contributor

@michaelsproul michaelsproul commented May 11, 2023

This PR modifies on_block so that it awards the proposer boost to the first block processed rather than the last block processed. This would cleanly mitigate the RPC unbundling attack described in this post: https://lighthouse-blog.sigmaprime.io/mev-unbundling-rpc.html

Because clients already exhibit a variety of behaviours in this case due to their attestation strategy and handling of RPC blocks, we might be able to roll this change out without a hard fork. If some clients implement the change while others do not, the worst case is that views may be split in the case of an equivocation, which can happen anyway.

Alternatives:

  • Headlock: https://ethresear.ch/t/equivocation-attacks-in-mev-boost-and-epbs/.
  • A scheme to remove attestation weight from equivocating blocks. The naive version of this is broken and exploitable by the attacker, but a restricted version might work. Something like: if the head block is an equivocation, set its weight to 0 and/or do not allow it to become head. In most cases we want to choose the parent of the equivocating head instead, but specifying this might be complicated.

TODO:

  • Add tests
  • Consider whether on_block should be idempotent if called again with the same block

@ppopth
Copy link
Member

ppopth commented May 22, 2023

I haven't read the attack yet and I'm not an expert in this field, but it's quite cleaner to set proposer_boost_root only once per slot.

@adiasg what do you think?

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

While other existing solutions may provide a better long term fix, this patch is way better than status quo

@adiasg
Copy link
Contributor

adiasg commented May 23, 2023

I agree with setting the proposer_boost_root only once per slot. This change fixes a bug the proposer boost design -- should have been how we spec-ed proposer boost to begin with.

@adiasg adiasg added the scope:security General protocol security-related items label May 23, 2023
@hwwhww
Copy link
Contributor

hwwhww commented Aug 4, 2023

Hey @michaelsproul @adiasg

I added a test case today to verify the modification.

What's the status of this PR? Do we need a security analysis to verify if clients can update it uncoordinatedly or not?

@adiasg
Copy link
Contributor

adiasg commented Aug 4, 2023

This change does not need coordination. If a malicious proposer makes multiple proposals, it may cause attesters from that slot to be split between those blocks - but as long as the next proposer is honest, everyone's fork choice will converge again. For a prolonged attack, the attacker needs to control many proposers in a range of slots.

Additionally, exploiting this requires the attacker to bear some slashing.

@michaelsproul
Copy link
Contributor Author

@hwwhww I agree with Aditya that this is safe to roll out gradually. Thank you for writing the test 😊

I also think the idempotence concern from my OP is taken care of by the current impl: if the same block is re-applied to fork choice then it won't set the proposer boost again, but the proposer boost should already be set to this block from the first time it was applied.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

This adjustment looks good to me. What do you think about bringing it to ACDC so we can merge it properly?

@dapplion
Copy link
Collaborator

dapplion commented Sep 11, 2023

Any reason to not merge this PR? All clients have this logic incorporated for multiple months, spec should reflect reality unless there's a strong security consideration

This was brough up on ACDC 115, with notes ethereum/pm#844 (comment)

FC proposer boost equivocation fix to "last call"

  • no objection, will merge this PR by end of the week

@michaelsproul
Copy link
Contributor Author

michaelsproul commented Sep 11, 2023

@dapplion I agree we should merge!

@hwwhww hwwhww merged commit c5c7233 into ethereum:dev Sep 12, 2023
26 checks passed
@michaelsproul michaelsproul deleted the boost-first-block branch September 12, 2023 03:37
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Sep 19, 2023
Implement spec changes to fork choice; this only affects equivocation
when multiple blocks are signed for the same slot. Regular operation
is not changed.

- ethereum/consensus-specs#3352
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Sep 20, 2023
* implement EIP-7514 for Deneb: Add Max Epoch Churn Limit

Cap activations per epoch according to EIP-7514:

- https://eips.ethereum.org/EIPS/eip-7514
- ethereum/consensus-specs#3499

* apply proposer boost to first block in case of equivocation

Implement spec changes to fork choice; this only affects equivocation
when multiple blocks are signed for the same slot. Regular operation
is not changed.

- ethereum/consensus-specs#3352

* bump test vectors to v1.4.0-beta.2-hotfix

---------

Co-authored-by: tersec <tersec@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fork-choice scope:security General protocol security-related items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants