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

Possible Overflow in username in big_intify_username combined with calling big_uint_to_fp #16

Open
bbresearcher opened this issue Mar 9, 2024 · 0 comments

Comments

@bbresearcher
Copy link
Collaborator

bbresearcher commented Mar 9, 2024

NB Need to give credit to Jin.s for the guidance to find the bug
Describe the bug
A malicious prover could create usernames that overflow if two users have the same balance thus they can exclude one of the records from the data.

Checked the bug with the developers below is a response from the developers.
Copy/Paste from Discord chat

Thank you for pointing that out. I understand the issue now.

Let's assume we have a username converted to 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000002, and another user with the identity 0x1, both of whom share the same user identity in a Merkle sum tree. This indicates a problem, as user identities in the Merkle sum tree should not be duplicated. You've made a great observation here.

In a worst-case scenario, two users share the same identity in the Merkle sum tree:
UserA: 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000002
UserB: 0x1
And both have the same amount of currencies. In this case, a malicious prover might exclude one of the elements from the tree and provide the same proof to different users with different usernames.
Although we add an assertion check for overflow, a malicious prover could bypass this. Therefore, it's important to inform users that converting their user identity over the modulus could lead to malicious activity.

From the perspective of the current implementation, this issue might not affect the generation of inclusion proofs for specific users, as the backend generates proofs using the user index rather than the user identity.

We should add a overflow check for username in on-chain verifier rather than rust implementation.

Where is this code used
https://github.com/summa-dev/summa-solvency/blob/master/zk_prover/src/circuits/merkle_sum_tree.rs#L252

https://github.com/summa-dev/summa-solvency/blob/master/zk_prover/src/merkle_sum_tree/tree.rs#L74

https://github.com/summa-dev/summa-solvency/blob/master/zk_prover/src/merkle_sum_tree/node.rs#L21

Steps to recreate

    let usr1 = big_intify_username("30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000002");
    let ua = big_uint_to_fp(&usr1);
    println!("User1 value is : {:?}",ua);
    let usr2 = big_intify_username("1");
    let ub = big_uint_to_fp(&usr1);
    println!("User2 value is : {:?}",ub);
    if ua == ub {
        println!("The two user values are equal");
    }

Gives output of:

User1 value is : 0x29fc300ca92b9374edb232c3567cceda058441c9c0c0d886ed5577f6f3ead4f2
User2 value is : 0x29fc300ca92b9374edb232c3567cceda058441c9c0c0d886ed5577f6f3ead4f2
The two user values are equal

Expected behavior
Usernames should be unique all the time

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

No branches or pull requests

1 participant