Skip to content

[elements-22.x] Backport #1134: Check the value assertion only on valid amounts#1135

Merged
psgreco merged 1 commit into
ElementsProject:elements-22.xfrom
gwillen:release-backports-22
Aug 8, 2022
Merged

[elements-22.x] Backport #1134: Check the value assertion only on valid amounts#1135
psgreco merged 1 commit into
ElementsProject:elements-22.xfrom
gwillen:release-backports-22

Conversation

@gwillen
Copy link
Copy Markdown
Contributor

@gwillen gwillen commented Aug 8, 2022

(Backport of #1134)

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.

  1. 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

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

(cherry picked from commit 53a75eb)
@gwillen gwillen force-pushed the release-backports-22 branch from 634a2c0 to c30f9a3 Compare August 8, 2022 20:35
@jhfrontz
Copy link
Copy Markdown
Contributor

jhfrontz commented Aug 8, 2022

Compared favorably with original PR.
utack c30f9a3

@psgreco psgreco merged commit 9ee071f into ElementsProject:elements-22.x Aug 8, 2022
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 this pull request may close these issues.

4 participants