search

ethereum-attestation-service / eas-sdk

Ethereum Attestation Service - TypeScript/JavaScript SDK
MIT License
89 stars 45 forks source link

Coinbase Smart Wallet EAS typed signature validation #109

Open Yuripetusko opened 1 month ago

Yuripetusko commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

Yuripetusko commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

Yuripetusko commented 1 month ago

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 commented 1 month ago

Oh, you're right. Built and updated.

Yuripetusko commented 1 month ago

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 commented 1 month ago

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

image
lbeder commented 1 month ago

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 commented 1 month ago

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

lbeder commented 1 month ago

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 commented 1 month ago

Don't hesitate to use me for debugging 😅

jxom commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

lbeder commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.