Created
March 30, 2020 20:50
-
-
Save dgpv/c580080cd6984fb0121b61f1e1b5db51 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ColdCard wallet firmware appears to have a vulnerability where it | |
can deem certain multisig output address as 'change address' that is actually spendable | |
by other entity than the multisig participants. | |
It checks that the pubkeys in the output redeem script match the derived | |
pubkeys for the trusted xpubs stored on the device, and if they match, the | |
output is considered 'change address'. | |
But the problem is that the firmware does not check the size of the redeem | |
script. When it disassembles the redeem script, it stops as soon as it finds | |
OP_CHECKMULTISIG opcode. | |
Excerpt from file `shared/multisig.py` function `disassemble_multisig()`: | |
``` | |
# generator | |
dis = disassemble(redeem_script) | |
# expect M value first | |
M, opcode = next(dis) | |
assert opcode == None and isinstance(M, int), 'garbage at start' | |
pubkeys = [] | |
for offset, (data, opcode) in enumerate(dis): | |
if opcode == OP_CHECKMULTISIG: | |
# should be last byte | |
break | |
if isinstance(data, int): | |
N = data | |
else: | |
pubkeys.append(data) | |
else: | |
raise AssertionError("end fall") | |
assert len(pubkeys) == N | |
assert 1 <= M <= N <= 20, 'M/N range' # will also happen if N not encoded. | |
return M, N, pubkeys | |
``` | |
It has the comment `# should be last byte`, but does not check for this. | |
This problem can be dealt with by checking that total size of the redeem | |
script matches the expected size given the number of pubkeys expected, but I | |
did not find any such checks in the code. | |
`disassemble_multisig_mn()` function in the same file only checks for first | |
byte and two last bytes of the script. | |
A script like | |
``` | |
1 <pubA> <pubB> 2 CHECKMULTISIG DROP 1 <pubM0> <pubM1> 2 CHECKMULTISIG | |
``` | |
(where pubA and pubB are pubkeys that match the xpubs on the device, while | |
pubM0 and pubM1 are pubkeys of the keys belonging to malicious actor that | |
wants to steal all the change of the ptransaction) | |
would successfully pass `disassemble_multisig_mn()` and will return N=2 M=1 | |
and also will return N=2 M=1 pubs=[<pubA>, <pubB>] from | |
`disassemble_multisig()`. | |
The output of the transaction that uses this script would pass as 'change' | |
output because the pubkeys returned from `disassemble_multisig()` would match | |
xpubs stored on the device. | |
Because CHECKMULTISIG opcopde does not stops the execution on signature | |
verification fail (as opposed to CHECKMULTISIGVERIFY), the execution | |
of the script would continue and would the second CHECKMULTISIG will execute. | |
<pubA> and <pubB> will not have a chance to spend such input, | |
because the result of the first CHECKMULTISIG is discarded. | |
Side note: if OP_SUCCESS opcode (that is proposed in bip-tapscript) would be | |
available, there would be no need for <pubM1> as the script could end with | |
`<pubM> CHECKSIGVERIFY SUCCESS` |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment