[bisq-network/bisq] Add auto-confirm feature for Monero (#4458)

[James Cox]

Code review of onion-monero-blockchain-explorer
================================================

The purpose of this review is to determine if using this explorer to validate receipt of XMR funds poses any risk.

The `onion-monero-blockchain-explorer` provides a web-API to prove that funds have been sent to an address.

### tldr;

 `onion-monero-blockchain-explorer` is a thin wrapper over the monero library, providing access over a web API call.  There is no modification of the data in or out of the monero library via the single API call used by Bisq (`/api/outputs`).

----------

### Background


Two instances of the explorer are hosted by Bisq ops team and queried from around the globe by Bisq clients that have opted-in to the XMR tx auto confirm feature.  The feature removes the need for an XMR buyer to manually check that their wallet has received the funds, allowing those trades to complete automatically, paying out the BTC funds from the multisig.

How could an attacker use this tool to defraud?  An attacker in this case would be an XMR seller who does not actually send the funds or the correct amount of funds.  The explorer could indicate that funds have been sent to the queried address when indeed they have not.  But the explorer does not know what **amount** the user is expecting to receive so if it is to falsify the results it must have a way of knowing the amount to send in the response back to the client. (only an address, txId and viewkey are provided to the API call).  

So there would have to be a specific trigger to cause the fake result (expected amount) to be sent.  

The amount could be encoded into a fake txId/viewkey.  The attacker would generate the txId/viewkey in such a way that they are recognizable as such by the explorer, encoding the correct amount inside.  The explorer could extract the amount, returning a success status with amount back to the user.  

Or an old transaction could be used which has a matching amount, which the malicious explorer code would modify to look like a new transaction.

Either way, if there was malicious code included in `onion-monero-blockchain-explorer`, you would expect to see some conditional logic to handle the above, some special handling of the supplied hashes, or special handling of the results looked up in the blockchain.




------

### Code walk-through of the API

The onion-monero-blockchain-explorer is built using the monero LMDB library for interfacing to the blockchain.  Interface to the monero library is via MicroCore.h which provides methods:
    get_core(); get_mempool(); get_block_by_height(); get_tx(); find_output_in_tx(); get_blk_timestamp(); get_block_complete_entry();


The `main.cpp` entry point checks command line options, starts a mempool monitoring thread, and sets up the URL routes it responds to, along with their respective parameters.  The only route that Bisq uses is `/api/outputs` which corresponds to the method `xmrblocks.json_outputs()` in page.h.

`json_outputs` first checks the three parameters tx_hash_str, address_str, and viewkey_str.  They must not be empty.

It calls xmreg::parse_str_secret_key with tx_hash_str and does the same with viewkey_str.  parse_str_secret_key() calls parse_hash256 which is part of the monero library.  It converts a hex string hash to binary data.  It calls xmreg::parse_str_address with address_str which in turn calls get_account_address_from_str() in the monero library.

Next it looks for the tx_hash in the mempool or blockchain (`find_tx`). If not found, the method returns with an error "Cant find tx hash".  MicroCore::get_tx() is called to get the transaction info.  This in turn calls cryptonote::Blockchain.get_db().get_tx() which is the monero LMDB library.

If not found in the blockchain, the mempool is checked via `search_mempool` which calls `get_mempool_txs` and iterates through the results looking for a matching txhash.  MempoolStatus.cpp contains the code for maintaining a list of active mempool transactions.  It in turn delegates to cryptonote::Blockchain.

Next, it calls `get_tx_details`.  It loads all the information about the transaction, such as number of inputs, outputs, fee, size, version, unlock time, and number of confirmations.  Much of this data is obtained by calling `summary_of_in_out_rct` which is in tools.cpp and just iterates through structs contained in the transaction object.

Next, it iterates over each output from the transaction details (output_pub_keys).  For each output, it includes in the json data returned to caller: "output_pubkey","amount","match","output_idx".  "match" is set to true if the "output_pubkey" matches the supplied decoded address (i.e. is the user's address)
   
The "tx_timestamp" is obtained from the transaction's block if present in a block, or the mempool.
                
tx_hash", "address", "viewkey", and "tx_prove" are pulled from data structures used above and returned to the caller in the json.

"tx_confirmations" is pulled from the transaction details object and returned in the json.

------

### Conclusion

No malicious code found.  See **tldr;** above.


-- 
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/4458#issuecomment-689951659
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200909/98520917/attachment-0001.html>