[bisq-network/bisq] Improve Validation of Deposit Inputs (#5618)

[jimmyneutront]

@jimmyneutront commented on this pull request.



> @@ -356,23 +356,46 @@ public static void validatePayoutTxInput(Transaction depositTx,
     }
 
     public static void validateDepositInputs(Trade trade) throws InvalidTxException {
-        // assumption: deposit tx always has 2 inputs, the maker and taker
-        if (trade == null || trade.getDepositTx() == null || trade.getDepositTx().getInputs().size() != 2) {
-            throw new InvalidTxException("Deposit transaction is null or has unexpected input count");
+        /*
+         * deposit tx usually has 2 inputs, the maker and taker

This change will allow me to bring much more liquidity to Bisq; specifically: more sell offers at much better prices.

The deposit transaction may have more than two inputs in the following scenario:

The maker/seller publishes a sell offer, and pays the maker fee. Then, it gets an address, flags it as "RESERVED_FOR_TRADE", and associates it with the new sell offer's ID. However, it does NOT send the seller security deposit amount + offer amount to the address that has been flagged as "RESERVED_FOR_TRADE" and associated with the new sell offer's ID.

The taker/buyer attempts to take the offer, sending an InputsForDepositTxRequest to the maker/seller, and awaits a InputsForDepositTxResponse.

As soon as the maker/seller receives the InputsForDepositTxRequest, it completes the MakerProcessesInputsForDepositTxRequest TradeTask. Then, it sends the seller security deposit amount + offer amount to the address that has been flagged as "RESERVED_FOR_TRADE" and associated with the id of the offer being taken. It may send multiple transactions that sum to this amount, and thus the deposit transaction will have more than 2 inputs as a result. (The maker/seller can purchase an amount of BTC equal to the seller security deposit amount + offer amount from a centralized exchange, and then immediately withdraw it to the address flagged as "RESERVED_FOR_TRADE" and associated with the id of the offer being taken.)  

Then, the maker/seller completes the following TradeTasks:
ApplyFilter
VerifyPeersAccountAgeWitness
MakerVerifyTakerFeePayment (although this does nothing)
MakerSetsLockTime
MakerCreateAndSignContract

Then, the maker/seller completes the SellerAsMakerCreatesUnsignedDepositTx TradeTask, and in the process it uses UTXOs at the "RESERVED_FOR_TRADE" address associated with the id of the offer being taken to fund the deposit transaction. Since the security deposit amount + offer amount may have been sent to that address split across multiple transactions (as described 2 paragraphs above) the deposit transaction will thus have more than 2 inputs, though it will otherwise be perfectly valid. The maker/seller will then complete the SellerAsMakerSendsInputsForDepositTxResponse TradeTask, sending an InputsForDepositTxResponse to the taker/buyer.

As soon as the taker/buyer receives the InputsForDepositTxResponse, it completes the following TradeTasks:
ApplyFilter
VerifyPeersAccountAgeWitness
TakerVerifyAndSignContract
TakerPublishFeeTx (this may be problematic, read on for details)
	
Then, the taker/buyer completes the BuyerAsTakerSignsDepositTx TradeTask, and in the process it verifies that the unsigned deposit transaction received from the maker/seller is properly formed. If the unsigned deposit transaction is not properly formed, it throws an exception. However, in this case, no exception is thrown, because the deposit transaction created by the maker/seller is valid. BuyerAsTakerSignsDepositTx doesn't count the number of inputs to the deposit transaction, because it is irrelevant. The only code that performs this action is the validateDepositInputs method in the TradeDataValidation class, and again, it is an unnecessary validation that is irrelevant. Furthermore, the validateDepositInputs function is only run AFTER the offer has been successfully taken and has become a trade; at that point, the taker/buyer has already paid the taker fee and locked up the buyer security deposit amount.

After the taker/buyer completes the BuyerAsTakerSignsDepositTx TradeTask, it then completes the BuyerAsTakerSendsDepositTxMessage TradeTask, and the trade process carries on normally, with no problems.

Because the maker/seller funded the deposit transaction with UTXOs sent from an external address, the maker fee transaction id will not match any of the transaction ids used to fund the deposit transaction. Although again: this validation is unnecessary and irrelevant. If there is anything wrong with the deposit transaction that could cause the trade to fail, it will be discovered by the taker/buyer's BuyerAsTakerSignsDepositTx TradeTask. Furthermore, this validation is currently performed by validateDepositInputs only AFTER the offer has been successfully taken and has become a trade with no problems.

If the maker/seller doesn't fund the "RESERVED_FOR_TRADE" address associated with the id of the offer being taken and send an InputsForDepositTxResponse to the taker/buyer in time:
The offer taking process will timeout and the trade will fail. However, because the taker/buyer doesn't publish the taker fee transaction until AFTER an InputsForDepositTxResponse has been received, the seller will not lose anything.

If the maker/seller sends an invalid unsigned deposit transaction to the taker/buyer:
The trade will fail. Additionally, because the seller/taker completes the TakerPublishFeeTx TradeTask before it completes the BuyerAsTakerSignsDepositTx TradeTask, which verifies that the unsigned deposit transaction received from the maker/seller is valid, the taker/buyer may lose their taker fee, unless the maker/seller resolves the issue with their deposit transaction. However, we cannot be sure the maker/seller will do this. Therefore, I believe we should cause the BuyerAsTakerSignsDepositTx TradeTask to be completed before the TakerPublishFeeTx TradeTask is completed.

I know that much of the maker/seller's behavior that I have described here isn't possible with Bisq at the moment. However, I have a private Bisq repo where I am working on this functionality myself. Again: the change I am proposing poses no new risk to the offer taker. It simply removes an unnecessary validation step and will allow me to bring much more liquidity to Bisq.

Concerning the issue with the TakerPublishFeeTx TradeTask: I believe this is a separate issue that needs attention. A malicious user can send invalid unsigned deposit transactions to takers, causing takers to lose their taker fee amount. I will look into this further.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5618#discussion_r668173182
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20210712/14db7af0/attachment-0001.htm>