I imagine the singleton registry is to facilitate the discovery of the account address so DApps have a clear way of interacting with the account. Is that right? If so, have we considered putting discovery on the NFT contract itself? I realize some of the goal is to make it compatible with all existing NFT contracts, so a registrar() interface wouldn’t be backwards compatible, but it would allow you to send someone an NFT and immediately be able to discover all the on-chain metadata required to interact with the account without succumbing to a centralized registry.
My implementation has a “PostOffice” contract which holds a registry of virtual account contract addresses mapped to individual ERC-1155 token IDs. In this way, I have my own unified account registry, that my application uses to facilitate many things under the hood for virtual accounts, and the UI. To be fully compatible, I wouldn’t necessarily want to adopt another registry, and it might require some indirection for me to properly integrate. I’ve also made the design decision to require deployment before usage, versus counterfactual addresses, so the creation workflow is a bit different. I could change this, but for simplicity I went with what I did.
In either case, I have my own registry. I imagine others will to. For me, I also have my own custom ERC-1155. This enables an account owner to create a fully managed collection of NFTs that have a parent/child relationship. In this way, any root key holder for a “Trust” as it is called can freely mint, copy, or burn any NFT in their collection, or create new ones. Because the analogy is each NFT is a “key”, they operate like physical keys would and so there could be multiple holders of a particular key/permission/account. So a virtual account could be actually owned, at a contract level, by an NFT, and anyone holding it is considered an owner, even if the trust owner has minted multiple copies of it.
Architecturally - everything else is very similar. You have an NFT contract (KeyVault.sol), a virtual account (VirtualKeyAddress.sol), and a registry (PostOffice.sol). There are other moving pieces but those likely aren’t relevant to the ERC.
My main interest in this ERC are two fold:
The mission to prove that NFT’s are more than JPEGs is gaining steam and folks are seeing the prospects of token gating increasingly complex contract functionality!
I’m actively wondering if this will solve my impending dapp integration problem. I want users to be able to easily interact with DApps, without those Apps necessarily doing a direct integration with Locksmith (as using the NFT keys as session keys allow for things like no-approval swaps. Will DApps be able to support 6551 accounts and thus transparently support projects like Locksmith if I add adapters for the interface?
While implementing ERC-6551, I was thinking of possible security issues.
Let’s say that someone creates a malicious NFT that when the user executes a safeTransferFrom with a delegated call attacks the executer. Since to transfer something from the bound account to another wallet, we should trigger executeCall in the ERC6551Account smart contract, the malicious NFT could make any kind of damage, including destroy the bound account calling a self-destruct.
Am I wrong? If not, I guess we must wait for the Duncan hard-fork before using it in production.
@sullof I think any calls coming out of the NFT contract from inside _beforeTokenTransfer or safeTransferFrom would have the NFT contract itself as the message sender.
Assuming authentication for account actions requires that the NFT is held by the message sender, the NFT contract itself might not be able to attack the tx.origin or the account itself. At least, that is the way I’m thinking about it.
To your point though, being able to use any NFT opens up a bunch of re-entrancy opportunities and undefined behavior. For instance, an ERC721 could be fully conformant on the interface, but allows the NFT contract owner to mint their own copies of the NFT or otherwise steal any virtual accounts attached to it.
From a security perspective, this is why at least for my smart wallet project I’ve enshrined a specific NFT contract that acts as the permissions for the virtual account. The same security holes plague generic ERC-20 coins and thus the token contract itself is a critical component of any on-chain account’s security model.
In this way, using an ERC-6551 account as conceived would mean that:
DYOR on which NFTs are considered secure to use for ERC-6551.
Making the wrong choice could compromise the entire account.
The threat model for the account is the entire NFT contract, the ERC-6551 implementation, and all subcomponents, combined.
@sullof In general, smart contract accounts should not execute delgatecall against untrusted contracts as that is a security risk. This is true for 6551 accounts as well as any other type of smart contract account. In the case of malicious NFTs held by a 6551 account, the account would use call instead of delegatecall to execute safeTransferFrom. There is little to no security risk in doing so, as the malicious contract would have no special permissions on the 6551 account to perform reentrancy attacks.
@scotthconner I don’t think there are reentrancy risks here, but you are correct that 6551 accounts are only as secure as the underlying NFT. If the underlying NFT contract allows the creator to arbitrarily change the owner of a given NFT, then the account bound to that NFT could be compromised. This is by design, and is a necessary trade off in order to allow every NFT to have it’s own account.
I’m excited about this new EIP!
I’m a little bit confused about a use-case:
Let’s say I want to implement a game where NFTs holders can buy accessories like hats, glasses, etc.
When an NFT holder buys an accessory, the character of the NFT should change to wear that accessory.
Do I still need to change the metadata of the NFT for that to point to the correct image?
Sorry if it’s a stupid question, but I could not figure out if this EIP could help in this use case.
Hi @lughino! This is something you can definitely build on top of ERC-6551, but it’s outside of the scope of the proposal. You would need to setup your metadata server to query the contents of the token bound account and modify the artwork of based on the contents.
This is a genuinely brilliant EIP proposal! I especially love the fact that it aims to support all existing NFTs.
Imagine a situation where an account holding 1M ETH is linked to an NFT. A malicious owner could collude with miners to sell the NFT on OpenSea. When the NFT is sold, the miner could prioritize a transaction to transfer the 1M ETH elsewhere before the atomic swap occurs.
Can EIP-5646 help here? The idea is to have a lockable token-bound account with an incremental nonce. Whenever the account is locked, the nonce will increase and update the EIP-5646s getStateFingerprint function. Marketplace/lending protocols can use the fingerprint in their offers. Of course, it would be ideal to propagate the information into the NFT of the account, which can be solved by implementing an auxiliary contract computing the fingerprint for the original NFT. Even without the auxiliary contract, this could be a semi-standard way to solve the issue of front-running withdrawals.
You can implement the ERC6551Account the way you like, which gives you a lot of flexibility. In the end, if your user’s profile changes based on what they have in their wallet, if the wallet is a standard EOA, like MetaMask, or a bound account, it changes little. The logic to handle it will be outside the wallet. Think of ERC6551 as a different way of generating wallets. What people put in the wallet, and how this content is managed is out of the scope of this EIP.
Thus said, if the profile starts with an NFT owned by the user, then, IMO, adopting ERC6551 is better than ask user to use their MetaMask
Having an ERC721-style NFT with metadata that changes is possible, and depending on how you do it could use IPFS as the storage for the metadata file, but it could also be generated on-chain. All of that is out-of-scope for this specific EIP. If you want to start a separate thread on your questions about making that sort of “dynamic NFT” in general, I’d be glad to give a more thorough answer (I’ve helped develop several of those sort of assets).
I am using the reference implementation of ERC-6551 in the Cruna Protocol, but I also played with ERC-3652 and the second produces much smaller proxies.
The ERC6551AccountProxy generates a bytecode of 964 bytes, while the ERC3652Proxy generates a bytecode of only 322 bytes, saving a significant amount of money on expensive chains like Ethereum.
It would be great if the two projects could merge and I would like to know what @jay and @k06a think of it. Since I will most likely merge the two approaches to optimize Cruna’s code, I would be happy to help if you decide to merge.
@sullof I’m not sure that these numbers are correct, since proxies deployed by the ERC-6551 registry are only 141 bytes in length (45 bytes for the ERC-1167 proxy and 96 bytes for the static data appended to the bytecode). You can see an example of the deployed proxy bytecode here.
With that said, @k06a’s contribution to this proposal would be welcomed.
@sullof ah I see! That contract is part of an example upgradable account implementation. In this case, ERC6551AccountProxy is a singleton - only one copy of the contract needs to be deployed. The proxies created by the registry will point to the ERC6551AccountProxy contract as the implementation address. This means that the cost for creating the ERC6551AccountProxy contract only needs to be paid once instead of every time an account is created via the registry.
In the case of ERC-3652, it seems like the 322 byte proxy is deployed each time an account is created. This means that ERC-6551 account proxies will cost ~1/2 as much gas to deploy.
In our implementation we allow the user to chose if using an immutable bound account or an upgradeable one that can evolve and in the future accept standard not created yet. In the second scenario, we get a conflict between the owner() function required by OwnableUpgradeable and the owner() function required by IERC6551Account, i.e., the owner of the smart contract and the owner of the account.
While I like the simplicity of using owner(), I think it would make our life easier if the name was different. accountOwner() or tokenOwner() would be good alternatives. What do you think?
BTW, I know that in your reference implementation is the owner of the NFT that can upgrade the implementation. That is a good solution if the owner of an NFT is a developer and can upgrade his own implementation without damaging the storage. However, it adds so much friction for normal users and introduces many risks at many levels. By changing the function’s name, we skip all the conflicts without forcing a solution that can reveal itself as a problem in the future.
