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

Restrict types of message in eddsa-poseidon's signMessage #230

Open
artwyman opened this issue Mar 23, 2024 · 8 comments · May be fixed by #318
Open

Restrict types of message in eddsa-poseidon's signMessage #230

artwyman opened this issue Mar 23, 2024 · 8 comments · May be fixed by #318
Assignees
Labels
good first issue Good for newcomers refactoring ♻️ A code change that neither fixes a bug nor adds a feature

Comments

@artwyman
Copy link
Collaborator

Describe the improvement you're thinking about

This issue is conceptually similar to #188, now applied to the message argument to eddsa-poseidon's signMessage rather than the private key. This argument is typed as BigNumberish and the docs don't say anything special about how it's interpreted. The message of the underlying Poseidon algorithm is expected to be a single field element (bigint mod r).

The checkMessage function used to check this input allows arbitrary strings. If a string is valid decimal or hex, it'll be converted to a bigint as digits. Otherwise it'll be converted to a Buffer as raw bytes. This works so long as the string isn't more than 32 characters. If it's longer, it'll trigger an exception later from leBigIntToBuffer which uses a size argument.

This might be intended to be a convenience feature for signing string messages. If so, it would need to be more clearly documented. I think the size limitation and the potential for confusion about whether a small string should be treated as digits or bytes suggests it would be better to remove this case and reject string inputs which can't be interpreted as stringified bigints.

Additional context
I came across this when writing tests for handling of bad inputs in the Zupass repo. I passed a string expecting a TypeError and was surprised to get no exception. I've added my own type check to avoid the potential confusion here, so I'm not blocked by this issue.

@artwyman artwyman added the refactoring ♻️ A code change that neither fixes a bug nor adds a feature label Mar 23, 2024
@cedoor cedoor added this to ZK-Kit Mar 25, 2024
@cedoor cedoor added this to the Beta milestone Mar 25, 2024
@cedoor cedoor moved this to ♻️ Grooming in ZK-Kit Mar 25, 2024
@cedoor cedoor removed this from the Beta milestone Apr 19, 2024
@cedoor cedoor added the good first issue Good for newcomers label Apr 25, 2024
@f3r10
Copy link

f3r10 commented Aug 25, 2024

hi @cedoor could I take this issue? I am part of the PSE 2024 core program

@cedoor
Copy link
Member

cedoor commented Aug 26, 2024

@f3r10 sure! Is there enough information?

@cedoor cedoor moved this from ♻️ Grooming to 🗒 Tasks in ZK-Kit Aug 26, 2024
@f3r10
Copy link

f3r10 commented Aug 26, 2024

@f3r10 sure! Is there enough information?

thanks @cedoor. for now, I think I understand the issue. I will let you know if something is missing.

@f3r10
Copy link

f3r10 commented Aug 27, 2024

@cedoor the only type supported for the message parameter should be bitint or bigint | number | Buffer | Uint8Array?

@cedoor
Copy link
Member

cedoor commented Aug 29, 2024

@cedoor the only type supported for the message parameter should be bitint or bigint | number | Buffer | Uint8Array?

message is limited to 32 bytes and is converted to bigint internally, so one simple option (which would make it more consistent with the other types) would be to have BigNumber imo. In case we need to add documentation to explain how devs can convert text or buffers to bigint tho (should happen rarely).

Tagging @artwyman here. Wdyt?

@f3r10
Copy link

f3r10 commented Aug 30, 2024

oh ok, using a BigNumber I created an additional unit test with

        const message = Buffer.from(crypto.getRandomValues(31))

with 31 as an example, the test passes but with 34 the test fails. Would be that ok?

@cedoor
Copy link
Member

cedoor commented Sep 2, 2024

Would be that ok?

Yes, please make sure to update/add tests to check the correct type. BigNumber is either bigint or string (string must still be a stringified bigint).

@cedoor cedoor moved this from 🗒 Tasks to 🏗 In Progress in ZK-Kit Sep 4, 2024
@cedoor cedoor removed the status in ZK-Kit Sep 4, 2024
@cedoor cedoor moved this to 🏗 In Progress in ZK-Kit Sep 4, 2024
f3r10 added a commit to f3r10/zk-kit that referenced this issue Sep 7, 2024
@f3r10 f3r10 linked a pull request Sep 7, 2024 that will close this issue
7 tasks
@f3r10
Copy link

f3r10 commented Sep 7, 2024

hi! @cedoor I just opened a PR about this issue, please let me know if that would be ok or if something else has to be done.

@cedoor cedoor moved this from 🏗 In Progress to 👀 In Review in ZK-Kit Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactoring ♻️ A code change that neither fixes a bug nor adds a feature
Projects
Status: 👀 In Review
Development

Successfully merging a pull request may close this issue.

3 participants