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

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

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

core/src/apps/bitcoin/sign_tx/change_detector.py

@@ -46,17 +46,9 @@ class ChangeDetector:
         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_fingerprint.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

@@ -103,7 +103,11 @@ class MultisigFingerprintChecker(MatchChecker):
         from .. import multisig
 
         if not txio.multisig:
-            return None
+            # The fingerprint of a singlesig input or output is defined as an empty byte string.
+            # This has two consequences: First, a singlesig output matches if and only if all
+            # the added inputs are singlesig. Second, a multisig output does not match if any of
+            # the added inputs is singlesig.
+            return bytes()
         return multisig.multisig_fingerprint(txio.multisig)
 
 

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)