AaronFiore/electrum
1
0
Fork 0
Code Activity

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

Browse Source 
Somewhat a follow-up to 649ce979ab.

This adds some safety belts so we don't accidentally sign a tx that
contains a dummy address.
Specifically we check that tx does not contain output for dummy addr:
- in wallet.sign_transaction
- in network.broadcast_transaction

The second one is perhaps redundant, but I think it does not hurt.
This commit is contained in:
SomberNight
2023-09-16 04:36:08 +00:00
parent 956b455954
commit 4c63d8729b
16 changed files with 94 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
electrum/bitcoin.py
View File

@@ -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

4
electrum/gui/qml/qechannelopener.py
View File

@@ -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():

6
electrum/gui/qml/qeswaphelper.py
View File

@@ -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(

3
electrum/gui/qt/confirm_tx_dialog.py
View File

@@ -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()):

4
electrum/gui/qt/main_window.py
View File

@@ -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,

6
electrum/gui/qt/send_tab.py
View File

@@ -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

6
electrum/gui/qt/swap_dialog.py
View File

@@ -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,

4
electrum/gui/qt/transaction_dialog.py
View File

@@ -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):

4
electrum/lnpeer.py
View File

@@ -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:

6
electrum/lnworker.py
View File

@@ -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,
@@ -1274,7 +1274,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))
@@ -2555,7 +2555,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)

4
electrum/network.py
View File

@@ -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

10
electrum/submarine_swaps.py
View File

@@ -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

21
electrum/tests/test_wallet_vertical.py
View File

@@ -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

13
electrum/transaction.py
View File

@@ -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

8
electrum/util.py
View File

@@ -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:

7
electrum/wallet.py
View File

@@ -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: