search

ubiquity-os / permit-generation

A standalone module to generate permits.
1 stars 19 forks source link

fix: fix permit signer #91

Closed rndquu closed 2 weeks ago

rndquu commented 2 weeks ago

Resolves https://github.com/ubiquity/pay.ubq.fi/issues/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:

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 commented 2 weeks ago

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

rndquu commented 2 weeks ago

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 commented 2 weeks ago

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

gentlementlegen commented 2 weeks ago

@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:

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

rndquu commented 2 weeks ago

@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 commented 2 weeks ago

@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.