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

Coinbase Smart Wallet EAS typed signature validation #109

Open
Yuripetusko opened this issue Jul 15, 2024 · 24 comments
Open

Coinbase Smart Wallet EAS typed signature validation #109

Yuripetusko opened this issue Jul 15, 2024 · 24 comments

Comments

@Yuripetusko
Copy link

I keep getting invalid raw signature lenght when calling signOffchainAttestation with signer derived from Coinbase Smart Wallet. Same code works fine with EOA.

Is it a known issue by any chance where ethers fails to validate typed signature created by smart account wallets?

@Yuripetusko
Copy link
Author

Looks like the eas-sdk assumes that signer is always of EOA type, and ethers signature verification don't work here for smart account wallets. Need to use a better signature verification

https://www.smartwallet.dev/guides/signature-verification#offchain

@lbeder
Copy link
Member

lbeder commented Jul 15, 2024

Hi @Yuripetusko,
We will check this out asap. Stay tuned.

@Yuripetusko
Copy link
Author

Hi @Yuripetusko, We will check this out asap. Stay tuned.

Perfect, I was almost ready to start rewriting eas-sdk to viem :D But if you can solve this, that would be much easier

@Yuripetusko
Copy link
Author

Just adding some more info. The error is thrown here:

https://github.com/ethereum-attestation-service/eas-sdk/blob/master/src/offchain/typed-data-handler.ts#L145

Which then triggers this validation on ether's Signature:

https://github.com/ethers-io/ethers.js/blob/fc66b8ad405df9e703d42a4b23bc452ec3be118f/src.ts/crypto/signature.ts#L302

Some more context here: https://warpcast.com/pentacle.eth/0x05114005

@lbeder
Copy link
Member

lbeder commented Jul 15, 2024

@Yuripetusko
Copy link
Author

Yuripetusko commented Jul 15, 2024

Hey @Yuripetusko, could you try this version? https://github.com/ethereum-attestation-service/eas-sdk/tree/erc-6492

Hey, @lbeder you need to buld it I think, as dist folder doesn't contain this new function from viem that you added

@lbeder
Copy link
Member

lbeder commented Jul 15, 2024

Oh, you're right. Built and updated.

@Yuripetusko
Copy link
Author

Oh, you're right. Built and updated.

Unfortunatly the same error :(

TypeError: invalid raw signature length (argument="signature", value="0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000c00000000000000000000000000000000000000000000000000000000000000120000000000000000000000000000000000000000000000000000000000000001700000000000000000000000000000000000000000000000000000000000000016504bc6fea1ac7c44ac88e5e2914bd0c5af345bcd4f7bc0d9eeb65993b3ed761725e538c02d1c9475c4422a9f44fcc82558aac9e728ca8dba6711d3d6e3e493b0000000000000000000000000000000000000000000000000000000000000025f198086b2db17256731bc456673b96bcef23f51d1fbacdd7c4379ef65465572f1d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008a7b2274797065223a22776562617574686e2e676574222c226368616c6c656e6765223a22727441317267636863765f6f45534e704c2d47756b486c3368376277503531534b6b78734634716b586a77222c226f726967696e223a2268747470733a2f2f6b6579732e636f696e626173652e636f6d222c2263726f73734f726967696e223a66616c73657d00000000000000000000000000000000000000000000", code=INVALID_ARGUMENT, version=6.13.1)
    at makeError (errors.js:122:21)
    at assert (errors.js:149:15)
    at assertArgument (errors.js:161:5)
    at assertError (signature.js:227:43)
    at Signature.from (signature.js:249:13)
    at Offchain.signTypedDataRequest (typed-data-handler.js:49:46)
    at async Offchain.signOffchainAttestation (offchain.js:141:31)
    at async sign (reactions.tsx:190:28)
    at async submit (reactions.tsx:152:35)
reactions.tsx:41 

@Yuripetusko
Copy link
Author

And I can confirm that the latest code is in my bundle

image

@lbeder
Copy link
Member

lbeder commented Jul 16, 2024

This is strange. It means that parseErc6492Signature failed to recognize that this is a valid ERC-6492 signature and returned it as is. I'll continue looking at it tomorrow 🙏

@Yuripetusko
Copy link
Author

Perhaps viem's verifyTypedData should be used instead of ethers Signature.from ?

@lbeder
Copy link
Member

lbeder commented Jul 16, 2024

Perhaps viem's verifyTypedData should be used instead of ethers Signature.from ?

viem's verifyTypedData will require too many adaptations, while parseErc6492Signature seems exactly what we need right now. I'll try to finish this approach and if it doens't work - we can consider verifyTypedData.

@Yuripetusko
Copy link
Author

Don't hesitate to use me for debugging 😅

@jxom
Copy link

jxom commented Jul 16, 2024

You shouldn't be parsing the response of signTypedData with parseErc6492Signature, since offchain secp256k1 verification does not support all SCA signatures. Instead, you should pass the raw signature onto a 6492-compliant signature verifier (ie. a verifyTypedData function that supports onchain verification).

@lbeder
Copy link
Member

lbeder commented Jul 16, 2024

You shouldn't be parsing the response of signTypedData with parseErc6492Signature, since offchain secp256k1 verification does not support all SCA signatures. Instead, you should pass the raw signature onto a 6492-compliant signature verifier (ie. a verifyTypedData function that supports onchain verification).

Yeah, it might be inevitable. We are working on it and will keep everyone posted 🙏

@lbeder
Copy link
Member

lbeder commented Jul 17, 2024

Could you guys expand more on your case? Why do you require ERC-6492 signatures (i.e., passing 'smartWalletOnly')? Fully support ERC-6492 will also require contract changes, coordinating upgrades where possible (e.g., Optimism, Base), which takes lots of time and effort, and will also require a full backward compatibility layer to support older, non-upgradeable, instances of EAS. Replacing the verification with verifyTypedData is only a tiny part of it, so it'd be great to understand the use case and the demand for this big effort first.

@Yuripetusko
Copy link
Author

Could you guys expand more on your case? Why do you require ERC-6492 signatures (i.e., passing 'smartWalletOnly')? Fully support ERC-6492 will also require contract changes, coordinating upgrades where possible (e.g., Optimism, Base), which takes lots of time and effort, and will also require a full backward compatibility layer to support older, non-upgradeable, instances of EAS. Replacing the verification with verifyTypedData is only a tiny part of it, so it'd be great to understand the use case and the demand for this big effort first.

My specific case came from helping with some code on https://www.welcomeonchain.xyz/ which aims to be a go to directory for web3 projects with a focus on Base ecosystem. They use EAS with a specific schema to allow people to review and rate projects (as an early demo of the upcoming EAS-centric tooling/infra called blossom.land).

Since the welcome onchain web app is focused on Base ecosystem, many users will be using Coinbase Smart Wallet, and they are looking for a way to allow them to create EAS with it.

So in this particular case it's using EAS on Base/Base-sepolia with a specific schema on a specific project to review and rate projects, which is a pre-requesete for a standalone eas-based library/infra.

So even if I would rewrite the sdk to viem, the eas wouldn't work? Is there perhaps some non-generalised solution that I can implement for my specific case before a more generalised solution can be considered?

@lbeder
Copy link
Member

lbeder commented Jul 17, 2024

Yeah, the problem isn't viem or even Coinbase Wallet SDK, but that your force ERC-6492 signatures by providing the 'smartWalletOnly' option when creating a wrapped provider. In order to fully support ERC-6492 signatures, we will also need to modify the contracts (and everything related to that).

Maybe this is actually unnecessary for this use case (ERC-6492 solves a very specific problem and this project might be using it as default by accident) and everything will work with 'eoaOnly' too).

@Yuripetusko
Copy link
Author

Yeah, the problem isn't viem or even Coinbase Wallet SDK, but that your force ERC-6492 signatures by providing the 'smartWalletOnly' option when creating a wrapped provider. In order to fully support ERC-6492 signatures, we will also need to modify the contracts (and everything related to that).

Maybe this is actually unnecessary for this use case (ERC-6492 solves a very specific problem and this project might be using it as default by accident) and everything will work with 'eoaOnly' too).

The wallet sdk config is set to all which prompts user to choose between smart wallet or eoa. Then I create an ethers compatible signer instance from the wallet provider and pass it to eas-sdk. setting coinbase wallet sdk to eoaOnly defeats the purpose of allowing users to use smart wallets 🤔

Is this about differentiating users just just created smart wallet account (that is not deployed yet) VS smart wallet users who's account is already deployed? I can try it with a smart wallet that is already deployed and see if this changes anything, but somehow I doubt it..

@Yuripetusko
Copy link
Author

@lbeder
Copy link
Member

lbeder commented Jul 18, 2024

So as I understand the problem is somewhere here right?

https://github.com/ethereum-attestation-service/eas-contracts/blob/f00a7e1f13475a49f9d3ef01d6a88d3a9961989e/contracts/eip1271/EIP1271Verifier.sol#L105

It's not exactly a problem. It just makes supporting ERC-6492 a big effort, which will also require coordinating upgrades (where applicable) and a new backwards compatibility layer (where upgrades aren't), so it might wait for v2. ERC-6492 is also not really adopted and might change a few times before becoming an industry standard, so adding it now is also a little bit premature.

@lbeder
Copy link
Member

lbeder commented Jul 18, 2024

Yeah, the problem isn't viem or even Coinbase Wallet SDK, but that your force ERC-6492 signatures by providing the 'smartWalletOnly' option when creating a wrapped provider. In order to fully support ERC-6492 signatures, we will also need to modify the contracts (and everything related to that).
Maybe this is actually unnecessary for this use case (ERC-6492 solves a very specific problem and this project might be using it as default by accident) and everything will work with 'eoaOnly' too).

The wallet sdk config is set to all which prompts user to choose between smart wallet or eoa. Then I create an ethers compatible signer instance from the wallet provider and pass it to eas-sdk. setting coinbase wallet sdk to eoaOnly defeats the purpose of allowing users to use smart wallets 🤔

Is this about differentiating users just just created smart wallet account (that is not deployed yet) VS smart wallet users who's account is already deployed? I can try it with a smart wallet that is already deployed and see if this changes anything, but somehow I doubt it..

But why do they need 'all' in the first place? How are they using ERC-6492 and why is it required for this use case? Only because they want to support a "Smart Wallet"?

@Yuripetusko
Copy link
Author

But why do they need 'all' in the first place? How are they using ERC-6492 and why is it required for this use case? Only because they want to support a "Smart Wallet"?

The website is aimed towards new web3 user onboarding (there is an educational section there), hence the name "Welcome onchain". The Base team wants to position Coinbase Smart wallet as the best method to onboard non web3 native users. And although project reviews using EAS are not strictly part of the educational section of the app, it is part of the website and a way to rate projects for new and seasonal users. I don't have exact stats, but I would say a fair number of visitors have smart wallet as their primary wallet

@lbeder
Copy link
Member

lbeder commented Jul 18, 2024

But why do they need 'all' in the first place? How are they using ERC-6492 and why is it required for this use case? Only because they want to support a "Smart Wallet"?

The website is aimed towards new web3 user onboarding (there is an educational section there), hence the name "Welcome onchain". The Base team wants to position Coinbase Smart wallet as the best method to onboard non web3 native users. And although project reviews using EAS are not strictly part of the educational section of the app, it is part of the website and a way to rate projects for new and seasonal users. I don't have exact stats, but I would say a fair number of visitors have smart wallet as their primary wallet

Got it. Let us discuss this with Coinbase directly and see what is the best path to move forward.

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

No branches or pull requests

3 participants