coldcard-multisig-change-vuln.txt

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`