Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing MASP Transaction Validation #1373

Closed
Tracked by #2020
murisi opened this issue May 15, 2023 · 2 comments · Fixed by #2248
Closed
Tracked by #2020

Missing MASP Transaction Validation #1373

murisi opened this issue May 15, 2023 · 2 comments · Fixed by #2248
Assignees
Labels

Comments

@murisi
Copy link
Collaborator

murisi commented May 15, 2023

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 murisi added the bug Something isn't working label May 15, 2023
@murisi murisi self-assigned this May 15, 2023
@murisi murisi added the MASP label May 15, 2023
@cwgoes
Copy link
Collaborator

cwgoes commented May 18, 2023

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

@grarco
Copy link
Collaborator

grarco commented Sep 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants