From 8d267520c8fc56867c204e5cfb13f5e93011d5a2 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 2 Dec 2021 14:54:57 +0000 Subject: [PATCH 1/3] Fix misleading comment: BSQ swap taker tx fee tolerance (Actual tolerance by maker of agreed tx fee is 50%, not 10%.) --- .../bsq_swap/BsqSwapTakeOfferRequestVerification.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/bisq/core/trade/bsq_swap/BsqSwapTakeOfferRequestVerification.java b/core/src/main/java/bisq/core/trade/bsq_swap/BsqSwapTakeOfferRequestVerification.java index 6918116728..a779a045b3 100644 --- a/core/src/main/java/bisq/core/trade/bsq_swap/BsqSwapTakeOfferRequestVerification.java +++ b/core/src/main/java/bisq/core/trade/bsq_swap/BsqSwapTakeOfferRequestVerification.java @@ -77,7 +77,7 @@ public class BsqSwapTakeOfferRequestVerification { checkArgument(isDateInTolerance(request), "Trade date is out of tolerance"); checkArgument(isTxFeeInTolerance(request, feeService), "Miner fee from taker not in tolerance"); checkArgument(request.getMakerFee() == Objects.requireNonNull(CoinUtil.getMakerFee(false, amountAsCoin)).value); - checkArgument(request.getTakerFee() == CoinUtil.getTakerFee(false, amountAsCoin).value); + checkArgument(request.getTakerFee() == Objects.requireNonNull(CoinUtil.getTakerFee(false, amountAsCoin)).value); } catch (Exception e) { log.error("BsqSwapTakeOfferRequestVerification failed. Request={}, peer={}, error={}", request, peer, e.toString()); return false; @@ -93,9 +93,9 @@ public class BsqSwapTakeOfferRequestVerification { private static boolean isTxFeeInTolerance(BsqSwapRequest request, FeeService feeService) { double myFee = (double) feeService.getTxFeePerVbyte().getValue(); double peersFee = (double) Coin.valueOf(request.getTxFeePerVbyte()).getValue(); - // Allow for 10% diff in mining fee, ie, maker will accept taker fee that's 10% - // off their own fee from service. Both parties will use the same fee while - // creating the bsq swap tx + // Allow for 50% diff in mining fee, ie, maker will accept taker fee that's less + // than 50% off their own fee from service (that is, 100% higher or 50% lower). + // Both parties will use the same fee while creating the bsq swap tx. double diff = abs(1 - myFee / peersFee); boolean isInTolerance = diff < 0.5; if (!isInTolerance) { From 915a79e6275a497b3966ad7a76505d22f478ca59 Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 2 Dec 2021 17:19:10 +0000 Subject: [PATCH 2/3] Fix BSQ swap buyer tx fee theft vulnerability Prevent the seller from stealing the combined tx fee as change by lying about the value of one or more of his BTC inputs, which are passed to the buyer as raw inputs in the 'BsqSwapFinalizeTxRequest' message. To this end, add a 'RawTransactionInput::validate' method to check the 'value' field against the output value of the respective spending tx and run it on every seller input in 'ProcessBsqSwapFinalizeTxRequest', so that the buyer is no longer just trusting those numbers. Additionally, check that the spending txIds from the raw BTC inputs supplied by the seller actually match those of his signed inputs in the accompanying partially signed tx, thus tying the raw input values to the seller's tx. --- .../core/btc/model/RawTransactionInput.java | 21 +++++++++++++++---- .../ProcessBsqSwapFinalizeTxRequest.java | 19 ++++++++++++----- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java b/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java index 0b2d74de36..073255da91 100644 --- a/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java +++ b/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java @@ -17,7 +17,7 @@ package bisq.core.btc.model; -import bisq.core.btc.wallet.BtcWalletService; +import bisq.core.btc.wallet.WalletService; import bisq.common.proto.network.NetworkPayload; import bisq.common.proto.persistable.PersistablePayload; @@ -36,6 +36,10 @@ import lombok.Getter; import javax.annotation.concurrent.Immutable; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkElementIndex; +import static com.google.common.base.Preconditions.checkNotNull; + @EqualsAndHashCode @Immutable @Getter @@ -56,7 +60,7 @@ public final class RawTransactionInput implements NetworkPayload, PersistablePay input.getConnectedOutput() != null && input.getConnectedOutput().getScriptPubKey() != null && input.getConnectedOutput().getScriptPubKey().getScriptType() != null ? - input.getConnectedOutput().getScriptPubKey().getScriptType().id : -1); + input.getConnectedOutput().getScriptPubKey().getScriptType().id : 0); } // Does not set the scriptTypeId. Use RawTransactionInput(TransactionInput input) for any new code. @@ -129,8 +133,17 @@ public final class RawTransactionInput implements NetworkPayload, PersistablePay return scriptTypeId == Script.ScriptType.P2WSH.id; } - public String getParentTxId(BtcWalletService btcWalletService) { - return btcWalletService.getTxFromSerializedTx(parentTransaction).getTxId().toString(); + public String getParentTxId(WalletService walletService) { + return walletService.getTxFromSerializedTx(parentTransaction).getTxId().toString(); + } + + public void validate(WalletService walletService) { + Transaction tx = walletService.getTxFromSerializedTx(checkNotNull(parentTransaction)); + checkArgument(index == (int) index, "Input index out of range."); + checkElementIndex((int) index, tx.getOutputs().size(), "Input index"); + long outputValue = tx.getOutput(index).getValue().value; + checkArgument(value == outputValue, + "Input value (%s) mismatches connected tx output value (%s).", value, outputValue); } @Override diff --git a/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/buyer/ProcessBsqSwapFinalizeTxRequest.java b/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/buyer/ProcessBsqSwapFinalizeTxRequest.java index e7a4ecd9fa..4ffe3b27f7 100644 --- a/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/buyer/ProcessBsqSwapFinalizeTxRequest.java +++ b/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/buyer/ProcessBsqSwapFinalizeTxRequest.java @@ -19,6 +19,7 @@ package bisq.core.trade.protocol.bsq_swap.tasks.buyer; import bisq.core.btc.model.RawTransactionInput; import bisq.core.btc.wallet.Restrictions; +import bisq.core.btc.wallet.WalletService; import bisq.core.trade.bsq_swap.BsqSwapCalculation; import bisq.core.trade.model.bsq_swap.BsqSwapTrade; import bisq.core.trade.protocol.bsq_swap.messages.BsqSwapFinalizeTxRequest; @@ -66,11 +67,13 @@ public abstract class ProcessBsqSwapFinalizeTxRequest extends BsqSwapTask { checkNotNull(request); Validator.checkTradeId(protocolModel.getOfferId(), request); - // We will use only the sellers buyersBsqInputs from the tx so we do not verify anything else + // We will use only the seller's BTC inputs from the tx, so we do not verify anything else. byte[] tx = request.getTx(); - Transaction sellersTransaction = protocolModel.getBtcWalletService().getTxFromSerializedTx(tx); + WalletService btcWalletService = protocolModel.getBtcWalletService(); + Transaction sellersTransaction = btcWalletService.getTxFromSerializedTx(tx); List sellersRawBtcInputs = request.getBtcInputs(); - checkArgument(!sellersRawBtcInputs.isEmpty(), "Sellers BTC buyersBsqInputs must not be empty"); + checkArgument(!sellersRawBtcInputs.isEmpty(), "SellersRawBtcInputs must not be empty"); + sellersRawBtcInputs.forEach(input -> input.validate(btcWalletService)); List buyersBsqInputs = protocolModel.getInputs(); int buyersInputSize = Objects.requireNonNull(buyersBsqInputs).size(); @@ -78,7 +81,13 @@ public abstract class ProcessBsqSwapFinalizeTxRequest extends BsqSwapTask { .filter(input -> input.getIndex() >= buyersInputSize) .collect(Collectors.toList()); checkArgument(sellersBtcInputs.size() == sellersRawBtcInputs.size(), - "Number of buyersBsqInputs in tx must match the number of sellersRawBtcInputs"); + "Number of sellersBtcInputs in tx must match the number of sellersRawBtcInputs"); + for (int i = 0; i < sellersBtcInputs.size(); i++) { + String parentTxId = sellersBtcInputs.get(i).getOutpoint().getHash().toString(); + String rawParentTxId = sellersRawBtcInputs.get(i).getParentTxId(btcWalletService); + checkArgument(parentTxId.equals(rawParentTxId), + "Spending tx mismatch between sellersBtcInputs and sellersRawBtcInputs at index %s", i); + } boolean hasUnSignedInputs = sellersBtcInputs.stream() .anyMatch(input -> input.getScriptSig() == null && !input.hasWitness()); @@ -113,7 +122,7 @@ public abstract class ProcessBsqSwapFinalizeTxRequest extends BsqSwapTask { checkArgument(change <= expectedChange, "Change must be smaller or equal to expectedChange"); - NetworkParameters params = protocolModel.getBtcWalletService().getParams(); + NetworkParameters params = btcWalletService.getParams(); String sellersBsqPayoutAddress = request.getBsqPayoutAddress(); checkNotNull(sellersBsqPayoutAddress, "sellersBsqPayoutAddress must not be null"); checkArgument(!sellersBsqPayoutAddress.isEmpty(), "sellersBsqPayoutAddress must not be empty"); From 0517facbc370658e955f2cb956cf6612f83a1aee Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 2 Dec 2021 18:25:53 +0000 Subject: [PATCH 3/3] Validate input script types to prevent BSQ swap tx fee underpaying Add a check of 'scriptTypeId' field, against the output of the spending tx, to the 'RawTransactionInput::validate' method. Also make the seller as well as the buyer validate each raw BSQ/BTC input received from the peer. This prevents either peer from claiming that any of their non-segwit inputs are segwit in order to underpay the tx fee. --- .../main/java/bisq/core/btc/model/RawTransactionInput.java | 5 +++++ .../bsq_swap/tasks/seller/ProcessTxInputsMessage.java | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java b/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java index 073255da91..72e9cbe653 100644 --- a/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java +++ b/core/src/main/java/bisq/core/btc/model/RawTransactionInput.java @@ -144,6 +144,11 @@ public final class RawTransactionInput implements NetworkPayload, PersistablePay long outputValue = tx.getOutput(index).getValue().value; checkArgument(value == outputValue, "Input value (%s) mismatches connected tx output value (%s).", value, outputValue); + var scriptPubKey = tx.getOutput(index).getScriptPubKey(); + var scriptType = scriptPubKey != null ? scriptPubKey.getScriptType() : null; + checkArgument(scriptTypeId <= 0 || scriptType != null && scriptType.id == scriptTypeId, + "Input scriptTypeId (%s) mismatches connected tx output scriptTypeId (%s.id = %s).", + scriptTypeId, scriptType, scriptType != null ? scriptType.id : 0); } @Override diff --git a/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/seller/ProcessTxInputsMessage.java b/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/seller/ProcessTxInputsMessage.java index b80a9d67a3..72f24f5234 100644 --- a/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/seller/ProcessTxInputsMessage.java +++ b/core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/seller/ProcessTxInputsMessage.java @@ -62,8 +62,11 @@ public abstract class ProcessTxInputsMessage extends BsqSwapTask { TxInputsMessage message = checkNotNull((TxInputsMessage) protocolModel.getTradeMessage()); checkNotNull(message); + BtcWalletService btcWalletService = protocolModel.getBtcWalletService(); + List inputs = message.getBsqInputs(); checkArgument(!inputs.isEmpty(), "Buyers BSQ inputs must not be empty"); + inputs.forEach(input -> input.validate(btcWalletService)); long sumInputs = inputs.stream().mapToLong(rawTransactionInput -> rawTransactionInput.value).sum(); long buyersBsqInputAmount = BsqSwapCalculation.getBuyersBsqInputValue(trade, getBuyersTradeFee()).getValue(); @@ -71,7 +74,6 @@ public abstract class ProcessTxInputsMessage extends BsqSwapTask { "Buyers BSQ input amount do not match our calculated required BSQ input amount"); DaoFacade daoFacade = protocolModel.getDaoFacade(); - BtcWalletService btcWalletService = protocolModel.getBtcWalletService(); long sumValidBsqInputValue = inputs.stream() .mapToLong(input -> daoFacade.getUnspentTxOutputValue(