From 7b96a83350c0d0c8f0b3e10710dc4314ff2190aa Mon Sep 17 00:00:00 2001 From: Sander van Grieken Date: Mon, 20 Nov 2023 15:23:49 +0100 Subject: [PATCH] wallet: add sighash check to class Abstract_Wallet qml: use backend sighash check and add user confirmation path qt: use backend sighash check and add user confirmation path qml: include get_warning_for_risk_of_burning_coins_as_fees test in txdetails qt: add warning icon to sighash warning add sighash and fee checks to wallet.sign_transaction, making all warnings fatal unless ignore_warnings is set to True tests: test sign_transaction on both code paths with ignore_warnings True and False, raise specific exceptions TransactionPotentiallyDangerousException and TransactionDangerousException --- electrum/commands.py | 4 +- electrum/gui/qml/components/TxDetails.qml | 25 +++++++++- electrum/gui/qml/qetxdetails.py | 27 +++++++++- electrum/gui/qml/qewallet.py | 3 +- electrum/gui/qt/main_window.py | 3 +- electrum/gui/qt/transaction_dialog.py | 41 +++++++++++++-- electrum/tests/test_wallet_vertical.py | 24 ++++++--- electrum/transaction.py | 1 + electrum/wallet.py | 61 ++++++++++++++++++++++- 9 files changed, 167 insertions(+), 22 deletions(-) diff --git a/electrum/commands.py b/electrum/commands.py index 2ddd37f2b..23d7710e0 100644 --- a/electrum/commands.py +++ b/electrum/commands.py @@ -469,10 +469,10 @@ class Commands: return tx.serialize() @command('wp') - async def signtransaction(self, tx, password=None, wallet: Abstract_Wallet = None): + async def signtransaction(self, tx, password=None, wallet: Abstract_Wallet = None, iknowwhatimdoing: bool=False): """Sign a transaction. The wallet keys will be used to sign the transaction.""" tx = tx_from_any(tx) - wallet.sign_transaction(tx, password) + wallet.sign_transaction(tx, password, ignore_warnings=iknowwhatimdoing) return tx.serialize() @command('') diff --git a/electrum/gui/qml/components/TxDetails.qml b/electrum/gui/qml/components/TxDetails.qml index c6115011b..a4cbcf4be 100644 --- a/electrum/gui/qml/components/TxDetails.qml +++ b/electrum/gui/qml/components/TxDetails.qml @@ -49,6 +49,16 @@ Pane { text: qsTr('On-chain Transaction') } + InfoTextArea { + id: warn + Layout.columnSpan: 2 + Layout.fillWidth: true + Layout.bottomMargin: constants.paddingLarge + visible: txdetails.warning + text: txdetails.warning + iconStyle: InfoTextArea.IconStyle.Warn + } + InfoTextArea { id: bumpfeeinfo Layout.columnSpan: 2 @@ -336,7 +346,20 @@ Pane { icon.source: '../../icons/key.png' text: qsTr('Sign') visible: txdetails.canSign - onClicked: txdetails.sign() + onClicked: { + if (txdetails.shouldConfirm) { + var dialog = app.messageDialog.createObject(app, { + text: qsTr('Confirm signing non-standard transaction?'), + yesno: true + }) + dialog.accepted.connect(function() { + txdetails.sign() + }) + dialog.open() + } else { + txdetails.sign() + } + } } FlatButton { diff --git a/electrum/gui/qml/qetxdetails.py b/electrum/gui/qml/qetxdetails.py index 62f933a04..acd353d2e 100644 --- a/electrum/gui/qml/qetxdetails.py +++ b/electrum/gui/qml/qetxdetails.py @@ -5,7 +5,7 @@ from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject from electrum.i18n import _ from electrum.logging import get_logger from electrum.util import format_time, TxMinedInfo -from electrum.transaction import tx_from_any, Transaction +from electrum.transaction import tx_from_any, Transaction, PartialTxInput, Sighash, PartialTransaction from electrum.network import Network from electrum.address_synchronizer import TX_HEIGHT_UNCONF_PARENT, TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_FUTURE @@ -32,6 +32,7 @@ class QETxDetails(QObject, QtEventListener): self._txid = '' self._rawtx = '' self._label = '' + self._warning = '' self._tx = None # type: Optional[Transaction] @@ -56,6 +57,7 @@ class QETxDetails(QObject, QtEventListener): self._is_mined = False self._is_final = False self._lock_delay = 0 + self._should_confirm = False self._mempool_depth = '' @@ -140,6 +142,10 @@ class QETxDetails(QObject, QtEventListener): def status(self): return self._status + @pyqtProperty(str, notify=detailsChanged) + def warning(self): + return self._warning + @pyqtProperty(QEAmount, notify=detailsChanged) def amount(self): return self._amount @@ -240,6 +246,10 @@ class QETxDetails(QObject, QtEventListener): def lockDelay(self): return self._lock_delay + @pyqtProperty(bool, notify=detailsChanged) + def shouldConfirm(self): + return self._should_confirm + def update(self, from_txid: bool = False): assert self._wallet @@ -284,6 +294,9 @@ class QETxDetails(QObject, QtEventListener): fee_per_kb = txinfo.fee / size * 1000 self._feerate_str = self._wallet.wallet.config.format_fee_rate(fee_per_kb) + self._should_confirm = False + should_reject = False + self._lock_delay = 0 self._is_mined = False if not txinfo.tx_mined_status else txinfo.tx_mined_status.height > 0 if self._is_mined: @@ -293,6 +306,10 @@ class QETxDetails(QObject, QtEventListener): self._mempool_depth = self._wallet.wallet.config.depth_tooltip(txinfo.mempool_depth_bytes) elif txinfo.tx_mined_status.height == TX_HEIGHT_FUTURE: self._lock_delay = txinfo.tx_mined_status.wanted_height - self._wallet.wallet.adb.get_local_height() + if isinstance(self._tx, PartialTransaction): + self._should_confirm, should_reject, message = self._wallet.wallet.check_sighash(self._tx) + if message: + self._warning = '\n'.join([_('Danger! This transaction is non-standard!'), message]) if self._wallet.wallet.lnworker: # Calling lnworker.get_onchain_history and wallet.get_full_history here @@ -320,7 +337,13 @@ class QETxDetails(QObject, QtEventListener): self._can_cpfp = txinfo.can_cpfp and not txinfo.can_remove self._can_save_as_local = txinfo.can_save_as_local and not txinfo.can_remove self._can_remove = txinfo.can_remove - self._can_sign = not self._is_complete and self._wallet.wallet.can_sign(self._tx) + self._can_sign = not self._is_complete and self._wallet.wallet.can_sign(self._tx) and not should_reject + + if isinstance(self._tx, PartialTransaction): + risk_of_burning_coins = (self._can_sign and txinfo.fee is not None + and self._wallet.wallet.get_warning_for_risk_of_burning_coins_as_fees(self._tx)) + if risk_of_burning_coins: + self._warning = '\n'.join([self._warning, risk_of_burning_coins]) self.detailsChanged.emit() diff --git a/electrum/gui/qml/qewallet.py b/electrum/gui/qml/qewallet.py index daea2e8cf..9f5cbd9fb 100644 --- a/electrum/gui/qml/qewallet.py +++ b/electrum/gui/qml/qewallet.py @@ -512,7 +512,8 @@ class QEWallet(AuthMixin, QObject, QtEventListener): def do_sign(self, tx, broadcast): try: - tx = self.wallet.sign_transaction(tx, self.password) + # ignore_warnings=True, because UI checks and asks user confirmation itself + tx = self.wallet.sign_transaction(tx, self.password, ignore_warnings=True) except BaseException as e: self._logger.error(f'{e!r}') self.signFailed.emit(str(e)) diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py index ffc3235da..524401b98 100644 --- a/electrum/gui/qt/main_window.py +++ b/electrum/gui/qt/main_window.py @@ -1270,7 +1270,8 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener): # can sign directly task = partial(tx.sign, external_keypairs) else: - task = partial(self.wallet.sign_transaction, tx, password) + # ignore_warnings=True, because UI checks and asks user confirmation itself + task = partial(self.wallet.sign_transaction, tx, password, ignore_warnings=True) msg = _('Signing transaction...') WaitingDialog(self, msg, task, on_success, on_failure) diff --git a/electrum/gui/qt/transaction_dialog.py b/electrum/gui/qt/transaction_dialog.py index e801c4599..7dbea0d9d 100644 --- a/electrum/gui/qt/transaction_dialog.py +++ b/electrum/gui/qt/transaction_dialog.py @@ -50,8 +50,7 @@ from electrum.bitcoin import base_encode, NLOCKTIME_BLOCKHEIGHT_MAX, DummyAddres from electrum.i18n import _ from electrum.plugin import run_hook from electrum import simple_config -from electrum.transaction import SerializationError, Transaction, PartialTransaction, PartialTxInput, TxOutpoint -from electrum.transaction import TxinDataFetchProgress +from electrum.transaction import SerializationError, Transaction, PartialTransaction, TxOutpoint, TxinDataFetchProgress from electrum.logging import get_logger from electrum.util import ShortID, get_asyncio_loop from electrum.network import Network @@ -77,15 +76,16 @@ _logger = get_logger(__name__) dialogs = [] # Otherwise python randomly garbage collects the dialogs... - class TxSizeLabel(QLabel): def setAmount(self, byte_size): self.setText(('x %s bytes =' % byte_size) if byte_size else '') + class TxFiatLabel(QLabel): def setAmount(self, fiat_fee): self.setText(('≈ %s' % fiat_fee) if fiat_fee else '') + class QTextBrowserWithDefaultSize(QTextBrowser): def __init__(self, width: int = 0, height: int = 0): self._width = width @@ -96,8 +96,8 @@ class QTextBrowserWithDefaultSize(QTextBrowser): def sizeHint(self): return QSize(self._width, self._height) -class TxInOutWidget(QWidget): +class TxInOutWidget(QWidget): def __init__(self, main_window: 'ElectrumWindow', wallet: 'Abstract_Wallet'): QWidget.__init__(self) @@ -113,9 +113,24 @@ class TxInOutWidget(QWidget): self.inputs_textedit.setContextMenuPolicy(Qt.CustomContextMenu) self.inputs_textedit.customContextMenuRequested.connect(self.on_context_menu_for_inputs) + self.sighash_label = QLabel() + self.sighash_label.setStyleSheet('font-weight: bold') + self.sighash_confirm = False + self.sighash_reject = False + self.sighash_message = '' + self.inputs_warning_icon = QLabel() + pixmap = QPixmap(icon_path("warning")) + pixmap_size = round(2 * char_width_in_lineedit()) + pixmap = pixmap.scaled(pixmap_size, pixmap_size, Qt.KeepAspectRatio, Qt.SmoothTransformation) + self.inputs_warning_icon.setPixmap(pixmap) + self.inputs_warning_icon.setVisible(False) + self.inheader_hbox = QHBoxLayout() self.inheader_hbox.setContentsMargins(0, 0, 0, 0) self.inheader_hbox.addWidget(self.inputs_header) + self.inheader_hbox.addStretch(2) + self.inheader_hbox.addWidget(self.sighash_label) + self.inheader_hbox.addWidget(self.inputs_warning_icon) self.txo_color_recv = TxOutputColoring( legend=_("Wallet Address"), color=ColorScheme.GREEN, tooltip=_("Wallet receiving address")) @@ -246,6 +261,13 @@ class TxInOutWidget(QWidget): short_id=str(txin.short_id), addr=addr, value=txin_value, ) + if isinstance(self.tx, PartialTransaction): + self.sighash_confirm, self.sighash_reject, self.sighash_message = self.wallet.check_sighash(self.tx) + if self.sighash_message: + self.sighash_label.setText(_('Danger! This transaction is non-standard!')) + self.inputs_warning_icon.setVisible(True) + self.inputs_warning_icon.setToolTip(self.sighash_message) + self.outputs_header.setText(_("Outputs") + ' (%d)'%len(self.tx.outputs())) o_text = self.outputs_textedit o_text.clear() @@ -645,6 +667,14 @@ class TxDialog(QDialog, MessageBoxMixin): self.update() self.main_window.pop_top_level_window(self) + if self.io_widget.sighash_confirm: + if not self.question('\n'.join([ + _('Danger! This transaction is non-standard!'), + self.io_widget.sighash_message, + '', + _('Are you sure you want to sign this transaction?') + ])): + return self.sign_button.setDisabled(True) self.main_window.push_top_level_window(self) self.main_window.sign_tx(self.tx, callback=sign_done, external_keypairs=self.external_keypairs) @@ -771,7 +801,8 @@ class TxDialog(QDialog, MessageBoxMixin): self.broadcast_button.setEnabled(tx_details.can_broadcast) can_sign = not self.tx.is_complete() and \ (self.wallet.can_sign(self.tx) or bool(self.external_keypairs)) - self.sign_button.setEnabled(can_sign) + self.sign_button.setEnabled(can_sign and not self.io_widget.sighash_reject) + self.sign_button.setToolTip(self.io_widget.sighash_message) if tx_details.txid: self.tx_hash_e.setText(tx_details.txid) else: diff --git a/electrum/tests/test_wallet_vertical.py b/electrum/tests/test_wallet_vertical.py index 841b07762..fc4f1b88a 100644 --- a/electrum/tests/test_wallet_vertical.py +++ b/electrum/tests/test_wallet_vertical.py @@ -12,12 +12,10 @@ from electrum import SimpleConfig from electrum import util from electrum.address_synchronizer import TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT from electrum.wallet import (sweep, Multisig_Wallet, Standard_Wallet, Imported_Wallet, - restore_wallet_from_text, Abstract_Wallet, CannotBumpFee, BumpFeeStrategy) -from electrum.util import ( - bfh, NotEnoughFunds, UnrelatedTransactionException, - UserFacingException) -from electrum.transaction import (TxOutput, Transaction, PartialTransaction, PartialTxOutput, - PartialTxInput, tx_from_any, TxOutpoint) + restore_wallet_from_text, Abstract_Wallet, CannotBumpFee, BumpFeeStrategy, + TransactionPotentiallyDangerousException, TransactionDangerousException) +from electrum.util import bfh, NotEnoughFunds, UnrelatedTransactionException, UserFacingException +from electrum.transaction import Transaction, PartialTxOutput, tx_from_any from electrum.mnemonic import seed_type from electrum.network import Network @@ -2963,13 +2961,23 @@ class TestWalletSending(ElectrumTestCase): partial_tx) # load tx into cosignerB's offline wallet tx = tx_from_any(partial_tx) - wallet1b_offline.sign_transaction(tx, password=None) + wallet1b_offline.sign_transaction(tx, password=None, ignore_warnings=True) self.assertEqual('02000000014e375f685f3205e0c7841036525b10f01654632c5ae91e7e04513b815e46a5e100000000d9004730440220414287f36a02b004d2e9a3892e1862edaf49c35d50b65ae10b601879b8c793ef0220073234c56d5a8ae9f4fcfeaecaa757e2724bf830d45aabfab8ffe37329ebf4590147304402203ba7cc21e407ce31c1eecd11c367df716a5d47f06e0bf7109f08063ede25a364022039f6bef0dd401aa2c3103b8cbab57cc4fed3905ccb0a726dc6594bf5930ae0b401475221026addf5fd752c92e8a53955e430ca5964feb1b900ce569f968290f65ae7fecbfd2103a8b896e5216fe7239516a494407c0cc90c6dc33918c7df04d1cda8d57a3bb98152aefdffffff02400d0300000000001600144770c0bc4c42ed1cad089749cc887856ec0f9d99588004000000000017a914493900cdec652a41c633436b53d574647e329b18871c112500', str(tx)) self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.txid()) self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.wtxid()) + # again, but raise on warnings (here: signing non-segwit inputs is risky) + tx = tx_from_any(partial_tx) + try: + wallet1b_offline.sign_transaction(tx, password=None) + self.assertFalse(uses_qr_code2) + except TransactionDangerousException: + raise + except TransactionPotentiallyDangerousException: + self.assertTrue(uses_qr_code2) + @mock.patch.object(wallet.Abstract_Wallet, 'save_db') async def test_we_dont_sign_tx_including_dummy_address(self, mock_save_db): wallet1 = self.create_standard_wallet_from_seed('bitter grass shiver impose acquire brush forget axis eager alone wine silver') @@ -3095,7 +3103,7 @@ class TestWalletOfflineSigning(ElectrumTestCase): self.assertEqual(tx.txid(), tx_copy.txid()) # sign tx - tx = wallet_offline.sign_transaction(tx_copy, password=None) + tx = wallet_offline.sign_transaction(tx_copy, password=None, ignore_warnings=True) self.assertTrue(tx.is_complete()) self.assertFalse(tx.is_segwit()) self.assertEqual('d9c21696eca80321933e7444ca928aaf25eeda81aaa2f4e5c085d4d0a9cf7aa7', tx.txid()) diff --git a/electrum/transaction.py b/electrum/transaction.py index 1e18fca38..e40421145 100644 --- a/electrum/transaction.py +++ b/electrum/transaction.py @@ -42,6 +42,7 @@ import copy from . import ecc, bitcoin, constants, segwit_addr, bip32 from .bip32 import BIP32Node +from .i18n import _ from .util import profiler, to_bytes, bfh, chunks, is_hex_str, parse_max_spend from .bitcoin import (TYPE_ADDRESS, TYPE_SCRIPT, hash_160, hash160_to_p2sh, hash160_to_p2pkh, hash_to_segwit_addr, diff --git a/electrum/wallet.py b/electrum/wallet.py index 96ff5e2cb..e04dfab2a 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -71,7 +71,7 @@ from .storage import StorageEncryptionVersion, WalletStorage from .wallet_db import WalletDB from . import transaction, bitcoin, coinchooser, paymentrequest, ecc, bip32 from .transaction import (Transaction, TxInput, UnknownTxinType, TxOutput, - PartialTransaction, PartialTxInput, PartialTxOutput, TxOutpoint) + PartialTransaction, PartialTxInput, PartialTxOutput, TxOutpoint, Sighash) from .plugin import run_hook from .address_synchronizer import (AddressSynchronizer, TX_HEIGHT_LOCAL, TX_HEIGHT_UNCONF_PARENT, TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_FUTURE, TX_TIMESTAMP_INF) @@ -233,20 +233,29 @@ class CannotBumpFee(CannotRBFTx): def __str__(self): return _('Cannot bump fee') + ':\n\n' + Exception.__str__(self) + class CannotDoubleSpendTx(CannotRBFTx): def __str__(self): return _('Cannot cancel transaction') + ':\n\n' + Exception.__str__(self) + class CannotCPFP(Exception): def __str__(self): return _('Cannot create child transaction') + ':\n\n' + Exception.__str__(self) + class InternalAddressCorruption(Exception): def __str__(self): return _("Wallet file corruption detected. " "Please restore your wallet from seed, and compare the addresses in both files") +class TransactionPotentiallyDangerousException(Exception): pass + + +class TransactionDangerousException(TransactionPotentiallyDangerousException): pass + + class BumpFeeStrategy(enum.Enum): PRESERVE_PAYMENT = enum.auto() DECREASE_PAYMENT = enum.auto() @@ -2454,7 +2463,7 @@ class Abstract_Wallet(ABC, Logger, EventListener): txout.is_change = self.is_change(address) self._add_txinout_derivation_info(txout, address, only_der_suffix=only_der_suffix) - def sign_transaction(self, tx: Transaction, password) -> Optional[PartialTransaction]: + def sign_transaction(self, tx: Transaction, password, *, ignore_warnings: bool = False) -> Optional[PartialTransaction]: """ returns tx if successful else None """ if self.is_watching_only(): return @@ -2467,6 +2476,24 @@ class Abstract_Wallet(ABC, Logger, EventListener): if swap: self.lnworker.swap_manager.sign_tx(tx, swap) return tx + + # check if signing is dangerous + should_confirm, should_reject, message = self.check_sighash(tx) + if should_reject: + raise TransactionDangerousException('Not signing transaction:\n' + message) + if not ignore_warnings: + if should_confirm: + message = '\n'.join([_('Danger! This transaction is non-standard!'), message]) + fee = self.get_wallet_delta(tx).fee + risk_of_burning_coins = (fee is not None + and self.get_warning_for_risk_of_burning_coins_as_fees(tx)) + if risk_of_burning_coins: + should_confirm = True + message = '\n'.join([message, risk_of_burning_coins]) + + if should_confirm: + raise TransactionPotentiallyDangerousException('Not signing transaction:\n' + message) + # add info to a temporary tx copy; including xpubs # and full derivation paths as hw keystores might want them tmp_tx = copy.deepcopy(tx) @@ -3041,6 +3068,36 @@ class Abstract_Wallet(ABC, Logger, EventListener): "do not accept to sign it more than once,\n" "otherwise you could end up paying a different fee.")) + def check_sighash(self, tx: 'PartialTransaction') -> (bool, bool, str): + """Checks the Sighash for my inputs and return hints in the form + (confirm, reject, message), where confirm indicates the user should explicitly confirm, + reject indicates the transaction should be rejected (unless overruled in e.g. a config property), + and message containing a user-facing message describing the context""" + hintmap = dict([ + (0, (False, False, None)), + (Sighash.NONE, (True, True, _('Input {} is marked SIGHASH_NONE.'))), + (Sighash.SINGLE, (True, False, _('Input {} is marked SIGHASH_SINGLE.'))), + (Sighash.ALL, (False, False, None)), + (Sighash.ANYONECANPAY, (True, False, _('Input {} is marked SIGHASH_ANYONECANPAY.'))) + ]) + confirm = False + reject = False + messages = [] + for txin_idx, txin in enumerate(tx.inputs()): + if txin.sighash and txin.sighash != Sighash.ALL: # non-standard + addr = self.adb.get_txin_address(txin) + if self.adb.is_mine(addr): + sh_out = txin.sighash & (Sighash.ANYONECANPAY ^ 0xff) + sh_in = txin.sighash & Sighash.ANYONECANPAY + confirm |= hintmap[sh_out][0] | hintmap[sh_in][0] + reject |= hintmap[sh_out][1] | hintmap[sh_in][1] + for sh in [sh_out, sh_in]: + msg = hintmap[sh][2] + if msg: + messages.append('%s: %s' % (_('Fatal') if hintmap[sh][1] else _('Warning'), + msg.format(txin_idx))) + return confirm, reject, '\n'.join(messages) + def get_tx_fee_warning( self, *, invoice_amt: int,