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

Verify signatures before bumping a transaction #352

Closed
LeoComandini opened this issue May 25, 2021 · 4 comments · Fixed by #355
Closed

Verify signatures before bumping a transaction #352

LeoComandini opened this issue May 25, 2021 · 4 comments · Fixed by #355
Assignees

Comments

@LeoComandini
Copy link

When bumping a transaction, one may assume that its outputs were previously confirmed by the user.
However a malicious server can tweak unconfirmed transactions so that bumping them will result in sending the funds elsewhere.

Alice broadcasts tx A, in1 -> out1, which won't be confirmed.
Alice restores her wallet.
Server returns tx B, in1 -> out2, instead of tx A.
Alice bumps tx B, but instead of sending funds to out1, funds are sent to out2.

A possible solution consists in verifying the signatures of the transaction to be replaced.
For instance, unless unusual sighashes were used, tx B will fail verification, while tx A will pass.

This attack may be relevant for wallets relying on Electrum servers.
Although in most cases (I assume) the transaction is persisted when it's sent, so this attack is not viable, IMO verifying signatures before bumping is still worth the performance penalty.

@RCasatta
Copy link
Member

Agree we should check, I think it's even better to validate the script (which internally verify the signature) using https://github.com/rust-bitcoin/rust-bitcoinconsensus/blob/master/src/lib.rs#L132

@afilini
Copy link
Member

afilini commented May 26, 2021

Yeah this makes a lot of sense, and I think having the code to verify signatures/script execution is gonna come in handy for other things as well (like making sure that all the signatures are valid before finalizing a transaction)

@afilini afilini self-assigned this May 26, 2021
@LLFourn
Copy link
Contributor

LLFourn commented May 27, 2021

I don't really understand the attack here. It sounds like the system is just signing whatever the server send it which is not going to be secure no matter what you do.

Would be handy to be able to verify a input's witness being signing it but I don't get the scenario being described here.

@afilini
Copy link
Member

afilini commented May 27, 2021

If you restore a wallet on a new device using electrum, BDK will sync and download all the txs from the server, including unconfirmed ones. Then when trying to bump a tx we just look at the old one that we are bumping and create the same outputs on the new one.

If the server lies and sends us an invalid tx (unsigned) with different outputs compared to the real one we might use that as the "model" for our fee bump, thus creating a valid (signed) tx with the wrong outputs. It can be detected by, for instance, noticing that the txid doesn't match what you would expect or that the outputs are wrong, but the average user would probably just see an unconfirmed tx and bump it without noticing any of that.

afilini added a commit to afilini/bdk that referenced this issue May 27, 2021
Verify the unconfirmed transactions we download agains the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
afilini added a commit to afilini/bdk that referenced this issue May 27, 2021
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
@afilini afilini added this to the Release 0.9.0 Feature Freeze milestone Jun 18, 2021
afilini added a commit to afilini/bdk that referenced this issue Jun 18, 2021
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
afilini added a commit to afilini/bdk that referenced this issue Jun 18, 2021
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
afilini added a commit to afilini/bdk that referenced this issue Jun 18, 2021
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
afilini added a commit to afilini/bdk that referenced this issue Jul 1, 2021
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.

Closes bitcoindevkit#352
@afilini afilini closed this as completed in a186d82 Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants