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 Safe wallet signature verification #76

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

ljiatu
Copy link
Contributor

@ljiatu ljiatu commented Oct 10, 2024

The current implementation doesn't work for Safe wallets for a couple of reasons:

  1. Safe wallets always return 0x as the signature, which would cause an IndexError when calling recover_message. Since only ValueError is being caught, the exception would bubble up and cause a failure.
  2. The signature 0x has to be passed into the isValidSignature function as-is, but an empty string is currently being passed in due to signature[2:].
  3. The wallet contract raises a ContractLogicError if the signature is invalid, but it's not caught, which also causes an eventual failure.

@sbihel
Copy link
Member

sbihel commented Oct 10, 2024

Seems related to #63

From Discord:

I'd like to add a test case but not exactly sure how, i.e. which Safe wallet should I use to generate the signature

I don't know about Safe wallet and how easy it is to generate one. But a mainnet example, or potentially Sepolia testnet, would greatly help

@ljiatu
Copy link
Contributor Author

ljiatu commented Oct 11, 2024

Seems related to #76

From Discord:

I'd like to add a test case but not exactly sure how, i.e. which Safe wallet should I use to generate the signature

I don't know about Safe wallet and how easy it is to generate one. But a mainnet example, or potentially Sepolia testnet, would greatly help

Hi @sbihel, I was able to get a testcase working on Sepolia Eth. Should I create another PR and submit it to the TS SIWE repo?
The Web3 provider URL you are using seems to be for Eth mainnet, so not sure if you can run the Sepolia eth test case with the other two EIP-1271 test cases.

@sbihel
Copy link
Member

sbihel commented Oct 11, 2024

The test can just be in this repo for the time being if it's not using mainet I reckon. Yes it will have to be separate, and if there's no public node for Sepolia we can simply configure the test to run on the CI (which is using a private node)

@ljiatu
Copy link
Contributor Author

ljiatu commented Oct 11, 2024

The test can just be in this repo for the time being if it's not using mainet I reckon. Yes it will have to be separate, and if there's no public node for Sepolia we can simply configure the test to run on the CI (which is using a private node)

Got it. I've added a new test case test_safe_wallet_message. There in fact is a public Sepolia node so I'm using that for the new test case. Also bumped the patch version given the bug fix nature.

@sbihel
Copy link
Member

sbihel commented Oct 11, 2024

Thank you. CI is failing because secrets aren't available for forks

@sbihel sbihel merged commit 9741fcf into spruceid:main Oct 11, 2024
3 of 8 checks passed
@ljiatu ljiatu deleted the fix-safe-wallet-signature-verification branch October 11, 2024 15:29
@ljiatu
Copy link
Contributor Author

ljiatu commented Oct 11, 2024

Thank you. CI is failing because secrets aren't available for forks

Got it. Thanks for pushing this over the finish line!

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