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