From a49611a234f617476982e739848fea1b26372138 Mon Sep 17 00:00:00 2001 From: woodser Date: Sun, 15 Jan 2023 17:29:19 -0500 Subject: [PATCH] verify payout & dispute payout tx fees are within range of recreated tx --- .../core/btc/wallet/TradeWalletService.java | 1 + .../core/support/dispute/DisputeManager.java | 18 +++++++------ .../arbitration/ArbitrationManager.java | 17 ++++++++++-- core/src/main/java/bisq/core/trade/Trade.java | 26 +++++++++++++++---- .../windows/DisputeSummaryWindow.java | 2 +- 5 files changed, 48 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java b/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java index 922c913d..d1af93e8 100644 --- a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java @@ -698,6 +698,7 @@ public class TradeWalletService { * @throws TransactionVerificationException if there was an unexpected problem with the payout tx or its signatures * @throws WalletException if the seller's wallet is null or structurally inconsistent */ + @Deprecated public Transaction sellerSignsAndFinalizesPayoutTx(Transaction depositTx, byte[] buyerSignature, Coin buyerPayoutAmount, diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java index a8f42fa5..b81e7cff 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java @@ -701,7 +701,7 @@ public abstract class DisputeManager> extends Sup if (trade == null) throw new RuntimeException("Dispute trade " + dispute.getTradeId() + " does not exist"); // create dispute payout tx if not given - if (payoutTx == null) payoutTx = createDisputePayoutTx(trade, dispute, disputeResult); // can be null if already published or we don't have receiver's multisig hex + if (payoutTx == null) payoutTx = createDisputePayoutTx(trade, dispute, disputeResult, false); // can be null if already published or we don't have receiver's multisig hex // persist result in dispute's chat message ChatMessage chatMessage = new ChatMessage( @@ -804,7 +804,7 @@ public abstract class DisputeManager> extends Sup // Utils /////////////////////////////////////////////////////////////////////////////////////////// - public MoneroTxWallet createDisputePayoutTx(Trade trade, Dispute dispute, DisputeResult disputeResult) { + public MoneroTxWallet createDisputePayoutTx(Trade trade, Dispute dispute, DisputeResult disputeResult, boolean skipMultisigImport) { // sync and save wallet trade.syncWallet(); @@ -813,14 +813,16 @@ public abstract class DisputeManager> extends Sup // create unsigned dispute payout tx if not already published and arbitrator has trader's updated multisig info TradingPeer receiver = trade.getTradingPeer(dispute.getTraderPubKeyRing()); if (!trade.isPayoutPublished() && receiver.getUpdatedMultisigHex() != null) { + MoneroWallet multisigWallet = trade.getWallet(); // import multisig hex - MoneroWallet multisigWallet = trade.getWallet(); - List updatedMultisigHexes = new ArrayList(); - if (trade.getBuyer().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getBuyer().getUpdatedMultisigHex()); - if (trade.getSeller().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getSeller().getUpdatedMultisigHex()); - if (!updatedMultisigHexes.isEmpty()) { - multisigWallet.importMultisigHex(updatedMultisigHexes.toArray(new String[0])); // TODO (monero-project): fails if multisig hex imported individually + if (!skipMultisigImport) { + List updatedMultisigHexes = new ArrayList(); + if (trade.getBuyer().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getBuyer().getUpdatedMultisigHex()); + if (trade.getSeller().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getSeller().getUpdatedMultisigHex()); + if (!updatedMultisigHexes.isEmpty()) { + multisigWallet.importMultisigHex(updatedMultisigHexes.toArray(new String[0])); // TODO (monero-project): fails if multisig hex imported individually + } } // sync and save wallet diff --git a/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java b/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java index 8a830a28..7188fcd4 100644 --- a/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java +++ b/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java @@ -329,8 +329,6 @@ public final class ArbitrationManager extends DisputeManager XmrWalletService.MINER_FEE_TOLERANCE) throw new RuntimeException("Miner fee is not within " + (XmrWalletService.MINER_FEE_TOLERANCE * 100) + "% of estimated fee, expected " + feeEstimate + " but was " + arbitratorSignedPayoutTx.getFee()); + log.info("Payout tx fee {} is within tolerance, diff %={}", arbitratorSignedPayoutTx.getFee(), feeDiff); + } + // submit fully signed payout tx to the network List txHashes = multisigWallet.submitMultisigTxHex(signedTxSet.getMultisigTxHex()); signedTxSet.getTxs().get(0).setHash(txHashes.get(0)); // manually update hash which is known after signed diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index f5bbef42..d089b5bf 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -716,7 +716,6 @@ public abstract class Trade implements Tradable, Model { BigInteger sellerPayoutAmount = sellerDepositAmount.subtract(tradeAmount); // create transaction to get fee estimate - if (multisigWallet.isMultisigImportNeeded()) throw new RuntimeException("Cannot create payout tx because multisig import is needed"); MoneroTxWallet feeEstimateTx = multisigWallet.createTx(new MoneroTxConfig() .setAccountIndex(0) .addDestination(buyerPayoutAddress, buyerPayoutAmount.multiply(BigInteger.valueOf(9)).divide(BigInteger.valueOf(10))) // reduce payment amount to compute fee of similar tx @@ -798,17 +797,34 @@ public abstract class Trade implements Tradable, Model { BigInteger expectedSellerPayout = sellerDepositAmount.subtract(tradeAmount).subtract(txCost.divide(BigInteger.valueOf(2))); if (!sellerPayoutDestination.getAmount().equals(expectedSellerPayout)) throw new RuntimeException("Seller destination amount is not deposit amount - trade amount - 1/2 tx costs, " + sellerPayoutDestination.getAmount() + " vs " + expectedSellerPayout); - // TODO (woodser): verify fee is reasonable (e.g. within 2x of fee estimate tx) - - // sign payout tx + // handle tx signing if (sign) { + + // sign tx MoneroMultisigSignResult result = multisigWallet.signMultisigTxHex(payoutTxHex); if (result.getSignedMultisigTxHex() == null) throw new RuntimeException("Error signing payout tx"); payoutTxHex = result.getSignedMultisigTxHex(); + describedTxSet = multisigWallet.describeMultisigTxSet(payoutTxHex); // update described set + payoutTx = describedTxSet.getTxs().get(0); + + // verify fee is within tolerance by recreating payout tx + // TODO (monero-project): creating tx will require exchanging updated multisig hex if message needs reprocessed. provide weight with describe_transfer so fee can be estimated? + MoneroTxWallet feeEstimateTx = null; + try { + feeEstimateTx = createPayoutTx(); + } catch (Exception e) { + log.warn("Could not recreate payout tx to verify fee: " + e.getMessage()); + } + if (feeEstimateTx != null) { + BigInteger feeEstimate = feeEstimateTx.getFee(); + double feeDiff = payoutTx.getFee().subtract(feeEstimate).abs().doubleValue() / feeEstimate.doubleValue(); // TODO: use BigDecimal? + if (feeDiff > XmrWalletService.MINER_FEE_TOLERANCE) throw new RuntimeException("Miner fee is not within " + (XmrWalletService.MINER_FEE_TOLERANCE * 100) + "% of estimated fee, expected " + feeEstimate + " but was " + payoutTx.getFee()); + log.info("Payout tx fee {} is within tolerance, diff %={}", payoutTx.getFee(), feeDiff); + } } // update trade state - setPayoutTx(describedTxSet.getTxs().get(0)); + setPayoutTx(payoutTx); setPayoutTxHex(payoutTxHex); // submit payout tx 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 index ef5ecdec..835b398d 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java @@ -583,7 +583,7 @@ public class DisputeSummaryWindow extends Overlay { closeTicketButton.setOnAction(e -> { // create payout tx - MoneroTxWallet payoutTx = arbitrationManager.createDisputePayoutTx(trade, dispute, disputeResult); + MoneroTxWallet payoutTx = arbitrationManager.createDisputePayoutTx(trade, dispute, disputeResult, false); // show confirmation if (dispute.getSupportType() == SupportType.ARBITRATION &&