From e5cf2f8429a20e9acc1e6d6c8561a14b71d3eaa3 Mon Sep 17 00:00:00 2001 From: woodser Date: Wed, 21 Dec 2022 11:00:40 +0000 Subject: [PATCH] fix potential double spend by locking wallet when thawing outputs --- .../core/btc/wallet/XmrWalletService.java | 27 +++++++++++++++-- .../bisq/core/offer/OpenOfferManager.java | 2 +- .../java/bisq/core/trade/TradeManager.java | 29 ++++++++++--------- .../tasks/MaybeSendSignContractRequest.java | 8 ----- 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/wallet/XmrWalletService.java b/core/src/main/java/bisq/core/btc/wallet/XmrWalletService.java index 456db74691..a679751c2b 100644 --- a/core/src/main/java/bisq/core/btc/wallet/XmrWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/XmrWalletService.java @@ -30,6 +30,7 @@ import java.math.BigDecimal; import java.math.BigInteger; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -287,6 +288,17 @@ public class XmrWalletService { } } + /** + * Thaw the given outputs with a lock on the wallet. + * + * @param keyImages the key images to thaw + */ + public void thawOutputs(Collection keyImages) { + synchronized (getWallet()) { + for (String keyImage : keyImages) wallet.thawOutput(keyImage); + } + } + /** * Create the reserve tx and freeze its inputs. The full amount is returned * to the sender's payout address less the trade fee. @@ -314,8 +326,19 @@ public class XmrWalletService { BigInteger tradeFee = HavenoUtils.coinToAtomicUnits(trade instanceof MakerTrade ? trade.getOffer().getMakerFee() : trade.getTakerFee()); BigInteger peerAmount = HavenoUtils.coinToAtomicUnits(trade instanceof BuyerTrade ? Coin.ZERO : offer.getAmount()); BigInteger securityDeposit = HavenoUtils.coinToAtomicUnits(trade instanceof BuyerTrade ? offer.getBuyerSecurityDeposit() : offer.getSellerSecurityDeposit()); - log.info("Creating deposit tx with fee={}, peerAmount={}, securityDeposit={}", tradeFee, peerAmount, securityDeposit); - return createTradeTx(tradeFee, peerAmount, securityDeposit, multisigAddress); + + // thaw reserved outputs then create deposit tx + MoneroWallet wallet = getWallet(); + synchronized (wallet) { + + // thaw reserved outputs + if (trade.getSelf().getReserveTxKeyImages() != null) { + thawOutputs(trade.getSelf().getReserveTxKeyImages()); + } + + log.info("Creating deposit tx with fee={}, peerAmount={}, securityDeposit={}", tradeFee, peerAmount, securityDeposit); + return createTradeTx(tradeFee, peerAmount, securityDeposit, multisigAddress); + } } private MoneroTxWallet createTradeTx(BigInteger tradeFee, BigInteger peerAmount, BigInteger securityDeposit, String address) { diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index 72fb56e89e..6e6fb3bbab 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -587,7 +587,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe private void onRemoved(@NotNull OpenOffer openOffer) { Offer offer = openOffer.getOffer(); if (offer.getOfferPayload().getReserveTxKeyImages() != null) { - for (String frozenKeyImage : offer.getOfferPayload().getReserveTxKeyImages()) xmrWalletService.getWallet().thawOutput(frozenKeyImage); + xmrWalletService.thawOutputs(offer.getOfferPayload().getReserveTxKeyImages()); xmrWalletService.saveMainWallet(); } offer.setState(Offer.State.REMOVED); diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index 56fbf7d3e9..5d4e078a3f 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -306,18 +306,18 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi } // thaw unreserved outputs - Set frozenKeyImages = xmrWalletService.getWallet().getOutputs(new MoneroOutputQuery() + Set unreservedFrozenKeyImages = xmrWalletService.getWallet().getOutputs(new MoneroOutputQuery() .setIsFrozen(true) .setIsSpent(false)) .stream() .map(output -> output.getKeyImage().getHex()) .collect(Collectors.toSet()); - frozenKeyImages.removeAll(reservedKeyImages); - for (String unreservedFrozenKeyImage : frozenKeyImages) { - log.info("Thawing output which is not reserved for offer or trade: " + unreservedFrozenKeyImage); - xmrWalletService.getWallet().thawOutput(unreservedFrozenKeyImage); + unreservedFrozenKeyImages.removeAll(reservedKeyImages); + if (!unreservedFrozenKeyImages.isEmpty()) { + log.info("Thawing outputs which are not reserved for offer or trade: " + unreservedFrozenKeyImages); + xmrWalletService.thawOutputs(unreservedFrozenKeyImages); + xmrWalletService.saveMainWallet(); } - if (!frozenKeyImages.isEmpty()) xmrWalletService.saveMainWallet(); } public TradeProtocol getTradeProtocol(Trade trade) { @@ -1059,21 +1059,22 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi synchronized(tradableList) { if (!tradableList.contains(trade)) return; - // skip if trade wallet possibly funded + // unreserve key images + if (trade instanceof TakerTrade && trade.getSelf().getReserveTxKeyImages() != null) { + xmrWalletService.thawOutputs(trade.getSelf().getReserveTxKeyImages()); + xmrWalletService.saveMainWallet(); + trade.getSelf().setReserveTxKeyImages(null); + } + + // stop if trade wallet possibly funded if (xmrWalletService.multisigWalletExists(trade.getId()) && trade.isDepositRequested()) { - log.warn("Not removing trade {} because trade wallet could be funded", trade.getId()); + log.warn("Refusing to delete {} {} because trade wallet could be funded", trade.getClass().getSimpleName(), trade.getId()); return; } // delete trade wallet if exists if (xmrWalletService.multisigWalletExists(trade.getId())) trade.deleteWallet(); - // unreserve key images - if (trade instanceof TakerTrade && trade.getSelf().getReserveTxKeyImages() != null) { - for (String keyImage : trade.getSelf().getReserveTxKeyImages()) xmrWalletService.getWallet().thawOutput(keyImage); - xmrWalletService.saveMainWallet(); - } - // remove trade removeTrade(trade); } diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/MaybeSendSignContractRequest.java b/core/src/main/java/bisq/core/trade/protocol/tasks/MaybeSendSignContractRequest.java index e0551d5676..89e7e30453 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/MaybeSendSignContractRequest.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/MaybeSendSignContractRequest.java @@ -66,17 +66,9 @@ public class MaybeSendSignContractRequest extends TradeTask { return; } - // thaw reserved outputs - MoneroWallet wallet = trade.getXmrWalletService().getWallet(); - for (String reserveTxKeyImage : trade.getSelf().getReserveTxKeyImages()) { - wallet.thawOutput(reserveTxKeyImage); - } - // create deposit tx and freeze inputs MoneroTxWallet depositTx = trade.getXmrWalletService().createDepositTx(trade); - // TODO (woodser): save frozen key images and unfreeze if trade fails before deposited to multisig - // save process state processModel.setDepositTxXmr(depositTx); // TODO: trade.getSelf().setDepositTx() trade.getSelf().setDepositTxHash(depositTx.getHash());