Skip to content

Commit

Permalink
Add missing output value check to takerSignsDepositTx
Browse files Browse the repository at this point in the history
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 bisq-network#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.
  • Loading branch information
stejbac committed Nov 26, 2020
1 parent 99d5652 commit d59b425
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
17 changes: 12 additions & 5 deletions core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<RawTransactionInput> buyerInputs,
List<RawTransactionInput> sellerInputs,
byte[] buyerPubKey,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()),
Expand All @@ -82,6 +87,7 @@ protected void run() {
false,
contractHash,
processModel.getPreparedDepositTx(),
msOutputAmount,
buyerInputs,
sellerInputs,
buyerMultiSigPubKey,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down

0 comments on commit d59b425

Please sign in to comment.