Sanity check deposit tx output & receiver total before refunding

stejbac · stejbac · commit 8b9ff7f34c8d · 2025-01-19T19:45:19.000+08:00
Enhance the tx chain validation logic in 'DisputeSummaryWindow' to check
that the deposit tx funds no less than the total trade collateral of the
contract, and that the BM won't receive less than the proposed refund,
due to excessive tx fees. This is to guard against a malicious pair of
traders who could attempt to trick the refund agent into paying out more
than they deposited, by supplying a mismatched contract, or (less
feasibly) colluding with a miner and using an unreasonably high mining
fee to steal some of the funds that should go to the BM.

Also verify that the DPT output address matches the donation address in
the 'Dispute' DTO, for any dispute using the legacy BM, as that option
is not forbidden for v4 protocol trades.

Finally, validate the txId string passed in the URL to mempool.space
from the methods 'MempoolHttpClient::(getTxDetails|requestTxAsHex)', for
safety and to avoid GET requests like '/null' or '/null/hex' if the txId
is null.
diff --git a/core/src/main/java/bisq/core/provider/MempoolHttpClient.java b/core/src/main/java/bisq/core/provider/MempoolHttpClient.java
@@ -22,6 +22,8 @@
 
 import bisq.common.app.Version;
 
+import org.bitcoinj.core.Sha256Hash;
+
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
@@ -41,7 +43,7 @@ public MempoolHttpClient(@Nullable Socks5ProxyProvider socks5ProxyProvider) {
     // returns JSON of the transaction details
     public String getTxDetails(String txId) throws IOException {
         super.shutDown(); // close any prior incomplete request
-        String api = "/" + txId;
+        String api = "/" + Sha256Hash.wrap(txId);
         return get(api, "User-Agent", "bisq/" + Version.VERSION);
     }
 
@@ -50,7 +52,7 @@ public CompletableFuture<String> requestTxAsHex(String txId) {
         super.shutDown(); // close any prior incomplete request
 
         return CompletableFuture.supplyAsync(() -> {
-            String api = "/" + txId + "/hex";
+            String api = "/" + Sha256Hash.wrap(txId) + "/hex";
             try {
                 return get(api, "User-Agent", "bisq/" + Version.VERSION);
             } catch (IOException e) {
diff --git a/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java b/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java
@@ -40,6 +40,7 @@
 import bisq.core.trade.ClosedTradableManager;
 import bisq.core.trade.TradeManager;
 import bisq.core.trade.bisq_v1.FailedTradesManager;
+import bisq.core.trade.model.bisq_v1.Contract;
 import bisq.core.trade.model.bisq_v1.Trade;
 import bisq.core.trade.protocol.bisq_v5.model.StagedPayoutTxParameters;
 
@@ -55,6 +56,7 @@
 import bisq.common.util.Hex;
 import bisq.common.util.Tuple2;
 
+import org.bitcoinj.core.Coin;
 import org.bitcoinj.core.NetworkParameters;
 import org.bitcoinj.core.Transaction;
 import org.bitcoinj.core.TransactionInput;
@@ -351,21 +353,34 @@ private static boolean firstOutputConnectsToFirstInput(Transaction parent, Trans
     }
 
     public void verifyDelayedPayoutTxReceivers(Transaction depositTx, Transaction delayedPayoutTx, Dispute dispute) {
-        long inputAmount = depositTx.getOutput(0).getValue().value;
-        int selectionHeight = dispute.getBurningManSelectionHeight();
-
-        List<Tuple2<Long, String>> receivers = delayedPayoutTxReceiverService.getReceivers(
-                selectionHeight,
-                inputAmount,
-                dispute.getTradeTxFee(),
-                DelayedPayoutTxReceiverService.ReceiverFlag.flagsActivatedBy(dispute.getTradeDate()));
-        log.info("Verify delayedPayoutTx using selectionHeight {} and receivers {}", selectionHeight, receivers);
-        checkArgument(delayedPayoutTx.getOutputs().size() == receivers.size(),
-                "Number of outputs must equal number of receivers");
-        checkOutputsPrefixMatchesReceivers(delayedPayoutTx, receivers);
+        if (dispute.isUsingLegacyBurningMan()) {
+            checkArgument(delayedPayoutTx.getOutputs().size() == 1,
+                    "delayedPayoutTx must have 1 output when using legacy BM");
+            NetworkParameters params = btcWalletService.getParams();
+            TransactionOutput transactionOutput = delayedPayoutTx.getOutput(0);
+            String outputAddress = transactionOutput.getScriptPubKey().getToAddress(params).toString();
+            checkArgument(outputAddress.equals(dispute.getDonationAddressOfDelayedPayoutTx()),
+                    "Output address does not match donation address (%s). transactionOutput=%s",
+                    dispute.getDonationAddressOfDelayedPayoutTx(), transactionOutput);
+        } else {
+            long inputAmount = depositTx.getOutput(0).getValue().value;
+            int selectionHeight = dispute.getBurningManSelectionHeight();
+
+            List<Tuple2<Long, String>> receivers = delayedPayoutTxReceiverService.getReceivers(
+                    selectionHeight,
+                    inputAmount,
+                    dispute.getTradeTxFee(),
+                    DelayedPayoutTxReceiverService.ReceiverFlag.flagsActivatedBy(dispute.getTradeDate()));
+            log.info("Verify delayedPayoutTx using selectionHeight {} and receivers {}", selectionHeight, receivers);
+            checkArgument(delayedPayoutTx.getOutputs().size() == receivers.size(),
+                    "Number of outputs must equal number of receivers");
+            checkOutputsPrefixMatchesReceivers(delayedPayoutTx, receivers);
+        }
     }
 
     public void verifyRedirectTxReceivers(Transaction warningTx, Transaction redirectTx, Dispute dispute) {
+        checkArgument(!dispute.isUsingLegacyBurningMan(), "Legacy BM use not permitted with redirectTx");
+
         long inputAmount = warningTx.getOutput(0).getValue().value;
         long inputAmountMinusFeeBumpAmount = inputAmount - StagedPayoutTxParameters.REDIRECT_TX_FEE_BUMP_OUTPUT_VALUE;
         int selectionHeight = dispute.getBurningManSelectionHeight();
@@ -389,11 +404,33 @@ private void checkOutputsPrefixMatchesReceivers(Transaction delayedPayoutOrRedir
             TransactionOutput transactionOutput = delayedPayoutOrRedirectTx.getOutput(i);
             Tuple2<Long, String> receiverTuple = receivers.get(i);
             checkArgument(transactionOutput.getScriptPubKey().getToAddress(params).toString().equals(receiverTuple.second),
-                    "Output address does not match receiver address (%s). transactionOutput=%s",
-                    receiverTuple.second, transactionOutput);
+                    "Output address does not match receiver address (%s). transactionOutput=%s, index=%s",
+                    receiverTuple.second, transactionOutput, i);
             checkArgument(transactionOutput.getValue().value == receiverTuple.first,
-                    "Output value does not match receiver value (%s). transactionOutput=%s",
-                    receiverTuple.first, transactionOutput);
+                    "Output value does not match receiver value (%s). transactionOutput=%s, index=%s",
+                    receiverTuple.first, transactionOutput, i);
         }
     }
+
+    public void validateCollateralAndPayoutTotals(Transaction depositTx,
+                                                  Transaction delayedPayoutOrRedirectTx,
+                                                  Dispute dispute,
+                                                  DisputeResult disputeResult) {
+        Contract contract = dispute.getContract();
+        Coin expectedCollateral = contract.getTradeAmount()
+                .add(Coin.valueOf(contract.getOfferPayload().getBuyerSecurityDeposit()))
+                .add(Coin.valueOf(contract.getOfferPayload().getSellerSecurityDeposit()));
+        Coin depositOutputValue = depositTx.getOutput(0).getValue();
+
+        checkArgument(!depositOutputValue.isLessThan(expectedCollateral),
+                "First depositTx output value (%s) is less than expected trade collateral: %s sats",
+                depositOutputValue, expectedCollateral);
+
+        Coin totalPayoutAmount = disputeResult.getBuyerPayoutAmount().add(disputeResult.getSellerPayoutAmount());
+        Coin totalReceiverPlusPossibleFeeBumpOutputAmount = delayedPayoutOrRedirectTx.getOutputSum();
+
+        checkArgument(!totalReceiverPlusPossibleFeeBumpOutputAmount.isLessThan(totalPayoutAmount),
+                "DPT/redirectTx output sum (%s) is less than proposed refund of %s sats to traders",
+                totalReceiverPlusPossibleFeeBumpOutputAmount, totalPayoutAmount);
+    }
 }
diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java
@@ -842,10 +842,6 @@ public void onFailure(TxBroadcastException exception) {
         }
     }
 
-    // TODO: We should probably check that the tx fees paid by the warning & redirect txs (or DPT for v4) don't reduce
-    //  the amount going to the BM by too much, say by no more than the trade security deposit amount, as the fees could
-    //  spike (or be manipulated) and in general the redirect tx pays out a final sum that will be less than the total
-    //  trade collateral (due to fees).
     private CompletableFuture<Boolean> maybeCheckTransactions() {
         final CompletableFuture<Boolean> asyncStatus = new CompletableFuture<>();
         var disputeManager = getDisputeManager();
@@ -862,16 +858,16 @@ private CompletableFuture<Boolean> maybeCheckTransactions() {
                 if (throwable == null) {
                     try {
                         refundManager.verifyTradeTxChain(txList);
-                        if (!dispute.isUsingLegacyBurningMan()) {
-                            Transaction depositTx = txList.get(2);
-                            if (txList.size() == 4) {
-                                Transaction delayedPayoutTx = txList.get(3);
-                                refundManager.verifyDelayedPayoutTxReceivers(depositTx, delayedPayoutTx, dispute);
-                            } else {
-                                Transaction warningTx = txList.get(3);
-                                Transaction redirectTx = txList.get(4);
-                                refundManager.verifyRedirectTxReceivers(warningTx, redirectTx, dispute);
-                            }
+                        Transaction depositTx = txList.get(2);
+                        if (txList.size() == 4) {
+                            Transaction delayedPayoutTx = txList.get(3);
+                            refundManager.verifyDelayedPayoutTxReceivers(depositTx, delayedPayoutTx, dispute);
+                            refundManager.validateCollateralAndPayoutTotals(depositTx, delayedPayoutTx, dispute, disputeResult);
+                        } else {
+                            Transaction warningTx = txList.get(3);
+                            Transaction redirectTx = txList.get(4);
+                            refundManager.verifyRedirectTxReceivers(warningTx, redirectTx, dispute);
+                            refundManager.validateCollateralAndPayoutTotals(depositTx, redirectTx, dispute, disputeResult);
                         }
                         asyncStatus.complete(true);
                     } catch (Throwable error) {
