limitbreakinc / creator-token-contracts

MIT License
108 stars 26 forks source link

`EOARegistry` breaks DeFi composability #1

Closed aviggiano closed 1 year ago

aviggiano commented 1 year ago

Limit Break is introducing opt-in, backwards-compatible, programmable royalties contracts that work on any ERC-721 contract through a novel staking solution.

One of the contracts of this repository, the EOARegistry, is a deployable contract where users can sign a message to prove they are an EOA. A global community-use EOA registry will be deployed and made available as there is no real need for users to prove they are an EOA in more than one contract.

The problem with this approach is that allowing only EOAs to interact with smart contracts breaks DeFi composability. More specifically, it will not be possible for multisig wallets (such as Gnosis Safe) or Smart Contract wallets (such as Argent) to interact with contracts using the EOARegistry.

Furthermore, now that EIP-4337: Account Abstraction Using Alt Mempool is on its way out, we should see more and more applications making use of AA in order to onboard the next hundred million users to crypto. For those who don't know about AA, here's a summary of why everybody wants it.

The core motivation for account abstraction stems from the goal of reducing from two account types to only one account type i.e., contract type. A single account type for all the Ethereum accounts will enable developers to provide a seamless experience to users on a single account with the functionality to transact tokens as well as create new contracts. It will also make it easier for developers to create better protocols and services without the need to make distinctions between account types since transacting will be moved fully into the EVM and off of the blockchain protocol.

Another motivation for account abstraction is to offer a rich user experience such as multi-signature security, social recovery, rate-limiting, creating allow/deny-list of addresses, and gasless meta-transactions. Currently, only contract accounts (smart contract wallets) are capable of offering these features, however, the UX offered by these accounts is hindered by relying on fluctuating gas prices. Additionally, since contract accounts cannot pay gas, the user needs an EOA to pay gas fees or leverage a relayer (usually centralized entities).

I understand that Limit Break, being a thought leader in crypto, has a very important part in shaping the future of this industry. This is why I encourage you to reconsider whether EOARegistry is aligned with the rest of the Ethereum community's goals and development roadmap.

yoavw commented 1 year ago

I second that. EOARegistry goes against the Account Abstraction trend, and will prevent users from enjoying the benefits it brings.

Furthermore, we are working to move away from EOA. At some point EOAs may become deprecated, and all new accounts will have code. EOARegistry will give false sense of security when it happens. The user will be able to sign with the ECDSA key associated with the address, but also have a contract in the same address.

tempest-sol commented 1 year ago

Limit Break is introducing opt-in, backwards-compatible, programmable royalties contracts that work on any ERC-721 contract through a novel staking solution.

One of the contracts of this repository, the EOARegistry, is a deployable contract where users can sign a message to prove they are an EOA. A global community-use EOA registry will be deployed and made available as there is no real need for users to prove they are an EOA in more than one contract.

The problem with this approach is that allowing only EOAs to interact with smart contracts breaks DeFi composability. More specifically, it will not be possible for multisig wallets (such as Gnosis Safe) or Smart Contract wallets (such as Argent) to interact with contracts using the EOARegistry.

Furthermore, now that EIP-4337: Account Abstraction Using Alt Mempool is on its way out, we should see more and more applications making use of AA in order to onboard the next hundred million users to crypto. For those who don't know about AA, here's a summary of why everybody wants it.

The core motivation for account abstraction stems from the goal of reducing from two account types to only one account type i.e., contract type. A single account type for all the Ethereum accounts will enable developers to provide a seamless experience to users on a single account with the functionality to transact tokens as well as create new contracts. It will also make it easier for developers to create better protocols and services without the need to make distinctions between account types since transacting will be moved fully into the EVM and off of the blockchain protocol.

Another motivation for account abstraction is to offer a rich user experience such as multi-signature security, social recovery, rate-limiting, creating allow/deny-list of addresses, and gasless meta-transactions. Currently, only contract accounts (smart contract wallets) are capable of offering these features, however, the UX offered by these accounts is hindered by relying on fluctuating gas prices. Additionally, since contract accounts cannot pay gas, the user needs an EOA to pay gas fees or leverage a relayer (usually centralized entities).

I understand that Limit Break, being a thought leader in crypto, has a very important part in shaping the future of this industry. This is why I encourage you to reconsider whether EOARegistry is aligned with the rest of the Ethereum community's goals and development roadmap.

These are great and valid points, however, being that the recommended approach and future compatibility is still an EIP. There is no current solution, other than what they are currently offering. Once that EIP becomes a production standard, if it does, then proposing an upgrade/change will be more likely.

I do not see any alternative to what was done, aside from speculation, do you have any alternative recommendations at hand?

Also, more importantly, there is no revoke function for the EOARegistry, therefore users who adopt and authorize signatures, can have wrapped assets moved at will, without the ability to revoke.

niftynathang commented 1 year ago

Thank you for the feeback!

First, I want to point out that this is simply a tool, and it can be used by a developer or not based on their needs. Second, you are correct that using an EOA Registry to prevent a specific address from interacting with a contract breaks Defi composability. Rest assured, this is intentional and by design!

There are valid use cases where smart contract composability is necessary and desired. The use of the EOA Registry is not appropriate for those use cases. Developers are always responsible to think about what tools they are using and decide if they are using the correct tools for their application.

However, there are also valid use cases where composability is undesirable and the intention of the EOA Registry is to help solve for those cases.

Please also notice that by default the EOA Registry is not used. IF the creator/developer does not want to allow smart contracts to stake tokens that is their prerogative and they can set the flag that disables smart contract stakers. In that case, the _msgSender() != tx.origin check is used instead of the EOA Registry, unless the developer explicitly points at an EOA registry. There were two motivations for including the EOA Registry as a fallback option:

  1. For years, the removal of tx.origin has been discussed so it may not be a viable way to check whether the caller is a smart contract in the future.
  2. If you want to check that multiple parties participating in a transaction are all EOAs, the EOA Registry provides a gas efficient lookup for arbitrary addresses, not jus the address that initiated the transaction.

At the current time, the referenced EIP-4337 is likely a long way from becoming reality. This seems like a bridge that can be crossed at the appropriate time once we see what that implementation looks like. As for gasless meta-transactions, note the use of _msgSender() instead of msg.sender. Using OpenZeppelin's Context contract, it is possible to know that the original msg.sender was an EOA, not the gas relay node contract so there is no reason to assume that gasless meta-transactions are incompatible with the EOA registry.

To reiterate, we are simply providing a tool. We are not prescribing that everybody use an EOA registry for all applications. Every developer should assess whether the tool works for their application or not.

On this point: "Also, more importantly, there is no revoke function for the EOARegistry, therefore users who adopt and authorize signatures, can have wrapped assets moved at will, without the ability to revoke." The EOA registry does not create permissions to move assets at all. There is no need to revoke proof that an address was an EOA. It is simply a one-time record proving that a user signed the "EOA" string. It grants no permissions whatsoever to move anything.

niftynathang commented 1 year ago

By the way, I want to thank you for taking the time to read and understand the contracts! We have clearly stated that we would love for people to use this either as is, or to use the parts of this library that they like and agree with. Even if you like just one pattern we have created here and adapt the pattern in your own contracts with your own preferences we consider it a win for the community in terms of driving innovation, which is what this is all about.

tempest-sol commented 1 year ago

By the way, I want to thank you for taking the time to read and understand the contracts! We have clearly stated that we would love for people to use this either as is, or to use the parts of this library that they like and agree with. Even if you like just one pattern we have created here and adapt the pattern in your own contracts with your own preferences we consider it a win for the community in terms of driving innovation, which is what this is all about.

Love the contrast. I'd like to ask if your team would be keen to open up contributions for future expansions. Not only this repository. I am looking to contribute to security and security abstracts in this space more, and I think this is a hell of a first step for opportunities, this whole thing has my mind on overload with ideas.

aviggiano commented 1 year ago

I do not see any alternative to what was done, aside from speculation, do you have any alternative recommendations at hand?

The recommendation is to not rely on the msg.sender being an EOA for smart contract logic.

However, there are also valid use cases where composability is undesirable and the intention of the EOA Registry is to help solve for those cases.

I am not sure what exactly use cases allow only EOAs to interact with a protocol (as legitimate users may prefer to have smart contract wallets). The only one I can think of is prohibiting custom logic from being performed when interacting with a smart contract. However, as yoavw pointed out

At some point all EOAs will be able to run code. It is undecided how exactly (conversion of all EOAs to a default proxy, adding code to the EOA through a special transaction, EIP 3074+5003 conversion...) but either way it'll happen eventually. When it does, users will be able to sign a message with the associated ECDSA key to enter the EOARegistry, and still have a contract in that address.

Are there any other use cases where EOARegistry might be useful?

Even though this contract is not used by default here, third-party applications relying on it might be unaware of the drawbacks it brings, especially in the medium-long term. This is the reason why it should be discouraged in my point of view.

tempest-sol commented 1 year ago

The recommendation is to not rely on the msg.sender being an EOA for smart contract logic.

However, there are also valid use cases where composability is undesirable and the intention of the EOA Registry is to help solve for those cases.

I am not sure what exactly use cases allow only EOAs to interact with a protocol (as legitimate users may prefer to have smart contract wallets). The only one I can think of is prohibiting custom logic from being performed when interacting with a smart contract. However, as yoavw pointed out

The only way to currently prove EOA is by msg.sender, practice is to stray from use of tx.origin.

Also, the use case for this would be proxy or upgradeable contracts, as mentioned above, they could still use a contract wallet with the use of msg.sender but there would need to be an alternative to signatures. Which, there technically isn’t one, however we could introduce the ability to publish a contract that produces / stores the signature on behalf of the owning address if that were the case for a temporary solution.

yoavw commented 1 year ago

they could still use a contract wallet with the use of msg.sender but there would need to be an alternative to signatures. Which, there technically isn’t one, however we could introduce the ability to publish a contract that produces / stores the signature on behalf of the owning address if that were the case for a temporary solution.

There is a standard solution: EIP-1271. All contract wallets should support it, and any dapp that wishes to be compatible with account abstraction should implement a signature check that checks isValidSignature() of the account has code.

It's important to check isValidSignature() and not attempt to ecrecover() the signature if the account has code, since an EOA may get code in the future, and there is no other way to invalidate its original ECDSA key for off-chain checks. If a user rotated the key after it was compromised, then any dapp that attempts to ecrecover will ignore the key rotation and accept the compromised key as valid.

niftynathang commented 1 year ago

A new version is available. CreatorTokenERC721 no longer has the option to disable smart contract stakers, and does not use EOA Registry anymore. However, we have added an EOAOnlyCreatorERC721 that does disable smart contracts stakers using either a msg.sender != tx.origin or EOA Registry check. We have updated the documentation to be explicit about the downsides and to caution developers to carefully consider whether or not they want to exercise that option.

niftynathang commented 1 year ago

I'm closing this issue as we have refactored to make the optionality more explicit and more clearly documented.