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

Fix taker use all BSQ for fee payment #4354

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

sqrrm
Copy link
Member

@sqrrm sqrrm commented Jul 6, 2020

Fixes https://github.com/bisq-network/security/issues/4

Issue: if a taker used exactly all BSQ from the BSQ inputs to pay the
trading fee, there was no BSQ change in the takeOfferFeeTx. It was
assumed that the second output was the reservedForTrade output, but in
the case of missing BSQ change it was the first output.

Fix: added a check to make sure the value of the inputs to the deposit
tx match the expected inputAmount.

Added a check that if there is no BSQ outputs in the bsqTradingFeeTx a
change output is added of value 1 satoshi more than the BSQ input value.
This ensures that the second output is always the reservedForTrade
output. It also ensures that the BSQ is burnt, even in the very unlikely
case that the amount of BSQ burnt is larger than the reservedForTrade
amount.

Issue: if a taker used exactly all BSQ from the BSQ inputs to pay the
trading fee, there was no BSQ change in the takeOfferFeeTx. It was
assumed that the second output was the reservedForTrade output, but in
the case of missing BSQ change it was the first output.

Fix: added a check to make sure the value of the inputs to the deposit
tx match the expected inputAmount.

Added a check that if there is no BSQ outputs in the bsqTradingFeeTx a
change output is added of value 1 satoshi more than the BSQ input value.
This ensures that the second output is always the reservedForTrade
output. It also ensures that the BSQ is burnt, even in the very unlikely
case that the amount of BSQ burnt is larger than the reservedForTrade
amount.
@sqrrm sqrrm requested a review from ripcurlx July 6, 2020 17:19
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

I tested the use-case described on regtest spending all remaining BSQ in my wallet and everything worked as expected. I also tested buyer as taker with BTC and BSQ fee without spending all BSQ and all worked as expected as well.

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.

2 participants