Redundant Computation in Sapling and Orchard Note Validation

[mpguerra]

The following issue (ZcashFoundation/zebra#6392 on zebra github repo) was raised by the auditors working on the Zebra audit:

Impact

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

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:

librustzcash/zcash_primitives/src/sapling/note_encryption.rs

Lines 254 to 268 in fb49cd4

	fn parse_note_plaintext_without_memo_ovk(
	&self,
	pk_d: &Self::DiversifiedTransmissionKey,
	esk: &Self::EphemeralSecretKey,
	ephemeral_key: &EphemeralKeyBytes,
	plaintext: &NotePlaintextBytes,
	) -> Option<(Self::Note, Self::Recipient)> {
	sapling_parse_note_plaintext_without_memo(self, &plaintext.0, |diversifier| {
	if esk.derive_public(diversifier.g_d()?.into()).to_bytes().0 == ephemeral_key.0 {
	Some(*pk_d)
	} else {
	None
	}
	})
	}

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

librustzcash/components/zcash_note_encryption/src/lib.rs

Lines 514 to 537 in fb49cd4

	fn check_note_validity<D: Domain>(
	note: &D::Note,
	ephemeral_key: &EphemeralKeyBytes,
	cmstar_bytes: &D::ExtractedCommitmentBytes,
	) -> NoteValidity {
	if &D::ExtractedCommitmentBytes::from(&D::cmstar(note)) == cmstar_bytes {
	if let Some(derived_esk) = D::derive_esk(note) {
	if D::epk_bytes(&D::ka_derive_public(note, &derived_esk))
	.ct_eq(ephemeral_key)
	.into()
	{
	NoteValidity::Valid
	} else {
	NoteValidity::Invalid
	}
	} else {
	// Before ZIP 212
	NoteValidity::Valid
	}
	} else {
	// Published commitment doesn't match calculated commitment
	NoteValidity::Invalid
	}
	}

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.

[mpguerra]

Decided not to do anything about this one, see ZcashFoundation/zebra#6392 (comment)

[mpguerra]

We found a fix for this in the end

[mpguerra]

Will be fixed by the ECC team in #848 and zcash/orchard#394

Nothing else for us to do here