Check the value assertion only on valid amounts#1134
Merged
Conversation
This causes crash on elements wallet when dealing with transactions that have explicit values and confidential assets. This creates a somewhat serious DoS attack as the sender can cause the reciever's wallet to crash by partially blinding the change output. To make matters worse, the wallet initially accepts the transaction, but fails while spending the output. This is likely caused by a combination of two bugs: 1) The wallet's current behaviour stores the complete transaction of interest in CWalletTx instead of just Outpoints. Only that the spend time do we iterate over all outputs, try to unblind them and check which are isMine. When calling wtx.GetOutputValueOut() or similar calls, we hit this assertion. While the current behaviour is okay, I think the correct way is move the IsMine == ISMINE_NO at the start of the loop. We should not do be any checks on outputs that are not ours. This is used in multiple places at different parts of the codebase for different RPCs. 2) When dealing with partially blinded trasactions, ComputeBlindingData correctly sets value = -1, and the cache byte to 1. When getting the data again with GetBlindingData for explicit value and confidential asset, we load the precomputed data with value = -1 and assert the loaded value be the explicit value in the transaction. This is only true for explicit value and explicit asset. The changed assertion checks that written value should be same as the explicit value that was written only when the amounts are valid
delta1
changed the title
apoelstra
approved these changes
Aug 8, 2022
psgreco
added a commit
that referenced
this pull request
Aug 8, 2022
[elements-22.x] Backport #1134: Check the value assertion only on valid amounts
This was referenced Dec 5, 2022
gwillen
added a commit
to gwillen/elements
that referenced
this pull request
Mar 2, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1125
This causes crash on elements wallet when dealing with transactions that
have explicit values and confidential assets. This creates a somewhat
serious DoS attack as the sender can cause the reciever's wallet to
crash by partially blinding the change output. To make matters worse,
the wallet initially accepts the transaction, but fails while spending
the output.
This is likely caused by a combination of two bugs:
in CWalletTx instead of just Outpoints. Only that the spend time do we
iterate over all outputs, try to unblind them and check which are
isMine. When calling wtx.GetOutputValueOut() or similar calls, we hit this assertion.
While the current behaviour is okay, I think the correct way is move
the IsMine == ISMINE_NO at the start of the loop. We should not do be
any checks on outputs that are not ours. This is used in multiple
places at different parts of the codebase for different RPCs.
correctly sets value = -1, and the cache byte to 1. When getting the
data again with GetBlindingData for explicit value and confidential
asset, we load the precomputed data with value = -1 and assert the
loaded value be the explicit value in the transaction. This is only true
for explicit value and explicit asset.
The changed assertion checks that written value should be same as the
explicit value that was written only when the amounts are valid