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: support Smart Contract Accounts #1035

Closed
deodad opened this issue Jun 23, 2023 · 12 comments
Closed

feat: support Smart Contract Accounts #1035

deodad opened this issue Jun 23, 2023 · 12 comments
Labels
help wanted Well specified and ready to be worked on s-ready Ready to be picked up t-feat Add a new feature or protocol improvement
Milestone

Comments

@deodad
Copy link
Member

deodad commented Jun 23, 2023

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:

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

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.

@github-actions github-actions bot added the s-triage Needs to be reviewed, designed and prioritized label Jun 23, 2023
@varunsrin
Copy link
Member

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

@deodad deodad changed the title feat: support verifying smart contract account addresses feat: support Smart Contract Accounts Jul 5, 2023
@varunsrin varunsrin added help wanted Well specified and ready to be worked on t-feat Add a new feature or protocol improvement s-ready Ready to be picked up and removed s-triage Needs to be reviewed, designed and prioritized labels Jul 7, 2023
@varunsrin varunsrin added this to the vNext milestone Jul 7, 2023
@Hmac512
Copy link
Contributor

Hmac512 commented Jul 14, 2023

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
Copy link
Contributor

Hmac512 commented Jul 14, 2023

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
Copy link
Member Author

deodad commented Jul 14, 2023

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

  • supporting smart contract wallets as custody addresses will be non-trivial, with changes needed to the contracts and hubs. There is also an attack vector if we want to support meta transactions, as smart contract wallets signature verification can take an arbitrary amount of gas.
  • supporting smart contract accounts in VerificationAddEthAddressBody would be easier: the only change would be to hubs where an off-chain verification of the signature is performed. This has an attack vector where a user can trigger an arbitrary number of eth_calls by continually resubmitting these messages.

@Hmac512
Copy link
Contributor

Hmac512 commented Jul 14, 2023

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
Copy link
Contributor

Hmac512 commented Jul 14, 2023

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
Copy link
Member Author

deodad commented Jul 14, 2023

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

@Hmac512
Copy link
Contributor

Hmac512 commented Jul 15, 2023

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
Copy link
Member

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
Copy link
Contributor

Hmac512 commented Jul 17, 2023

  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
Copy link
Contributor

horsefacts commented Jul 27, 2023

Added support for ERC-1271 sigs in farcasterxyz/contracts#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
Copy link
Member

Custody address support is now complete in v3.

There's an open FIP for adding support to verifications: farcasterxyz/protocol#109

Closing this issue for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Well specified and ready to be worked on s-ready Ready to be picked up t-feat Add a new feature or protocol improvement
Projects
Archived in project
Development

No branches or pull requests

4 participants