diff --git a/core/.changelog.d/4351.added b/core/.changelog.d/4351.added new file mode 100644 index 0000000000..a657afd431 --- /dev/null +++ b/core/.changelog.d/4351.added @@ -0,0 +1 @@ +Added support for sortedmulti. diff --git a/core/src/apps/bitcoin/multisig.py b/core/src/apps/bitcoin/multisig.py index 8ac1b7a1e5..ecb2fa8613 100644 --- a/core/src/apps/bitcoin/multisig.py +++ b/core/src/apps/bitcoin/multisig.py @@ -1,5 +1,6 @@ from typing import TYPE_CHECKING +from trezor.enums import MultisigPubkeysOrder from trezor.wire import DataError if TYPE_CHECKING: @@ -28,7 +29,8 @@ def multisig_fingerprint(multisig: MultisigRedeemScriptType) -> bytes: if len(d.public_key) != 33 or len(d.chain_code) != 32: raise DataError("Invalid multisig parameters") - pubnodes = sorted(pubnodes, key=lambda n: n.public_key + n.chain_code) + if multisig.pubkeys_order == MultisigPubkeysOrder.LEXICOGRAPHIC: + pubnodes = sorted(pubnodes, key=lambda n: n.public_key + n.chain_code) h = HashWriter(sha256()) write_uint32(h, m) @@ -84,9 +86,14 @@ def multisig_get_pubkey(n: HDNodeType, p: paths.Bip32Path) -> bytes: def multisig_get_pubkeys(multisig: MultisigRedeemScriptType) -> list[bytes]: validate_multisig(multisig) if multisig.nodes: - return [multisig_get_pubkey(hd, multisig.address_n) for hd in multisig.nodes] + pubkeys = [multisig_get_pubkey(hd, multisig.address_n) for hd in multisig.nodes] else: - return [multisig_get_pubkey(hd.node, hd.address_n) for hd in multisig.pubkeys] + pubkeys = [ + multisig_get_pubkey(hd.node, hd.address_n) for hd in multisig.pubkeys + ] + if multisig.pubkeys_order == MultisigPubkeysOrder.LEXICOGRAPHIC: + pubkeys = sorted(pubkeys) + return pubkeys def multisig_get_pubkey_count(multisig: MultisigRedeemScriptType) -> int: diff --git a/core/src/apps/bitcoin/sign_tx/change_detector.py b/core/src/apps/bitcoin/sign_tx/change_detector.py index 3f38bc0d02..374c24eb54 100644 --- a/core/src/apps/bitcoin/sign_tx/change_detector.py +++ b/core/src/apps/bitcoin/sign_tx/change_detector.py @@ -19,6 +19,7 @@ class ChangeDetector: from .matchcheck import ( MultisigChecker, MultisigFingerprintChecker, + PubkeysOrderChecker, ScriptTypeChecker, WalletPathChecker, ) @@ -29,6 +30,9 @@ class ChangeDetector: # Checksum of multisig inputs, used to validate change-output. self.multisig_fingerprint = MultisigFingerprintChecker() + # Whether all inputs use sorted pubkeys or all inputs use unsorted pubkeys, used to validate change-output. + self.pubkeys_order = PubkeysOrderChecker() + # Common prefix of input paths, used to validate change-output. self.wallet_path = WalletPathChecker() @@ -41,19 +45,25 @@ class ChangeDetector: self.script_type.add_input(txi) self.multisig_fingerprint.add_input(txi) self.multisig.add_input(txi) + self.pubkeys_order.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) + self.pubkeys_order.check_input(txi) def output_is_change(self, txo: TxOutput) -> bool: if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES: return False - if txo.multisig and not self.multisig_fingerprint.output_matches(txo): - return False + if txo.multisig: + if not ( + self.pubkeys_order.output_matches(txo) + and self.multisig_fingerprint.output_matches(txo) + ): + return False return ( self.multisig.output_matches(txo) diff --git a/core/src/apps/bitcoin/sign_tx/matchcheck.py b/core/src/apps/bitcoin/sign_tx/matchcheck.py index 3cedb75034..2e1072cf5a 100644 --- a/core/src/apps/bitcoin/sign_tx/matchcheck.py +++ b/core/src/apps/bitcoin/sign_tx/matchcheck.py @@ -107,6 +107,13 @@ class MultisigFingerprintChecker(MatchChecker): return multisig.multisig_fingerprint(txio.multisig) +class PubkeysOrderChecker(MatchChecker): + def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: + if not txio.multisig: + return None + return txio.multisig is not None and txio.multisig.pubkeys_order + + class MultisigChecker(MatchChecker): def attribute_from_tx(self, txio: TxInput | TxOutput) -> Any: return txio.multisig is not None diff --git a/core/tests/test_apps.bitcoin.change_detector.py b/core/tests/test_apps.bitcoin.change_detector.py index d262d146b5..7eec98f791 100644 --- a/core/tests/test_apps.bitcoin.change_detector.py +++ b/core/tests/test_apps.bitcoin.change_detector.py @@ -1,7 +1,8 @@ from common import H_, await_result, unittest # isort:skip from ubinascii import hexlify, unhexlify -from trezor.enums import InputScriptType, OutputScriptType +from trezor.enums import InputScriptType, OutputScriptType, MultisigPubkeysOrder +from trezor.enums.MultisigPubkeysOrder import LEXICOGRAPHIC, PRESERVED from trezor.messages import ( TxInput, TxOutput, @@ -39,12 +40,13 @@ xpub3 = HDNodeType( ) -def get_multisig(path: list[int], xpubs: list[HDNodeType]) -> MultisigRedeemScriptType: +def get_multisig(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> MultisigRedeemScriptType: return MultisigRedeemScriptType( nodes=xpubs, signatures=b"" * len(xpubs), address_n=path[-2:], m=2, + pubkeys_order=pubkeys_order, ) @@ -58,23 +60,23 @@ def get_singlesig_input(path: list[int]) -> TxInput: ) -def get_multisig_input(path: list[int], xpubs: list[HDNodeType]) -> TxInput: +def get_multisig_input(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> TxInput: return TxInput( address_n=path, amount=1_000_000, prev_hash=bytes(32), prev_index=0, script_type=InputScriptType.SPENDMULTISIG, - multisig=get_multisig(path, xpubs), + multisig=get_multisig(path, xpubs, pubkeys_order), ) -def get_internal_multisig_output(path: list[int], xpubs: list[HDNodeType]) -> TxOutput: +def get_internal_multisig_output(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> TxOutput: return TxOutput( address_n=path, amount=1_000_000, script_type=OutputScriptType.PAYTOMULTISIG, - multisig=get_multisig(path, xpubs), + multisig=get_multisig(path, xpubs, pubkeys_order), ) @@ -115,7 +117,8 @@ class TestChangeDetector(unittest.TestCase): 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 + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -130,23 +133,24 @@ class TestChangeDetector(unittest.TestCase): 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 + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 1, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 1, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig(self): + def test_unsorted_multisig(self): # Different change and account index - 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, 1], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2])) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 1], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2], PRESERVED)) # 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_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2], PRESERVED)) == True # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False @@ -155,22 +159,60 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False # Different order of xpubs - 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_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False # Different xpubs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs_order(self): + def test_sorted_multisig(self): + # Different change and account index + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 1], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2], LEXICOGRAPHIC)) + + # Same outputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2], LEXICOGRAPHIC)) == True + + # Singlesig instead of multisig + 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 + # 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])) + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == True + + # Unsorted instead of sorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + + # Different xpubs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # External output + assert self.d.output_is_change(get_external_singlesig_output()) == False + + def test_unsorted_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) # 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 + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == False # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False @@ -178,14 +220,70 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs(self): - # Different 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], [xpub1, xpub3])) + def test_sorted_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) # Same ouputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == False - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == True + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Singlesig instead of multisig + 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 + + def test_unsorted_multisig_different_xpubs(self): + # Different xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # Singlesig instead of multisig + 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 + + def test_sorted_multisig_different_xpubs(self): + # Different xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False + + # Singlesig instead of multisig + 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 + + def test_mixed_sorted_and_unsorted_multisig_1(self): + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 55caaf9342..990c92c82e 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -63,7 +63,7 @@ NODE_INT = bip32.deserialize( # m/2 => 038caebd6f753bbbd2bb1f3346a43cd32140648583673a31d62f2dfb56ad0ab9e3 multisig_in1 = messages.MultisigRedeemScriptType( - nodes=[NODE_EXT2, NODE_EXT1, NODE_INT], + nodes=[NODE_EXT1, NODE_EXT2, NODE_INT], address_n=[0, 0], signatures=[b"", b"", b""], m=2, @@ -84,7 +84,7 @@ multisig_in3 = messages.MultisigRedeemScriptType( ) prev_hash_1, prev_tx_1 = forge_prevtx( - [("3HwrvQEfYw4wUvGHpGmixWB15HPgqrvTh1", 50_000_000)] + [("3Ltgk5WPUMLcT2QvwRXKj9CWsYuAKqeHJ8", 50_000_000)] ) INP1 = messages.TxInputType( address_n=[H_(45), 0, 0, 0],