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

overflow checks #628

Merged
merged 6 commits into from
Apr 28, 2022
Merged

overflow checks #628

merged 6 commits into from
Apr 28, 2022

Conversation

Westlad
Copy link
Contributor

@Westlad Westlad commented Apr 25, 2022

NB: To test this, you will need to delete your proving_files volume so that new keys are created. docker compose down -v will do this. Also, test without stubs because the circuits are changed, thus a stubbed test is inadequate.

The nullifier is truncated to 31 bytes to prevent overflow but this truncation is not checked. This means that new nullifiers can be created by adding high-order bits to the nullifier, even without exceeding the modulus.

This PR fixes that by requiring the higher-order byte of the nullifier is zero. If not, a challenge is raised. This is slightly more efficient than including the check in the circuit, which would make the proof bigger but requires less code.

We also need to produce nullifier hashes with a zero higher order byte so that we don't fail our own check! This is done in the Nullifier class.

While we're at it, we make similar checks of the ERC addresses and commitments in the transaction, and that the root is less than the group order. These checks don't cover an obvious security issue, but we do it anyway, in case there's a less obvious one!

TokenIDs and Token values are not checked because these use u32 variables in the circuit, so cannot be overflowed in a useful way. Other variables have data types that are smaller than the group order and so cannot be attacked in this way.

The compressedSecrets were also truncated. This is an issue because an adversary could manipulate the higher bits to create encrypted secrets that the recipient cannot decrypt. This is more of a nuisance than a real attack but it does give the recipient plausible deniability of receipt. It is more complex to fix because it requires circuit witness changes (truncating a hash value just reduces entropy, truncating a compressed secret throws away required information) . The compressed secrets can be bigger than the group order because they contain an extra parity bit. We thus pass them into the circuit as a separate parity bit and an ordinate. This avoids any truncation.

@Westlad Westlad added bug DNM Do not merge labels Apr 25, 2022
@Westlad Westlad removed the DNM Do not merge label Apr 27, 2022
@Westlad Westlad requested a review from IlyasRidhuan April 28, 2022 08:22
@ChaitanyaKonda ChaitanyaKonda merged commit faaa2fd into master Apr 28, 2022
@ChaitanyaKonda ChaitanyaKonda deleted the westlad/truncation-attack branch April 28, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants