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

feat: update verifySignature to use viem client #298

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

stephancill
Copy link
Contributor

This PR updates the verifySignature function to use a viem client so that it can also verify smart contract wallet signatures.

Open questions:

  1. Should the public client be connected to mainnet or optimism? I would assume mainnet because optimism would require updating the domain's chain id to be 10?
  2. The public client should use a dedicated RPC, can I add an env var like OP_ALCHEMY_SECRET e.g. MAINNET_ALCHEMY_SECRET?

Copy link
Member

@sds sds left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

To answer your questions:

  1. Does publicClient.verifyTypedData care about chainId in this case? It's just validating the signature, so one would hope it need not concern itself if it's just validating a hash of those fields? (I'm personally not yet familiar enough with the internals of smart contract wallet signatures)
  2. If a new secret is indeed necessary, then sure. Otherwise would be nice if we can get away without needing a separate L1 provider.

Also going to tag @sanjayprabhu who has more context than I.

src/signature.ts Outdated

const client = createPublicClient({
chain: mainnet,
transport: http(),
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to do this as part of this change, but we really should be using a fallback provider backed by at least 2 http providers. These can be passed as secrets separated by commas in the corresponding environment variable.

With that said, we don't yet have that so fine to use a single http provider for now.

@sanjayprabhu
Copy link
Contributor

Hmm, this might be more complicated. I think we do need to update the chain id to optimism's chain id. I need to brush up on the nuances of smart contract signature verification, but since the fid lives on optimism, we should only treat the OP smart contract as valid. If contracts exist on mainnet and OP, but are configured differently, we should not trust the mainnet signature.

One way to do this is to pick a timestamp in the future and require that all signatures after that timestamp use the OP chain id, and we continue to respect the existing chain id 1 signatures before that timestamp. This requires some backend changes on Warpcast side (and any others in the ecosystem who are generating fname signatures).

@stephancill
Copy link
Contributor Author

@sanjayprabhu I agree that the signature domain should match the chain that FIDs live on and it makes sense that you should treat the signature verification result on that chain as the truth.

Is this something you're willing to change? What do you see as the way forward here?

@horsefacts
Copy link
Contributor

horsefacts commented Jul 9, 2024

Actually, I don't think changes to chain ID are necessary here.

Chain ID is included in EIP-712 signatures for cross-chain replay protection. If the same contract is deployed on multiple chains, it prevents taking a signature meant for one chain and reusing it on another. If a signature authorizes something like a token approval, you can reuse it on other chains even though it was meant for only one.

To protect against this, the domain separator includes a chain ID and the address of the contract that will verify the signature. Since this is hashed as part of the signed data, it ensures the signature changes any time either of these parameters change.

We're not in this situation. There are not multiple contracts that accept these signatures, only the fname server (an offchain signature) and the CCIP resolver contract (on L1). Strictly technically, the offchain fname server should maybe not use a chain ID and contract in its domain separator at all, but it's OK as long as you can't replay the signature. Chain ID here is just part of the hashed signed message. So chain ID 1 is proper for these messages, which are either used offchain or on mainnet.

The other factor here is where the wallet signing the message is deployed. This doesn't matter for EOAs since you can recover the address offchain, but for a smart contract wallet you need an RPC connection to whatever chain it's deployed on to verify the signature. Modern AA smart wallets use EIP-6492, which performs "counterfactual" signature validation, i.e. validating signatures from wallets that are not yet deployed. So a smart wallet that's only deployed on OP mainnet can sign a message with a mainnet chain ID, which will be valid as long as that wallet contract has the same address across chains. But other contract wallets, like a Safe, require an RPC connection to the chain where they are deployed.

So we should be using an OP client, since that's where smart contracts that own FIDs must live. Identity on L2 signing a message for L1 is a little odd but not actually dangerous IMO.

@stephancill
Copy link
Contributor Author

ty for the feedback @horsefacts, makes sense!

I have updated the client to use the optimism chain with a fallback transport (alchemy, falling back to public rpc)

@stephancill stephancill marked this pull request as ready for review July 9, 2024 21:25
@stephancill stephancill requested a review from sds July 12, 2024 09:03
@sds
Copy link
Member

sds commented Jul 16, 2024

Thanks for making those changes, @stephancill.

Team will be reviewing this week just to make sure we're 100% on board. Will let you know!

@sds
Copy link
Member

sds commented Jul 30, 2024

farcasterxyz/hub-monorepo#2181 finally landed, so we can reconsider this for review @horsefacts and @sanjayprabhu.

@sanjayprabhu sanjayprabhu merged commit a6d219a into farcasterxyz:main Aug 26, 2024
3 checks passed
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.

4 participants