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

feat: constrain (r, s, v) signature values #259

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Conversation

karmacoma-eth
Copy link
Collaborator

Sort of an integration test to make sure that the (v, r, s) values we generate can be encoded and decoded properly in ECDSA.tryRecover
+ document missing feature in our implementation of ecrecover
"""Extract bytes from calldata. Zero-pad if out of bounds."""
if data is None:
return BitVecVal(0, size_bytes * 8)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a regression test that would fail without this fix? (it's not clear when None can be passed to this function.) also, does this fix the vm.etch(x, "") case as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's related but separate from the vm.etch(x, "") issue, we don't have a very nice way to deal with empty bytes in general. They get sometimes represented as bytes(), sometimes as [], sometimes as None

the test that revealed this issue is check_ecrecover_invalidCallReturnsNothing, in the following line:
let succ := call(gas(), ECRECOVER_PRECOMPILE, 0, 0, 0, 0, 0)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. now i can see where None comes from. as you mentioned, i agree that we need a better way to handle nicely empty bytes. could you please open an issue about this to discuss further later?

@daejunpark daejunpark merged commit 30c6bb7 into main Mar 26, 2024
41 checks passed
@daejunpark daejunpark deleted the signature-constraints branch March 26, 2024 17:11
fubuloubu pushed a commit to ApeWorX/halmos that referenced this pull request Apr 11, 2024
@daejunpark
Copy link
Collaborator

@karmacoma-eth wdyt?
30c6bb7#r141719157

@karmacoma-eth
Copy link
Collaborator Author

@karmacoma-eth wdyt? 30c6bb7#r141719157

replied to the question on the commit, but copying here just in case:

I don't think we can do that unfortunately, EIP-2 specifies only the behavior for transaction signatures.

AFAIK, the ecrecover precompile is still perfectly happy with s values in the full range

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.

2 participants