search

farcasterxyz / hub-monorepo

Implementation of the Farcaster Hub specification and supporting libraries for building applications on Farcaster
https://www.thehubble.xyz
MIT License
700 stars 392 forks source link

feat: support Smart Contract Accounts #1035

Closed deodad closed 1 year ago

deodad commented 1 year ago

Users primarily use Externally Owned Accounts to interact with the Ethereum, but as more wallets implement Account Abstraction we will see an increasing number of users with Smart Contract Accounts. To support both accounts types hubs will need to verify signatures via ERC-6492.

There are multiple places where a smart contract account could be a signer:

Verifying signatures from smart contract accounts will require an eth_call; hubs will need to implement limiting to prevent malicious users from triggering an arbitrary number of eth_call.

varunsrin commented 1 year ago

@deodad can you add more details on what the issue is?

Hmac512 commented 1 year ago

There are multiple places where a smart contract account could be a signer:

  • custody address signing SignerAdd and SignerRemove messages or a Farcaster name claim
  • subject of an VerificationAddEthAddressBody message

Seems like it starts with supporting ERC1271 support in the idRegistry contract (see mentioned PR).

Refactoring the hub-monorepo code to support ERC1271 signatures will be a huge pain.

Verifying signatures from smart contract accounts will require an eth_call; hubs will need to implement limiting to prevent malicious users from triggering an arbitrary number of eth_call.

UniversalSigValidator.isValidSig(_signer, _hash, _signature): returns a bool of whether the signature is valid or not; this is reentrancy-safe

I am pretty sure what you want to do is implement an external view on the idRegistry that is re-entrency safe.

Hmac512 commented 1 year ago

Smart contract wallet support along with meta-tx has a lot of issues.

I can deploy a smart contract wallet where the signature verification method takes a lot of gas.

If you call the signature verification method from a meta-tx, then you can force the tx sender to pay a larger fee than is necessary.

deodad commented 1 year ago

@Hmac512 good points thanks for the callout, my take aways:

Hmac512 commented 1 year ago

This has an attack vector where a user can trigger an arbitrary number of eth_calls by continually resubmitting these messages.

This is definitely non-trivial to handle in the hub.

This is why I made an early PR. The system design of the hub is not setup to handle making external calls to verify signatures.

Tbh, this is why I think Ethereum addresses are inadequate for identity going forward.

My initial thoughts:

If you want to support SMWs, then I think you’d want to attach a separate delegate signing address to each fid.

If a delegate address is not set for a fid, the owner of the fid (the one used now) will be used.

Signatures from the delegate address are verified via ECDSA.

The idea is to let a SMW update the delegate address arbitrarily.

The problem here is you’d have to index the delegate signing address if you want to sync a hub from scratch.

You can mitigate this by charging a fee to update that address to prevent people from going crazy.

Mot in love with this approach.

You can avoid the indexing of previous delegate signer addresses by using a sparse merkle tree to store the state. Then you can add a merkle proof of the most recent delegate address for a fid with every message.

… But it’s a lot of changes to be done by an outside party.

Hmac512 commented 1 year ago

I think it’s worth considering the diamond pattern for implementing these changes to keep the contracts clean, and leave open the possibility for optimized implementations in the future.

deodad commented 1 year ago

Thanks for the context and proposal, I'll bring this back to the team.

Hmac512 commented 1 year ago

https://github.com/Agusx1211/EIPs/blob/6f2da19085dc44407bfe21d6a7589299e490fdf6/EIPS/eip-6492.md?plain=1#L238-L250

I am not sure EIP-6492 is going to help for the farcaster use case. Although it is very clever.

The off-chain verification of a signature only works if the smart contract wallet has not been deployed yet, and the SCW is based on CREATE2 so the address can be predicted from the contract bytecode.

This spec was not made for verifying a lot of sigs at scale.

https://github.com/Hmac512/erc6492/blob/546e0283c8aa1329982302c70b4dea8d527c76d0/SIgValidator.sol#L9-L15

It's a galaxy brain trick I can't believe someone thought of. You return a bool from the constructor, so you can verify a signature from just the contract code (prev link).

On-chain verification of a SCW signature that hasn’t been deployed will be too expensive to be feasible at scale.

THIS ONLY WORKS IF THE CONTRACT HAS NOT BEEN DEPLOYED YET. If it has been deployed, then you fallback to having to verify the signature against the live contract.

Key rotation is a big part of account abstraction.

varunsrin commented 1 year ago

I'm not sure I follow the concerns:

  1. In the current world, Hubs can validate 1271 compatible smart contract wallets entirely off-chain using isValidSignature which is a view function. 6492 is a little tricker, but isn't that relevant because of (2)

  2. With the proposed on-chain signer refactor, Hubs don't do any validation anymore, since its done on-chain by the KeyRegistry contract

Hmac512 commented 1 year ago
  1. In the current world, Hubs can validate 1271 compatible smart contract wallets entirely off-chain using isValidSignature which is a view function. 6492 is a little tricker, but isn't that relevant because of (2)

I guess my concern here isn't as pertinent considering the signer refactor you mentioned. I'll explain to close the loop.

EOA wallet signatures are intrinsically verifiable. Meaning you can validate them locally, and they are non-revocable.

SCW signatures, in the general case, have to be verified through an RPC call through Infura/Alchemy/etc. They are also revocable.

Verifiying signatures

There is not way to tell if a wallet is a EOA or SCW from the address itself. So to support SCWs, you will have to verify all signatures through eth_call or add some caching.

Also, a SCW can be setup so that the signature verification takes arbitrarily long. Which provides an attack surface to halt hub syncing if not accounted for.

It might be worth running a test to see how much routing signature verifications through eth_call slows the hub syncing down. Although, the proposed signer refactor may render this a moot point.

Revocable Signatues

A SCW signature can be valid at block N, and become invalid at block N+1. This must be accounted for in the hub, as someone syncing a hub from scratch can run into problems.

  1. With the proposed on-chain signer refactor, Hubs don't do any validation anymore, since its done on-chain by the KeyRegistry contract

Ah, I didn't see this proposal. It is in the same direction as my delegated signer comment from above. I'll take a look through the thread.

I think BLS signatures are a better choice than eddsa, but I can talk about it in the proposal's thread.

horsefacts commented 1 year ago

Added support for ERC-1271 sigs in https://github.com/farcasterxyz/contracts/pull/286. Although hubs may not make use of ERC-1271 signatures, we do want smart wallet accounts to be able to register/transfer fids and add/remove signers with 1271 sigs.

varunsrin commented 1 year ago

Custody address support is now complete in v3.

There's an open FIP for adding support to verifications: https://github.com/farcasterxyz/protocol/discussions/109

Closing this issue for now