[NCC-E005955-MU2] zebra-consensus: Redundant Computation in Sapling and Orchard Note Validation

[mpguerra]

Impact

During Sapling and Orchard output validation, one redundant elliptic curve scalar multiplication is performed. It may be considered for removal for efficiency purposes.

This is an issue in librustzcash:
zcash/librustzcash#802

Description

In order to verify that Sapling and Orchard outputs are decryptable and consistent with ZIP 212 rules, Zebra uses the zcash_primitives crate’s try_sapling_output_recovery function. This function is called, for example, when coinbase transaction outputs are validated to adhere to the rules specified in ZIP 212.

Once decrypted, an output’s note is parsed. An ephemeral key validation function is passed as a lambda:

The check implemented by the highlighted lambda function is mandated by ZIP 212. A few lines below, the check_note_validity function is called:

The validation highlighted in the first code snippet happens regardless of whether ZIP 212 is activated or not. This is also what the original Zcash client does; see zcash/Note.cpp.
The validation highlighted in the second code snippet aims to only validate the public key post-ZIP 212 and is likely meant to be a blanket end-of-function validation helper. This helper includes an ECC point multiplication and as such is not inexpensive.

Recommendation

It appears that the highlighted check inside the check_note_validity function overlaps with the previous validation, and, as such, may be removed.

[teor2345]

@mpguerra this isn't a bug in Zebra, it's in an ECC dependency that is out of scope for the audit.

[mpguerra]

@mpguerra this isn't a bug in Zebra, it's in an ECC dependency that is out of scope for the audit.

That's right. We want to track everything raised by the auditors and try to fix it if it makes sense.

[teor2345]

@mpguerra this isn't a bug in Zebra, it's in an ECC dependency that is out of scope for the audit.

That's right. We want to track everything raised by the auditors and try to fix it if it makes sense.

@mpguerra is there an equivalent librustzcash issue?

[mpguerra]

@mpguerra is there an equivalent librustzcash issue?

There is now! zcash/librustzcash#802

[upbqdn]

The function check_note_validity, where the redundancy occurs, is also called in

• parse_note_plaintext_without_memo_ivk, which is called in
• try_note_decryption_inner, which is called in
• try_note_decryption, which is a public function for trial decryption.

The call stack above doesn't contain the epk check from parse_note_plaintext_without_memo_ovk mentioned in the finding; thus, removing the check from check_note_validity might cause unexpected errors.

It would be beneficial to separate the two call stacks and remove the redundancy from the affected one, but my estimate is that the effort required for the refactoring doesn't match the benefits. I, therefore, propose we close this issue without addressing it.

[mpguerra]

@upbqdn can you please add a size for this issue based on how much effort it took to investigate and analyze this?

[upbqdn]

Done.