From d9b7a57e8b478fa8fa3be3463f08f242d95c4936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Fri, 25 Oct 2024 13:40:19 +0200 Subject: [PATCH] feat(core)): forbid multisig to singlesig change outputs --- core/.changelog.d/4351.changed.1 | 1 + .../apps/bitcoin/sign_tx/change_detector.py | 15 +++---- core/src/apps/bitcoin/sign_tx/matchcheck.py | 5 +++ .../test_apps.bitcoin.change_detector.py | 40 +++++++++---------- .../bitcoin/test_multisig_change.py | 16 +++++++- 5 files changed, 48 insertions(+), 29 deletions(-) create mode 100644 core/.changelog.d/4351.changed.1 diff --git a/core/.changelog.d/4351.changed.1 b/core/.changelog.d/4351.changed.1 new file mode 100644 index 0000000000..46ad33772a --- /dev/null +++ b/core/.changelog.d/4351.changed.1 @@ -0,0 +1 @@ +Forbidden multisig to singlesig change outputs. diff --git a/core/src/apps/bitcoin/sign_tx/change_detector.py b/core/src/apps/bitcoin/sign_tx/change_detector.py index 39630aa6ed..3f38bc0d02 100644 --- a/core/src/apps/bitcoin/sign_tx/change_detector.py +++ b/core/src/apps/bitcoin/sign_tx/change_detector.py @@ -17,11 +17,15 @@ _BIP32_MAX_LAST_ELEMENT = const(1_000_000) class ChangeDetector: def __init__(self) -> None: from .matchcheck import ( + MultisigChecker, MultisigFingerprintChecker, ScriptTypeChecker, WalletPathChecker, ) + # Whether all inputs are multisig or all inputs are singlesig, used to validate change-output. + self.multisig = MultisigChecker() + # Checksum of multisig inputs, used to validate change-output. self.multisig_fingerprint = MultisigFingerprintChecker() @@ -36,27 +40,24 @@ class ChangeDetector: self.wallet_path.add_input(txi) self.script_type.add_input(txi) self.multisig_fingerprint.add_input(txi) + self.multisig.add_input(txi) def check_input(self, txi: TxInput) -> None: self.wallet_path.check_input(txi) self.script_type.check_input(txi) self.multisig_fingerprint.check_input(txi) + self.multisig.check_input(txi) def output_is_change(self, txo: TxOutput) -> bool: if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES: return False - # Check the multisig fingerprint only for multisig outputs. This means - # that a transfer from a multisig account to a singlesig account is - # treated as a change-output as long as all other change-output - # conditions are satisfied. This goes a bit against the concept of a - # multisig account but the other cosigners will notice that they are - # relinquishing control of the funds, so there is no security risk. if txo.multisig and not self.multisig_fingerprint.output_matches(txo): return False return ( - self.wallet_path.output_matches(txo) + self.multisig.output_matches(txo) + and self.wallet_path.output_matches(txo) and self.script_type.output_matches(txo) and len(txo.address_n) >= common.BIP32_WALLET_DEPTH and txo.address_n[-2] <= _BIP32_CHANGE_CHAIN diff --git a/core/src/apps/bitcoin/sign_tx/matchcheck.py b/core/src/apps/bitcoin/sign_tx/matchcheck.py index a9db1aa11e..3cedb75034 100644 --- a/core/src/apps/bitcoin/sign_tx/matchcheck.py +++ b/core/src/apps/bitcoin/sign_tx/matchcheck.py @@ -107,6 +107,11 @@ class MultisigFingerprintChecker(MatchChecker): return multisig.multisig_fingerprint(txio.multisig) +class MultisigChecker(MatchChecker): + def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: + return txio.multisig is not None + + class ScriptTypeChecker(MatchChecker): def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: from trezor.enums import InputScriptType diff --git a/core/tests/test_apps.bitcoin.change_detector.py b/core/tests/test_apps.bitcoin.change_detector.py index 52183eab08..d262d146b5 100644 --- a/core/tests/test_apps.bitcoin.change_detector.py +++ b/core/tests/test_apps.bitcoin.change_detector.py @@ -120,17 +120,17 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs_order(self): - # Different order of xpubs - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1])) + def test_singlesig_different_account_indices(self): + # Different account indices + self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0])) + self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0])) - # Same ouputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + # Same outputs as inputs + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False - # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + # Multisig instead of singlesig + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -149,7 +149,7 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2])) == True # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False # Different account index assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False @@ -163,17 +163,17 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_singlesig_different_account_indices(self): - # Different account indices - self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0])) - self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0])) + def test_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1])) - # Same outputs as inputs + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + + # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False - - # Multisig instead of singlesig - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -188,7 +188,7 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False # Singlesig instead of multisig - assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 694b35bde1..55caaf9342 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -222,7 +222,13 @@ def test_external_internal(client: Client): with client: client.set_expected_responses( - _responses(client, INP1, INP2, change_indices=[2], foreign_indices=[2]) + _responses( + client, + INP1, + INP2, + change_indices=[] if is_core(client) else [2], + foreign_indices=[2], + ) ) if is_core(client): IF = InputFlowConfirmAllWarnings(client) @@ -252,7 +258,13 @@ def test_internal_external(client: Client): with client: client.set_expected_responses( - _responses(client, INP1, INP2, change_indices=[1], foreign_indices=[1]) + _responses( + client, + INP1, + INP2, + change_indices=[] if is_core(client) else [1], + foreign_indices=[1], + ) ) if is_core(client): IF = InputFlowConfirmAllWarnings(client)