Merge pull request #8616 from SomberNight/202309_dont_sign_tx_with_dummy_addr

add sanity checks we don't sign tx including dummy addr

electrum/bitcoin.py

@@ -28,7 +28,7 @@ from typing import List, Tuple, TYPE_CHECKING, Optional, Union, Sequence
 import enum
 from enum import IntEnum, Enum
 
-from .util import bfh, BitcoinException, assert_bytes, to_bytes, inv_dict, is_hex_str
+from .util import bfh, BitcoinException, assert_bytes, to_bytes, inv_dict, is_hex_str, classproperty
 from . import version
 from . import segwit_addr
 from . import constants
@@ -754,6 +754,29 @@ def is_minikey(text: str) -> bool:
 def minikey_to_private_key(text: str) -> bytes:
     return sha256(text)
 
-# dummy address for fee estimation of funding tx
-def get_dummy_address(purpose):
+
+def _get_dummy_address(purpose: str) -> str:
     return redeem_script_to_address('p2wsh', sha256(bytes(purpose, "utf8")).hex())
+
+_dummy_addr_funcs = set()
+class DummyAddress:
+    """dummy address for fee estimation of funding tx
+    Use e.g. as: DummyAddress.CHANNEL
+    """
+    def purpose(func):
+        _dummy_addr_funcs.add(func)
+        return classproperty(func)
+
+    @purpose
+    def CHANNEL(self) -> str:
+        return _get_dummy_address("channel")
+    @purpose
+    def SWAP(self) -> str:
+        return _get_dummy_address("swap")
+
+    @classmethod
+    def is_dummy_address(cls, addr: str) -> bool:
+        return addr in (f(cls) for f in _dummy_addr_funcs)
+
+
+class DummyAddressUsedInTxException(Exception): pass

electrum/gui/qml/qechannelopener.py

@@ -9,7 +9,7 @@ from electrum.i18n import _
 from electrum.gui import messages
 from electrum.util import bfh
 from electrum.lnutil import extract_nodeid, ConnStringFormatError
-from electrum.bitcoin import get_dummy_address
+from electrum.bitcoin import DummyAddress
 from electrum.lnworker import hardcoded_trampoline_nodes
 from electrum.logging import get_logger
 
@@ -182,7 +182,7 @@ class QEChannelOpener(QObject, AuthMixin):
         """
         self._logger.debug('opening channel')
         # read funding_sat from tx; converts '!' to int value
-        funding_sat = funding_tx.output_value_for_address(get_dummy_address('channel'))
+        funding_sat = funding_tx.output_value_for_address(DummyAddress.CHANNEL)
         lnworker = self._wallet.wallet.lnworker
 
         def open_thread():

electrum/gui/qml/qeswaphelper.py

@@ -6,7 +6,7 @@ from typing import Union
 from PyQt5.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject, QTimer, Q_ENUMS
 
 from electrum.i18n import _
-from electrum.bitcoin import get_dummy_address
+from electrum.bitcoin import DummyAddress
 from electrum.logging import get_logger
 from electrum.transaction import PartialTxOutput
 from electrum.util import NotEnoughFunds, NoDynamicFeeEstimates, profiler, get_asyncio_loop
@@ -245,7 +245,7 @@ class QESwapHelper(AuthMixin, QObject, QtEventListener):
         # this is just to estimate the maximal spendable onchain amount for HTLC
         self.update_tx('!')
         try:
-            max_onchain_spend = self._tx.output_value_for_address(get_dummy_address('swap'))
+            max_onchain_spend = self._tx.output_value_for_address(DummyAddress.SWAP)
         except AttributeError:  # happens if there are no utxos
             max_onchain_spend = 0
         reverse = int(min(lnworker.num_sats_can_send(),
@@ -283,7 +283,7 @@ class QESwapHelper(AuthMixin, QObject, QtEventListener):
             self._tx = None
             self.valid = False
             return
-        outputs = [PartialTxOutput.from_address_and_value(get_dummy_address('swap'), onchain_amount)]
+        outputs = [PartialTxOutput.from_address_and_value(DummyAddress.SWAP, onchain_amount)]
         coins = self._wallet.wallet.get_spendable_coins(None)
         try:
             self._tx = self._wallet.wallet.make_unsigned_transaction(

electrum/gui/qt/confirm_tx_dialog.py

@@ -39,6 +39,7 @@ from electrum.plugin import run_hook
 from electrum.transaction import Transaction, PartialTransaction
 from electrum.wallet import InternalAddressCorruption
 from electrum.simple_config import SimpleConfig
+from electrum.bitcoin import DummyAddress
 
 from .util import (WindowModalDialog, ColorScheme, HelpLabel, Buttons, CancelButton,
                    BlockingWaitingDialog, PasswordLineEdit, WWLabel, read_QIcon)
@@ -574,7 +575,7 @@ class TxEditor(WindowModalDialog):
                 self.error = long_warning
             else:
                 messages.append(long_warning)
-        if self.tx.has_dummy_output('swap'):
+        if self.tx.has_dummy_output(DummyAddress.SWAP):
             messages.append(_('This transaction will send funds to a submarine swap.'))
         # warn if spending unconf
         if any((txin.block_height is not None and txin.block_height<=0) for txin in self.tx.inputs()):

electrum/gui/qt/main_window.py

@@ -51,7 +51,7 @@ import electrum
 from electrum.gui import messages
 from electrum import (keystore, ecc, constants, util, bitcoin, commands,
                       paymentrequest, lnutil)
-from electrum.bitcoin import COIN, is_address, get_dummy_address
+from electrum.bitcoin import COIN, is_address, DummyAddress
 from electrum.plugin import run_hook, BasePlugin
 from electrum.i18n import _
 from electrum.util import (format_time, UserCancelled, profiler, bfh, InvalidPassword,
@@ -1303,7 +1303,7 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener):
     @protected
     def _open_channel(self, connect_str, funding_sat, push_amt, funding_tx, password):
         # read funding_sat from tx; converts '!' to int value
-        funding_sat = funding_tx.output_value_for_address(get_dummy_address('channel'))
+        funding_sat = funding_tx.output_value_for_address(DummyAddress.CHANNEL)
         def task():
             return self.wallet.lnworker.open_channel(
                 connect_str=connect_str,

electrum/gui/qt/send_tab.py

@@ -11,7 +11,7 @@ from PyQt5.QtGui import QMovie, QColor
 
 from electrum.i18n import _
 from electrum.logging import Logger
-
+from electrum.bitcoin import DummyAddress
 from electrum.plugin import run_hook
 from electrum.util import NotEnoughFunds, NoDynamicFeeEstimates, parse_max_spend
 from electrum.invoices import PR_PAID, Invoice, PR_BROADCASTING, PR_BROADCAST
@@ -326,11 +326,11 @@ class SendTab(QWidget, MessageBoxMixin, Logger):
             return
         is_preview = conf_dlg.is_preview
 
-        if tx.has_dummy_output('swap'):
+        if tx.has_dummy_output(DummyAddress.SWAP):
             sm = self.wallet.lnworker.swap_manager
             coro = sm.request_swap_for_tx(tx)
             swap, invoice, tx = self.network.run_from_another_thread(coro)
-            assert not tx.has_dummy_output('swap')
+            assert not tx.has_dummy_output(DummyAddress.SWAP)
             tx.swap_invoice = invoice
             tx.swap_payment_hash = swap.payment_hash
 

electrum/gui/qt/swap_dialog.py

@@ -5,7 +5,7 @@ from PyQt5.QtWidgets import QLabel, QVBoxLayout, QGridLayout, QPushButton
 
 from electrum.i18n import _
 from electrum.util import NotEnoughFunds, NoDynamicFeeEstimates
-from electrum.bitcoin import get_dummy_address
+from electrum.bitcoin import DummyAddress
 from electrum.transaction import PartialTxOutput, PartialTransaction
 
 from electrum.gui import messages
@@ -173,7 +173,7 @@ class SwapDialog(WindowModalDialog, QtEventListener):
 
     def _spend_max_forward_swap(self, tx: Optional[PartialTransaction]) -> None:
         if tx:
-            amount = tx.output_value_for_address(get_dummy_address('swap'))
+            amount = tx.output_value_for_address(DummyAddress.SWAP)
             self.send_amount_e.setAmount(amount)
         else:
             self.send_amount_e.setAmount(None)
@@ -295,7 +295,7 @@ class SwapDialog(WindowModalDialog, QtEventListener):
             if max_amount > max_swap_amount:
                 onchain_amount = max_swap_amount
         self.config.WALLET_SEND_CHANGE_TO_LIGHTNING = False
-        outputs = [PartialTxOutput.from_address_and_value(get_dummy_address('swap'), onchain_amount)]
+        outputs = [PartialTxOutput.from_address_and_value(DummyAddress.SWAP, onchain_amount)]
         try:
             tx = self.window.wallet.make_unsigned_transaction(
                 coins=coins,

electrum/gui/qt/transaction_dialog.py

@@ -46,7 +46,7 @@ from electrum.simple_config import SimpleConfig
 from electrum.util import quantize_feerate
 from electrum import bitcoin
 
-from electrum.bitcoin import base_encode, NLOCKTIME_BLOCKHEIGHT_MAX, get_dummy_address
+from electrum.bitcoin import base_encode, NLOCKTIME_BLOCKHEIGHT_MAX, DummyAddress
 from electrum.i18n import _
 from electrum.plugin import run_hook
 from electrum import simple_config
@@ -183,7 +183,7 @@ class TxInOutWidget(QWidget):
                 fmt.setAnchor(True)
                 fmt.setUnderlineStyle(QTextCharFormat.SingleUnderline)
                 return fmt
-            elif sm and sm.is_lockup_address_for_a_swap(addr) or addr==get_dummy_address('swap'):
+            elif sm and sm.is_lockup_address_for_a_swap(addr) or addr == DummyAddress.SWAP:
                 tf_used_swap = True
                 return self.txo_color_swap.text_char_format
             elif self.wallet.is_billing_address(addr):

electrum/lnpeer.py

@@ -24,7 +24,7 @@ from . import constants
 from .util import (bfh, log_exceptions, ignore_exceptions, chunks, OldTaskGroup,
                    UnrelatedTransactionException, error_text_bytes_to_safe_str)
 from . import transaction
-from .bitcoin import make_op_return, get_dummy_address
+from .bitcoin import make_op_return, DummyAddress
 from .transaction import PartialTxOutput, match_script_against_template, Sighash
 from .logging import Logger
 from .lnonion import (new_onion_packet, OnionFailureCode, calc_hops_data_for_payment,
@@ -812,7 +812,7 @@ class Peer(Logger):
         redeem_script = funding_output_script(local_config, remote_config)
         funding_address = bitcoin.redeem_script_to_address('p2wsh', redeem_script)
         funding_output = PartialTxOutput.from_address_and_value(funding_address, funding_sat)
-        funding_tx.replace_dummy_output('channel', funding_address)
+        funding_tx.replace_output_address(DummyAddress.CHANNEL, funding_address)
         # find and encrypt op_return data associated to funding_address
         has_onchain_backup = self.lnworker and self.lnworker.has_recoverable_channels()
         if has_onchain_backup:

electrum/lnworker.py

@@ -56,7 +56,7 @@ from .lnchannel import ChannelState, PeerState, HTLCWithStatus
 from .lnrater import LNRater
 from . import lnutil
 from .lnutil import funding_output_script
-from .bitcoin import redeem_script_to_address, get_dummy_address
+from .bitcoin import redeem_script_to_address, DummyAddress
 from .lnutil import (Outpoint, LNPeerAddr,
                      get_compressed_pubkey_from_bech32, extract_nodeid,
                      PaymentFailure, split_host_port, ConnStringFormatError,
@@ -1281,7 +1281,7 @@ class LNWallet(LNWorker):
             funding_sat: int,
             node_id: bytes,
             fee_est=None) -> PartialTransaction:
-        outputs = [PartialTxOutput.from_address_and_value(get_dummy_address('channel'), funding_sat)]
+        outputs = [PartialTxOutput.from_address_and_value(DummyAddress.CHANNEL, funding_sat)]
         if self.has_recoverable_channels():
             dummy_scriptpubkey = make_op_return(self.cb_data(node_id))
             outputs.append(PartialTxOutput(scriptpubkey=dummy_scriptpubkey, value=0))
@@ -2562,7 +2562,7 @@ class LNWallet(LNWorker):
             # check that we can send onchain
             swap_server_mining_fee = 10000 # guessing, because we have not called get_pairs yet
             swap_funding_sat = swap_recv_amount + swap_server_mining_fee
-            swap_output = PartialTxOutput.from_address_and_value(get_dummy_address('swap'), int(swap_funding_sat))
+            swap_output = PartialTxOutput.from_address_and_value(DummyAddress.SWAP, int(swap_funding_sat))
             if not self.wallet.can_pay_onchain([swap_output], coins=coins):
                 continue
             return (chan, swap_recv_amount)

electrum/network.py

@@ -48,7 +48,7 @@ from .util import (log_exceptions, ignore_exceptions, OldTaskGroup,
                    bfh, make_aiohttp_session, send_exception_to_crash_reporter,
                    is_hash256_str, is_non_negative_integer, MyEncoder, NetworkRetryManager,
                    nullcontext, error_text_str_to_safe_str)
-from .bitcoin import COIN
+from .bitcoin import COIN, DummyAddress, DummyAddressUsedInTxException
 from . import constants
 from . import blockchain
 from . import bitcoin
@@ -923,6 +923,8 @@ class Network(Logger, NetworkRetryManager[ServerAddr]):
             raise RequestTimedOut()
         if timeout is None:
             timeout = self.get_network_timeout_seconds(NetworkTimeout.Urgent)
+        if any(DummyAddress.is_dummy_address(txout.address) for txout in tx.outputs()):
+            raise DummyAddressUsedInTxException("tried to broadcast tx with dummy address!")
         try:
             out = await self.interface.session.send_request('blockchain.transaction.broadcast', [tx.serialize()], timeout=timeout)
             # note: both 'out' and exception messages are untrusted input from the server

electrum/submarine_swaps.py

@@ -17,7 +17,7 @@ from .transaction import PartialTxInput, PartialTxOutput, PartialTransaction, Tr
 from .transaction import script_GetOp, match_script_against_template, OPPushDataGeneric, OPPushDataPubkey
 from .util import log_exceptions, BelowDustLimit, OldTaskGroup
 from .lnutil import REDEEM_AFTER_DOUBLE_SPENT_DELAY
-from .bitcoin import dust_threshold, get_dummy_address
+from .bitcoin import dust_threshold, DummyAddress
 from .logging import Logger
 from .lnutil import hex_to_bytes
 from .lnaddr import lndecode
@@ -190,7 +190,7 @@ class SwapManager(Logger):
         self.wallet = wallet
         self.lnworker = lnworker
         self.taskgroup = None
-        self.dummy_address = get_dummy_address('swap')
+        self.dummy_address = DummyAddress.SWAP
 
         self.swaps = self.wallet.db.get_dict('submarine_swaps')  # type: Dict[str, SwapData]
         self._swaps_by_funding_outpoint = {}  # type: Dict[TxOutpoint, SwapData]
@@ -706,13 +706,13 @@ class SwapManager(Logger):
             funding_output = PartialTxOutput.from_address_and_value(swap.lockup_address, swap.onchain_amount)
             tx = self.wallet.create_transaction(outputs=[funding_output], rbf=True, password=password)
         else:
-            tx.replace_dummy_output('swap', swap.lockup_address)
+            tx.replace_output_address(DummyAddress.SWAP, swap.lockup_address)
             tx.set_rbf(True)
             self.wallet.sign_transaction(tx, password)
         return tx
 
     @log_exceptions
-    async def request_swap_for_tx(self, tx):
+    async def request_swap_for_tx(self, tx: 'PartialTransaction'):
         for o in tx.outputs():
             if o.address == self.dummy_address:
                 change_amount = o.value
@@ -725,7 +725,7 @@ class SwapManager(Logger):
         swap, invoice = await self.request_normal_swap(
             lightning_amount_sat = lightning_amount_sat,
             expected_onchain_amount_sat=change_amount)
-        tx.replace_dummy_output('swap', swap.lockup_address)
+        tx.replace_output_address(DummyAddress.SWAP, swap.lockup_address)
         return swap, invoice, tx
 
     @log_exceptions

electrum/tests/test_wallet_vertical.py

@@ -2891,6 +2891,27 @@ class TestWalletSending(ElectrumTestCase):
                         self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.txid())
                         self.assertEqual('d6823918ff82ed240995e9e6f02e0d2f3f15e0b942616ab34481ce8a3399dc72', tx.wtxid())
 
+    @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')
+
+        # bootstrap wallet1
+        funding_tx = Transaction('01000000014576dacce264c24d81887642b726f5d64aa7825b21b350c7b75a57f337da6845010000006b483045022100a3f8b6155c71a98ad9986edd6161b20d24fad99b6463c23b463856c0ee54826d02200f606017fd987696ebbe5200daedde922eee264325a184d5bbda965ba5160821012102e5c473c051dae31043c335266d0ef89c1daab2f34d885cc7706b267f3269c609ffffffff0240420f00000000001600148a28bddb7f61864bdcf58b2ad13d5aeb3abc3c42a2ddb90e000000001976a914c384950342cb6f8df55175b48586838b03130fad88ac00000000')
+        funding_txid = funding_tx.txid()
+        self.assertEqual('add2535aedcbb5ba79cc2260868bb9e57f328738ca192937f2c92e0e94c19203', funding_txid)
+        wallet1.adb.receive_tx_callback(funding_txid, funding_tx, TX_HEIGHT_UNCONFIRMED)
+
+        # wallet1 -> dummy address
+        outputs = [PartialTxOutput.from_address_and_value(bitcoin.DummyAddress.CHANNEL, 250000)]
+
+        with self.assertRaises(bitcoin.DummyAddressUsedInTxException):
+            tx = wallet1.mktx(outputs=outputs, password=None, fee=5000, tx_version=1, rbf=False)
+
+        coins = wallet1.get_spendable_coins(domain=None)
+        tx = wallet1.make_unsigned_transaction(coins=coins, outputs=outputs, fee=5000)
+        with self.assertRaises(bitcoin.DummyAddressUsedInTxException):
+            wallet1.sign_transaction(tx, password=None)
+
 
 class TestWalletOfflineSigning(ElectrumTestCase):
     TESTNET = True

electrum/transaction.py

@@ -52,7 +52,7 @@ from .bitcoin import (TYPE_ADDRESS, TYPE_SCRIPT, hash_160,
 from .crypto import sha256d
 from .logging import get_logger
 from .util import ShortID, OldTaskGroup
-from .bitcoin import get_dummy_address
+from .bitcoin import DummyAddress
 from .descriptor import Descriptor, MissingSolutionPiece, create_dummy_descriptor_from_address
 from .json_db import stored_in
 
@@ -1156,7 +1156,7 @@ class Transaction:
         script = bitcoin.address_to_script(addr)
         return self.get_output_idxs_from_scriptpubkey(script)
 
-    def replace_output_address(self, old_address, new_address):
+    def replace_output_address(self, old_address: str, new_address: str) -> None:
         idx = list(self.get_output_idxs_from_address(old_address))
         assert len(idx) == 1
         amount = self._outputs[idx[0]].value
@@ -1169,13 +1169,8 @@ class Transaction:
     def get_change_outputs(self):
         return  [o for o in self._outputs if o.is_change]
 
-    def replace_dummy_output(self, purpose, new_address):
-        dummy_addr = get_dummy_address(purpose)
-        self.replace_output_address(dummy_addr, new_address)
-
-    def has_dummy_output(self, purpose):
-        addr = get_dummy_address(purpose)
-        return len(self.get_output_idxs_from_address(addr)) == 1
+    def has_dummy_output(self, dummy_addr: str) -> bool:
+        return len(self.get_output_idxs_from_address(dummy_addr)) == 1
 
     def output_value_for_address(self, addr):
         # assumes exactly one output has that address

electrum/util.py

@@ -2003,6 +2003,14 @@ class nullcontext:
         pass
 
 
+class classproperty(property):
+    """~read-only class-level @property
+    from https://stackoverflow.com/a/13624858 by denis-ryzhkov
+    """
+    def __get__(self, owner_self, owner_cls):
+        return self.fget(owner_cls)
+
+
 def get_running_loop() -> Optional[asyncio.AbstractEventLoop]:
     """Returns the asyncio event loop that is *running in this thread*, if any."""
     try:

electrum/wallet.py

@@ -60,7 +60,8 @@ from .util import (NotEnoughFunds, UserCancelled, profiler, OldTaskGroup, ignore
                    Fiat, bfh, TxMinedInfo, quantize_feerate, OrderedDictWithIndex)
 from .simple_config import SimpleConfig, FEE_RATIO_HIGH_WARNING, FEERATE_WARNING_HIGH_FEE
 from .bitcoin import COIN, TYPE_ADDRESS
-from .bitcoin import is_address, address_to_script, is_minikey, relayfee, dust_threshold, get_dummy_address
+from .bitcoin import is_address, address_to_script, is_minikey, relayfee, dust_threshold
+from .bitcoin import DummyAddress, DummyAddressUsedInTxException
 from .crypto import sha256d
 from . import keystore
 from .keystore import (load_keystore, Hardware_KeyStore, KeyStore, KeyStoreWithMPK,
@@ -1767,7 +1768,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
                     amount = change[0].value
                     ln_amount = self.lnworker.swap_manager.get_recv_amount(amount, is_reverse=False)
                     if ln_amount and ln_amount <= self.lnworker.num_sats_can_receive():
-                        tx.replace_output_address(change[0].address, get_dummy_address('swap'))
+                        tx.replace_output_address(change[0].address, DummyAddress.SWAP)
         else:
             # "spend max" branch
             # note: This *will* spend inputs with negative effective value (if there are any).
@@ -2385,6 +2386,8 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             return
         if not isinstance(tx, PartialTransaction):
             return
+        if any(DummyAddress.is_dummy_address(txout.address) for txout in tx.outputs()):
+            raise DummyAddressUsedInTxException("tried to sign tx with dummy address!")
         # note: swap signing does not require the password
         swap = self.get_swap_by_claim_tx(tx)
         if swap: