feat(core)): forbid multisig to singlesig change outputs

core/.changelog.d/4351.changed.1

@@ -0,0 +1 @@
+Forbidden multisig to singlesig change outputs.

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

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

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):
+    def test_singlesig_different_account_indices(self):
-        # Different order of xpubs
+        # Different account indices
-        self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2]))
+        self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0]))
-        self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1]))
+        self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0]))
 
-        # Same ouputs as inputs
+        # Same outputs 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_singlesig_output([H_(45), 0, 0, 0])) == False
-        assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True
+        assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False
 
-        # Singlesig instead of multisig
+        # Multisig instead of singlesig
-        assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == True
+        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):
+    def test_multisig_different_xpubs_order(self):
-        # Different account indices
+        # Different order of xpubs
-        self.d.add_input(get_singlesig_input([H_(45), 0, 0, 0]))
+        self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2]))
-        self.d.add_input(get_singlesig_input([H_(45), 1, 0, 0]))
+        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

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)