[ZIP 244] preserves a bug from BIP-143, forcing hardware wallets to verify all transparent prevouts

[krnak]

TL;DR: The signed data should include all prevout amounts and all prevout script public keys (unless ANYONECANPAY flag is set).

Problem

Transparent UTXOs are referred as pair of previous txid and index. In order to verify UTXO's amount in HWW (hardware wallet), whole previous transaction containing this UTXO must be streamed into the device. This increases complexity of signing process significantly.

BIP-143 (SegWit) attempted to address this issue by concatenating input amount to signed data. Unfortunately, there is a bug, found by Saleem Rashid [1]. Due to this design flaw an attacker could create two Segwit transactions which, when combined together, can send the coins from wallet via a massive transaction fee.

Same bug repeats in ZIP-143, ZIP-243 and ZIP-244.

Solution

BIP-341 (Taproot) successfully fixed the issue by concatenating hash of all amounts to the signed data. Further, it also concatenates all public keys of previous outputs to prevent a similar type of attack related to external inputs [2].

I recommend modifying ZIP-244 to adapt BIP-341 approach to finally get rid of this issue.

Two new fields should be introduced:

• amounts_digest (32 bytes) - the hash of the serialization of all spent output amounts
• scriptpubkeys_digest (32 bytes) - the hash of all spent outputs' scriptPubKeys, serialized as script inside CTxOut

These two fields should be part of signed data iff ANYONECANPAY flag is not set.

@andrewkozlik @daira

[str4d]

If we made this change, it would be specifically to S.2: transparent_sig_digest. Currently, in ZIP 244 that digest is a BLAKE2b-256 hash of:

S.2a: prevouts_sig_digest (32-byte hash)
S.2b: sequence_sig_digest (32-byte hash)
S.2c: outputs_sig_digest  (32-byte hash)
S.2d: txin_sig_digest     (32-byte hash)

where the values of these digests depend on the sigtype. We would add the two new fields in between outputs_sig_digest and txin_sig_digest. It might also be possible to simplify the definition of txin_sig_digest, but I'd be inclined to leave that as-is, so even with ANYONECANPAY set, each input commits to its own value and script.

No change would need to be made for Sapling or Orchard signatures. As with ZIP 143 and ZIP 243, they are not subject to this bug, as shielded inputs are committed to directly (via the nullifier) instead of indirectly (via the prevout (txid, n)), so hardware wallets can check the input amounts directly.

[krnak]

I came to the absolutely same conclusion as @str4d, when I was thinking about these changes in more detail.

[nuttycom]

@str4d I think that these two fields should perhaps instead be one level lower, part of S.2d (https://zips.z.cash/zip-0244#s-2d-txin-sig-digest)? The reason for this is that we share the same structure of the hash computation for the S.2a...S.2c bits with https://zips.z.cash/zip-0244#t-2-transparent-digest. We use the same evaluator for both hash trees, with the 2d component being ignored when not creating a signature.

[nuttycom]

zcash/librustzcash#472 implements my suggestion here. @str4d if this looks reasonable I'll create a PR for the ZIP. cc/ @daira @teor2345

[teor2345]

zcash/librustzcash#472 implements my suggestion here. @str4d if this looks reasonable I'll create a PR for the ZIP. cc/ @daira @teor2345

Looks good to me!

Did you want to make the zcash_script module changes, or did you want us to?

We'll want a v5 zcash_script precomputation function that accepts a list of spent transparent outputs:

• in input order,
• serialized to bytes using the chain format, and
• concatenated into a single byte vector (like the existing transaction bytes).

Similarly, a new v5 non-precompute function will need the output data for every input script validation call.

There are some extra fields in each output that we're not signing, but it's easier to use the chain format, and discard those bytes. (Or parse them and not use the fields.)

If it's easier, we can just add arguments to the existing zcash_script functions. We're going to have to get all the outputs upfront for v5, so there's no reason to do it differently for v4.

[str4d]

@str4d I think that these two fields should perhaps instead be one level lower, part of S.2d (https://zips.z.cash/zip-0244#s-2d-txin-sig-digest)? The reason for this is that we share the same structure of the hash computation for the S.2a...S.2c bits with https://zips.z.cash/zip-0244#t-2-transparent-digest. We use the same evaluator for both hash trees, with the 2d component being ignored when not creating a signature.

The reason I said S.2 instead of S.2d is because the latter is specifically documented as (emphasis added):

a BLAKE2b-256 hash of the following properties of the transparent input being signed

The two new fields are not properties of the transparent input being signed; they are properties of the transaction as a whole. Hence it feels to me that they belong as commitments in the S.2 digest input (which covers the transparent parts of the transaction) rather than the S.2d digest input.

The way I'd see the hash input for S.2 being defined now is txid_part || signature_part, where txid_part is the same as in T.2, and signature_part is the concatenation of the signature-specific fields amounts_digest || scriptpubkeys_digest || txin_sig_digest. Then signature_part is ignored as a whole. If making that kind of change to our implementation is not possible, it suggests the implementation is too highly coupled to the exact specifics of ZIP 244, which is likely to also make it more brittle to adapt to subsequent changes from future pools.

That being said, it probably doesn't matter precisely where these commitments end up. Given that we designed the tree structure to enable chaining parts of the transaction to a block header, I'm wary of modifying the definition of txin_sig_digest, but I also recall we were mainly focused on chaining the shielded parts for compact blocks, so maybe the transparent part doesn't matter as much. If there's consensus to redefine S.2d as being for "signature-specific transaction-level commitments", rather than "transparent input-specific commitments", I won't block it.

[daira]

I'm agnostic as to whether the new fields go in S.2 or S.2d. If the latter, then the documentation of what S.2d covers can easily be updated, and I am slightly inclined to prefer that. Either option works from a security point of view.

[nuttycom]

My preference here is to redefine S.2d so as to preserve the relationship with the txid; I think the implementation reflects the semantics we want, which is that the signature part is computed on a per-input basis; it's just that we now want that per-input commitment to also commit to the specific breakdown of the full transparent value of the transaction. The lens I'm using here is "the context in which this input is being spent is considered a property of the input for the purposes of signing."

[str4d]

In ZIP sync today, @daira, @nuttycom and I looked at BIP 341 more closely. We decided to make changes to bring the transparent part closer to BIP 341:

• We will bring in a change to the semantics of sequence_sig_digest that was made in BIP 341 (rationale). The effect of this is to delete , and the sighash type is neither SIGHASH_SINGLE nor SIGHASH_NONE from S2.b.
• The two new fields would go into S.2, not S.2d, to match the way these fields are treated (as transaction-level data) in BIP 341. They will be inserted between the (currently-numbered) S.2a and S.2b (matching the way they are ordered in BIP 341, grouping four commitments together underneath a single conditional).
  • The personalisations of the two new fields will be ZTxTrAmountsHash and ZTxTrScriptsHash.
• We will remove the first sentence of S.2, and replace If we are producing a hash for signature over a transparent input, with In the case that transparent inputs or outputs are present, in the second sentence. This is because if the shielded inputs do not commit to all of the transparent input values, there is an equivalent attack in the case where the hardware wallet is only signing shielded inputs, in a transaction that also contains transparent inputs from a malicious other party (where that party lies about their coin's values).
  • We then need to copy over the In the case that the transaction has no transparent components section from T.2 into S.2 to the equivalent spot, since we will no longer be incorporating T.2 directly.
• In order to enable the above, we modify S.2d to add a "not a transparent input" variant to txin_sig_digest, which is just the digest over the empty byte array (as with other sentinel values). This value is used when calculating the sighash for shielded inputs.
• We'll swap the order of S.2d.ii and S.2d.iii to match the order in which they are committed to in BIP 341 (and also at the transaction level in the two new fields).
  • Note however that we will not stop committing to the per-input data if SIGHASH_ANYONECANPAY is not set. BIP 341 replaces this duplicate data with the index of the input (to reduce duplicate hashing of data while continuing to prevent collisions between signed data for different inputs), but that would introduce a third kind of serialization for S.2d, and the complexity of this isn't worth the savings (since we're still computing a BLAKE2b hash in all cases).

[str4d]

#577 implements the above changes. Additionally, while working on this with Daira, I noticed that ZIP 244 wasn't committing to hash_type (which we were doing for ZIPs 143 and 243). #577 adds hash_type to the transparent component, and additionally brings in two new consensus rules on hash_type that BIP 341 introduced (continuing with the previously-discussed rationale of having the transparent component of ZIP 244 align more closely with BIP 341).