Merge pull request #5889 from stejbac/fix-bsq-swap-tx-fee-theft-vulne…

…rability

Fix BSQ swap buyer tx fee theft vulnerability

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 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 RawTransactionInput(TransactionInput input) {
                 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,22 @@ public boolean isP2WSH() {
         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);
+        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

core/src/main/java/bisq/core/trade/bsq_swap/BsqSwapTakeOfferRequestVerification.java

@@ -77,7 +77,7 @@ public static boolean isValid(OpenOfferManager openOfferManager,
             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 @@ private static boolean isDateInTolerance(BsqSwapRequest request) {
     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) {

core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/buyer/ProcessBsqSwapFinalizeTxRequest.java

@@ -19,6 +19,7 @@
 
 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,19 +67,27 @@ protected void run() {
             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<RawTransactionInput> 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<RawTransactionInput> buyersBsqInputs = protocolModel.getInputs();
             int buyersInputSize = Objects.requireNonNull(buyersBsqInputs).size();
             List<TransactionInput> sellersBtcInputs = sellersTransaction.getInputs().stream()
                     .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 @@ protected void run() {
             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");

core/src/main/java/bisq/core/trade/protocol/bsq_swap/tasks/seller/ProcessTxInputsMessage.java

@@ -62,16 +62,18 @@ protected void run() {
             TxInputsMessage message = checkNotNull((TxInputsMessage) protocolModel.getTradeMessage());
             checkNotNull(message);
 
+            BtcWalletService btcWalletService = protocolModel.getBtcWalletService();
+
             List<RawTransactionInput> 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();
             checkArgument(sumInputs >= buyersBsqInputAmount,
                     "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(