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

specification: clarify swap claim integrity checks #4484

Merged
merged 4 commits into from
May 29, 2024

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented May 26, 2024

Describe your changes

Specifies components of the integrity checks in the swap claim.

This references component B in the ECC audit log.

Issue ticket number and link

References #4483

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

@TalDerei
Copy link
Collaborator Author

TalDerei commented May 26, 2024

there are a few things to note here:

  1. The swap claim specification references that the body of a SwapClaim has "a balance commitment, which commits to the value balance of the spent note". This is inconsistent with the protobuf definition for a SwapClaimBody.

  2. Regarding the fee consistency check: in constructing the swapClaim plan, it uses the claimFee from the SwapPlaintext as the claimFee field inside SwapClaimProofPublic. Then, in the fee consistency check during circuit constraint generation, it uses the same SwapPlaintext inside SwapClaimProofPrivate. It’s comparing the swap claim fee from the same source, meaning the fee consistency check isn’t actually doing anything useful here from what I can tell?

@TalDerei TalDerei added the ecc-component-remediated Tag PRs that are remediating ecc findings label May 26, 2024
@cratelyn cratelyn added this to the Sprint 7 milestone May 28, 2024
@cratelyn cratelyn added the A-docs Area: Documentation needs for the project label May 28, 2024
@redshiftzero
Copy link
Member

redshiftzero commented May 28, 2024

The swap claim specification references that the body of a SwapClaim has "a balance commitment, which commits to the value balance of the spent note". This is inconsistent with the protobuf definition for a SwapClaimBody.

correctly pointed out, this is done in #4485 so leaving out here

Regarding the fee consistency check: in constructing the swapClaim plan, it uses the claimFee from the SwapPlaintext as the claimFee field inside SwapClaimProofPublic. Then, in the fee consistency check during circuit constraint generation, it uses the same SwapPlaintext inside SwapClaimProofPrivate. It’s comparing the swap claim fee from the same source, meaning the fee consistency check isn’t actually doing anything useful here from what I can tell?

This is true for an honest prover, but a malicious prover can construct a SwapClaim with an arbitrary public claim fee, however the ZKP demonstrates that it must match the claim fee from the SwapPlaintext

@TalDerei
Copy link
Collaborator Author

This is true for an honest prover, but a malicious prover can construct a SwapClaim with an arbitrary public claim fee, however the ZKP demonstrates that it must match the claim fee from the SwapPlaintext

Good point, I should've been strictly thinking about this from the vantage point of a malicious prover.

@redshiftzero redshiftzero marked this pull request as ready for review May 29, 2024 18:07
@redshiftzero redshiftzero merged commit 319a3d9 into main May 29, 2024
13 checks passed
@redshiftzero redshiftzero deleted the swap-claim-specification branch May 29, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation needs for the project ecc-component-remediated Tag PRs that are remediating ecc findings
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants