From d59b425c948cbe1c305b047502cc4b673e21177c Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Thu, 26 Nov 2020 23:39:51 +0000 Subject: [PATCH] Add missing output value check to takerSignsDepositTx Make sure the taker checks the value of the 2-of-2 multisig output of the deposit tx created by the maker, before signing it. This avoids a potential security hole, where the maker attempts to steal most of the deposit by using the wrong output value and adding an extra 'change' output to himself. Note that there's no actual vulnerability at present, as the output value is indirectly checked via the validation of the delayed payout tx. In particular, the extra checks added in 345426f as part of #4789 (Fix remaining blackmail vulnerabilities) place a lower bound on the delayed payout tx input value and with it the deposit tx output value. However, explicitly checking the amount is more robust. --- .../core/btc/wallet/TradeWalletService.java | 17 ++++++++++++----- .../BuyerAsTakerSignsDepositTx.java | 6 ++++++ .../SellerAsTakerSignsDepositTx.java | 6 ++++++ 3 files changed, 24 insertions(+), 5 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 2b05f47c182..fbd80bc9741 100644 --- a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java @@ -565,19 +565,21 @@ private PreparedDepositTxAndMakerInputs makerCreatesDepositTx(boolean makerIsBuy * @param takerIsSeller the flag indicating if we are in the taker as seller role or the opposite * @param contractHash the hash of the contract to be added to the OP_RETURN output * @param makersDepositTxSerialized the prepared deposit transaction signed by the maker + * @param msOutputAmount the MultiSig output amount, as determined by the taker * @param buyerInputs the connected outputs for all inputs of the buyer * @param sellerInputs the connected outputs for all inputs of the seller * @param buyerPubKey the public key of the buyer * @param sellerPubKey the public key of the seller * @throws SigningException if (one of) the taker input(s) was of an unrecognized type for signing * @throws TransactionVerificationException if a non-P2WH maker-as-buyer input wasn't signed, the maker's MultiSig - * script or contract hash doesn't match the taker's, or there was an unexpected problem with the final deposit tx - * or its signatures + * script, contract hash or output amount doesn't match the taker's, or there was an unexpected problem with the + * final deposit tx or its signatures * @throws WalletException if the taker's wallet is null or structurally inconsistent */ public Transaction takerSignsDepositTx(boolean takerIsSeller, byte[] contractHash, byte[] makersDepositTxSerialized, + Coin msOutputAmount, List buyerInputs, List sellerInputs, byte[] buyerPubKey, @@ -588,10 +590,15 @@ public Transaction takerSignsDepositTx(boolean takerIsSeller, checkArgument(!buyerInputs.isEmpty()); checkArgument(!sellerInputs.isEmpty()); - // Check if maker's MultiSig script is identical to the takers + // Check if maker's MultiSig script is identical to the taker's Script hashedMultiSigOutputScript = get2of2MultiSigOutputScript(buyerPubKey, sellerPubKey, false); if (!makersDepositTx.getOutput(0).getScriptPubKey().equals(hashedMultiSigOutputScript)) { - throw new TransactionVerificationException("Maker's hashedMultiSigOutputScript does not match to takers hashedMultiSigOutputScript"); + throw new TransactionVerificationException("Maker's hashedMultiSigOutputScript does not match taker's hashedMultiSigOutputScript"); + } + + // Check if maker's MultiSig output value is identical to the taker's + if (!makersDepositTx.getOutput(0).getValue().equals(msOutputAmount)) { + throw new TransactionVerificationException("Maker's MultiSig output amount does not match taker's MultiSig output amount"); } // The outpoints are not available from the serialized makersDepositTx, so we cannot use that tx directly, but we use it to construct a new @@ -642,7 +649,7 @@ public Transaction takerSignsDepositTx(boolean takerIsSeller, TransactionOutput makersContractHashOutput = makersDepositTx.getOutputs().get(1); log.debug("makersContractHashOutput {}", makersContractHashOutput); if (!makersContractHashOutput.getScriptPubKey().equals(contractHashOutput.getScriptPubKey())) { - throw new TransactionVerificationException("Maker's transaction output for the contract hash is not matching takers version."); + throw new TransactionVerificationException("Maker's transaction output for the contract hash is not matching taker's version."); } // Add all outputs from makersDepositTx to depositTx diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java index 24f988120da..78e18c85c70 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java @@ -20,6 +20,7 @@ import bisq.core.btc.model.AddressEntry; import bisq.core.btc.model.RawTransactionInput; import bisq.core.btc.wallet.BtcWalletService; +import bisq.core.offer.Offer; import bisq.core.trade.Trade; import bisq.core.trade.protocol.TradingPeer; import bisq.core.trade.protocol.tasks.TradeTask; @@ -71,6 +72,10 @@ protected void run() { buyerMultiSigAddressEntry.setCoinLockedInMultiSig(buyerInput.subtract(trade.getTxFee().multiply(2))); walletService.saveAddressEntryList(); + Offer offer = trade.getOffer(); + Coin msOutputAmount = offer.getBuyerSecurityDeposit().add(offer.getSellerSecurityDeposit()).add(trade.getTxFee()) + .add(checkNotNull(trade.getTradeAmount())); + TradingPeer tradingPeer = processModel.getTradingPeer(); byte[] buyerMultiSigPubKey = processModel.getMyMultiSigPubKey(); checkArgument(Arrays.equals(buyerMultiSigPubKey, buyerMultiSigAddressEntry.getPubKey()), @@ -82,6 +87,7 @@ protected void run() { false, contractHash, processModel.getPreparedDepositTx(), + msOutputAmount, buyerInputs, sellerInputs, buyerMultiSigPubKey, diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java index 887cbcf7636..527edc69a88 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java @@ -20,6 +20,7 @@ import bisq.core.btc.model.AddressEntry; import bisq.core.btc.model.RawTransactionInput; import bisq.core.btc.wallet.BtcWalletService; +import bisq.core.offer.Offer; import bisq.core.trade.Contract; import bisq.core.trade.Trade; import bisq.core.trade.protocol.TradingPeer; @@ -69,12 +70,17 @@ protected void run() { sellerMultiSigAddressEntry.setCoinLockedInMultiSig(sellerInput.subtract(totalFee)); walletService.saveAddressEntryList(); + Offer offer = trade.getOffer(); + Coin msOutputAmount = offer.getBuyerSecurityDeposit().add(offer.getSellerSecurityDeposit()).add(trade.getTxFee()) + .add(checkNotNull(trade.getTradeAmount())); + TradingPeer tradingPeer = processModel.getTradingPeer(); Transaction depositTx = processModel.getTradeWalletService().takerSignsDepositTx( true, trade.getContractHash(), processModel.getPreparedDepositTx(), + msOutputAmount, checkNotNull(tradingPeer.getRawTransactionInputs()), sellerInputs, tradingPeer.getMultiSigPubKey(),