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

Jump to bottom

fix(electrum): bdk_electrum exploit fixes #1675

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

fix(electrum): bdk_electrum exploit fixes #1675

wants to merge 4 commits into from

Conversation

LagginTimes
Copy link
Contributor

Description

The aim of this PR is to fix a few exploits that were found within bdk_electrum. Most notably:

  • Transaction Merkle proof verification does not check the Merkle proof for the coinbase. This enables an attack by which an attacker can fake a deeply-confirmed payment to a BDK wallet. (https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710#merkle-tree-attacks-using-64-bytes-transactions-8)
  • An Electrum server can trigger a panic in populate_with_txids by returning a transaction with no output.
  • fetch_prev_txout should not try to query the prevouts of coinbase transactions. This will query a transaction to the Electrum server which will return an error and will make the overall sync fail.

Notes to the reviewers

Changelog notice

TODO

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@LagginTimes
fix(electrum): validate coinbase merkle path
0eb24da 
This fix validates the coinbase merkle path when checking the merkle
proof of a transaction we are inserting within the same block. This check
mitigates a known brute force exploit that could insert an invalid
transaction.
@LagginTimes
fix(electrum): prevent fetch_prev_txout from querying coinbase tran…
8fc7faa 
…sactions

\`fetch_prev_txout\` should not try to query the prevouts of coinbase
transactions, as it may result in the Electrum server returning an error
which would cause the overall `sync` to fail.
@LagginTimes LagginTimes marked this pull request as draft November 6, 2024 17:40
@LagginTimes LagginTimes changed the title fix(electrum): bdk_electrum exploit fixes draft fix(electrum): bdk_electrum exploit fixes Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
BDK
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@LagginTimes