namada icon indicating copy to clipboard operation
namada copied to clipboard

Missing MASP Transaction Validation

Open murisi opened this issue 1 year ago • 2 comments

MASP transaction validation seems to be incomplete, and hence may accept MASP transactions that should be rejected. In particular it seems to be missing the following:

  • Spend description anchor validation. The anchor of each spend description must be confirmed to refer to an earlier treestate. If this is not done malicious clients will be able to spend notes from a fabricated note commitment tree. This requirement is inherited from the consensus rules subsection of https://zips.z.cash/protocol/protocol.pdf#spendsandoutputs .
  • Nullifier uniqueness enforcement. The nullifier revealed by a given spend description must not already be in the nullifier set. If this is not done malicious clients will be able to do double-spends. This requirement is inherited from the consensus rules subsection of https://zips.z.cash/protocol/protocol.pdf#nullifierset .
  • Convert description anchor validation. The anchor of a convert description must be confirmed to refer to the current conversion tree. If this is not done malicious clients will be able to apply conversions from a fabricated allowed conversion tree. This requirement is implicit in the specs, but perhaps ought to be stated more explicitly.
  • Note commitment tree capacity check. A transaction must be rejected if it would cause the note commitment tree to exceed its capacity. If this were allowed to happen the MASP shielded pool would enter an undefined state. See the consensus rules subsection of https://zips.z.cash/protocol/protocol.pdf#merkletree .

These seem to be all relevant consensus rules inherited from the Zcash spec that cannot/are not already enforced directly by the MASP crate. cc @juped @gijswijs @cwgoes @joebebel .

murisi avatar May 15 '23 06:05 murisi

Ah! I didn't realize we weren't doing these already. Is anything here likely to be particularly difficult to implement?

cwgoes avatar May 18 '23 00:05 cwgoes

We should also set the maximum block height on the Transaction object (ideally the same as the embedding Transfer even though this is not trivial since the tx expiration is optional and expressed as a DateTime), and validate it in the vp.

grarco avatar Sep 28 '23 18:09 grarco