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

fix: fix permit signer #91

Merged
merged 1 commit into from
Oct 22, 2024
Merged

fix: fix permit signer #91

merged 1 commit into from
Oct 22, 2024

Conversation

rndquu
Copy link
Member

@rndquu rndquu commented Oct 21, 2024

Resolves ubiquity/pay.ubq.fi#323

TLDR; This PR broke permit signature in this line.

Root cause

Check this buggy permit where permit owner is 0x9051eDa96dB419c967189F4Ac303a290F3327680 but recovered signer from signature is 0x51afbf762cdedc2683afecc2ccdd93da6a071ff8 while the expected behavior for both of those values to be equal. The root cause is that here domain.version was added to the signed data while original permit2's EIP712 contract verifies only: domain.name, domain.chainId and domain.verifyingContract. Thus adding a new field to signed data made a signature created by the ubiquity-os/permit-generation package not matching the one actually verified.

This PR:

  • removes domain.version from signed data (and keeps only domain.name, domain.chainId and domain.verifyingContract originally produced by SignatureTransfer.getPermitData())
  • adds a unit test so that possible issues with permit signatures are spotted easily

The PR that broke permit signatures was introduced in ubiquity-os/permit-generation v2.0.0 and ubiquity-os-marketplace/text-conversation-rewards started using that version in v1.5.1 on 12 October. So all permits generated since the 12 October are not claimable by contributors (and probably should be regenerated on demand once a new version of ubiquity-os/permit-generation is released).

P.S. The error could not be catched locally at pay.ubq.fi because pay.ubq.fi test helper script generates a permit signature without the help of the ubiquity-os/permit-generation plugin.

@rndquu rndquu marked this pull request as ready for review October 21, 2024 13:04
@rndquu
Copy link
Member Author

rndquu commented Oct 21, 2024

Somehow I don't have permissions to ask for a code review although my account is a member of the ubiquity-os organization.

Pls review @gentlementlegen @whilefoo @0x4007

Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

@rndquu Thanks a lot for looking into this. After this is deployed on NPM, we should update text-conversation-reward package version and regenerate the permits then.
I will try to test this locally.

@rndquu
Copy link
Member Author

rndquu commented Oct 21, 2024

we should update text-conversation-reward package version

As far as I understand because of ^ here it will automatically fetch the next v2.0.5

@gentlementlegen
Copy link
Member

@rndquu It should indeed, we still need to rebuild it because we run a compiled version.

@gentlementlegen
Copy link
Member

@rndquu Somehow having a har time testing this, sorry for the delay. I followed the README to create a proper key and encrypted EVM to run it, but I run into the following issue:

  • you wrote that the key should not start with 0x but I get this error Error: Failed to instantiate wallet: Error: invalid hexlify value (argument="value", value="d9530<rest of the wallet hex>", code=INVALID_ARGUMENT, version=bytes/5.7.0)
  • when adding 0x I get Error: Failed to instantiate wallet: Error: invalid private key (argument="privateKey", value="[[ REDACTED ]]", code=INVALID_ARGUMENT, version=signing-key/5.7.0)

I'll keep debugging but you might find out faster than me.

@rndquu
Copy link
Member Author

rndquu commented Oct 21, 2024

@rndquu Somehow having a har time testing this, sorry for the delay. I followed the README to create a proper key and encrypted EVM to run it, but I run into the following issue:

  • you wrote that the key should not start with 0x but I get this error Error: Failed to instantiate wallet: Error: invalid hexlify value (argument="value", value="d9530<rest of the wallet hex>", code=INVALID_ARGUMENT, version=bytes/5.7.0)
  • when adding 0x I get Error: Failed to instantiate wallet: Error: invalid private key (argument="privateKey", value="[[ REDACTED ]]", code=INVALID_ARGUMENT, version=signing-key/5.7.0)

I'll keep debugging but you might find out faster than me.

I suppose since ethersjs was downgraded from v6 to v5 then the 0x prefix is required again.

Error: Failed to instantiate wallet: Error: invalid private key

Hard to say, you should use the format ETHEREUM_PRIVATE_KEY:GITHUB_ORGANIZATION_ID.

This test actually QAs this PR pretty much. All you have to check is that this signature was created by this owner.

@gentlementlegen
Copy link
Member

@rndquu Long story short, I had yarn link on rpc-handler into permit-generation that I was linking against conversation-rewards which was using broken code. I runs great and I could generate a permit.

@0x4007 0x4007 merged commit 72b2b99 into ubiquity-os:development Oct 22, 2024
2 checks passed
@rndquu rndquu deleted the feat/323 branch October 22, 2024 07:07
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.

Claims Broken
4 participants