That sounds great. Thanks for explaining.
Interesting proposal and discussion. Are there any apps out there implementing this standard (sans the Draft status)?
I am implementing it in the Cruna Protocol
@jay In the IERC6551Account interface, we have
function owner() external view returns (address);
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?
1 Like
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.
1 Like
Nestability
Has thought been given on the ability to âchain togetherâ an ownership path? Namely, if Alice owns Token A, and Token A owns Token B, and Token B owns Token C, can Alice directly control the wallet holding Token C?
- If Aliceâs human-owned/-controlled address is Address Z, then with ERC6551, Token A is owned by Address Z.
- Address Y exists as a stamped-out proxy by the ERC6551 Registry as owned by Token A, and Token B is owned by Address Y.
- Similarly, Address X exists as a stamped-out proxy by the ERC6551 Registry as owned by Token B, and Token C is owned by Address X.
The issue arises that if Alice uses Address Z to try and take an action on Address X, Address X will reject it, as it would inspect who owns Token B (which is Address Y), and reject the mis-match.
To make this process nestable/chainable, the logic of the wallet contracts the ERC6551 Registry stamps out would need to have that logic. The EIP itself only enforces those wallet contracts have a specific interface (which includes an owner() function), which could do any additional logic. But I think having that logic in the provided example/reference implementation would be a large benefit, and let this EIP cover more use-cases (allow using ERC6551 instead of ERC6059, for example; the key difference that has been highlighted between those two standards).
The token-owned smart contract wallets already are required to be ERC165-compliant, so the owner() logic could easily inspect to see if the address it thinks is the owner of its key token is also a token-owned contract, it can simply return the result of the owner() function of that other contract.
Ownership Cycles
Using the knowledge that all other token-owned contracts are ERC165-compatible and therefore easily-identifiable as another token-owned contract means adding in a guard against creating trivial loops could be done by having the example token-owned contract implement ERC721-receivable (add an onERC721Received function) and using that hook to check the owner() chain to make sure its own address isnât encountered. An onERC721Received hook wouldnât catch all occurrences of cycles created, but seems like a good, lightweight way to catch a lot of accidental loop scenarios.
2 Likes
Hey there. If you want the equipping relationship on chain you should definitely check ERC-6220: Composable NFTs utilizing Equippable Parts
1 Like
These are great comments, thank you @MidnightLightning!
Thereâs a good discussion on nested execution above - essentially a wallet can receive a âproofâ of root ownership that consists of the account data for each token bound account above the one being used for execution. That chain of ownership can be verified by the account, allowing for direct execution against an account by a root owner.
This is true as long as all accounts in the ownership chain are deployed, but breaks if an account in the chain is not deployed (as you wonât be able to call in and look up the token information). Ownership Cycles also allow for interesting burn mechanics to be created, so I donât think weâll prevent them at the proposal level. Implementations are encouraged to implement mitigations against asset loss in this scenario.
1 Like
Ah, I see that now, here, thanks.
The ERC721 standard defines several âextensionsâ within the EIP itself. For many of these proposed features, youâve proposed applications implementing ERC6551 just figure out their own mitigations. Are you opposed to having ERC6551 have identified âextensionsâ that lay out best practices for these proposed additional, optional features beyond a base example implementation? Or is there any effort already creating a new EIP draft with these extensions on ERC6551?
I think some extensions in the proposal are a great idea! Trying to keep the required interface fairly uncluttered, but I think going into detail on these points in the proposal and specifying some extension interfaces could be very helpful.
Did you see my comment above about the owner() function? Your opinion?
Thanks for the reminder, I missed it. How does requiring the owner() method cause a conflict here? It should be fine fine to have two interfaces with an overlapping method, the logic of the method can be changed when the implementation does.
Letâs say that someone writes a upgradeable ERC6551Account contract using OpenZeppelin UUPS proxy contract. Most likely, they will implement
function _authorizeUpgrade(address newImplementation) internal virtual override onlyOwner {}
using OwnableUpgradeable. But the onlyOwner modifier is calling owner() to get the owner of the smart contract. Instead, it will get the owner of the wallet. Depending on how it is implemented, it may break the owner() function in IERC6551Account, which would be a disaster.
The workaround is to not use Ownable and managing the owner of the smart contract in a non-standard way, which may open to other issues.
IMO, renaming the function is the only solution to avoid problems.
If youâre using UUPS and upgrade to a new implementation with a different owner() implementation, the onlyOwner modifier will work as expected (since the old implementation of owner() that looks at token data is no longer part of the contract). This change would break compatibility with this proposal (as the account is no longer token bound), but shouldnât cause any logic issues in the contract.
You can check out my latest proposal ERC-8192, it could already do nested mapping and infinite dynamic mapping, we can have token A owning token B while token B owns token C, in which token A is owned by a wallet, token C(contract address, tokenId) â token B(contract address, tokenId) â token A(contract address, tokenId)-> wallet address. the ownerOf(tokenC) will be resolved to the correct wallet address that owns token A.
Curious as to why it was designed as an ERC instead of just a trustless immutable primitive service.
I donât think so. It breaks the compatibility with this proposal only if implementing Ownable we override IERC6551.owner() with Ownable.owner(), making the owner of the contract the owner of any account. That is exactly the problem I see here and why I suggest that the function that checks who is the owner of the account is different than owner().
Look at it from a different perspective.
I learned how to connect to the registry to create a token bound account to my Bored Ape. I choose a suggested implementation and create an account. Later I discover that my account is unable to receive ERC777, ERC1400, etc. and I look for how to upgrade the implementation. Since I am just a token user the probability that someone phishes me and let me upgrade to a malicious implementation becomes very high. If the contract instead is managed by a service that focuses on security, I can have the advantage of having an upgraded account without risks. But the second, right now, is made hard because of the function overlap.
Another case of fraud can be someone showing the source code of their ERC721Account where the final contract is extending a standard ERC721Account and implements Ownable. How many people would be able to see that even if the contract looks legit, it is a scam because it plays with the owner() function?
I see why you are resistant to this change, since you already are using this proposal in production at https://tokenbound.org/ but this is a big issue and if you do not solve it, sooner or later a new standard will be proposed to overcome it, as it always happened in the past.
1 Like
@sullof Ah I think I see what you mean. Are you saying that instead of the account implementation contract specifying the token holder as the owner, youâd like the account instance to be owned by a trusted central party that prevents the account from being upgraded to malicious implementations?
Enforcing that the owner of the account is the token holder does mean that implementations would not be able to inherit from the existing Ownable contract, which uses storage to determine the owner. This is by design, as 6551 accounts are supposed to be self-custodial, with the ownership rights belonging solely to the token holder. This means that implementations wishing to implement centralized access control as youâre mentioning should methods other than owner() to do so. One pattern (which weâve used in our implementation) is to move centralized logic into a separate Ownable contract that the account implementation calls into to check upgrade permissions. This account instances are owned by the token, and centralized access controls are clearer to the user.
Iâm not resistant to this change because of any tooling weâve launched - the tooling is designed to support the proposal and will adapt with it, not the other way around. Iâm resistant to this change because I think that token bound accounts should be solely owned by the NFT, not a centralized party. That is the essence of a token bound account. By using the owner() function to express that, existing ecosystem tooling that expects EIP-173 or EIP-5313 support can work out of the box with token bound accounts.
That said, open to modifying this if there is clear consensus that the owner() function causes more pain points than it provides benefits.
1 Like
Iâm arguing that the proposal should remain neutral on this matter. There are instances where smart contract control may be necessary. For instance, in my ERC6551 implementation within the Cruna protocol, the vault must have authority over the bound account, so it deploys a service NFT and subsequently create accounts tied to that NFT. Later, users can extract the safe-box from the vault by transferring the service NFT to themselves, with the option to re-inject it later. This flexibility is crucial to facilitate migrations between two versions of the vault.
That said, weâve received inquiries about the possibility of making the bound account upgradeable without potential risks. In response, although our entire contract suite is intentionally non-upgradeable to maintain trustless design, weâve decided to let users choose between an immutable account and an upgradeable one. In our upgradeable implementation (https://github.com/cruna-cc/cruna-protocol/blob/main/contracts/bound-account/ERC6551AccountUpgradeable.sol#L38), we avoid using Ownable due to the aforementioned conflict, and instead establish a _deployer variable in the constructor. As such, Iâve personally resolved the issue.
However, I recognize the potential for misuse, especially concerning the function overlap. Thereâs a real risk of individuals exploiting this to defraud others, so I believe this matter warrants resolution.
That implies that you saw the issue and solved in your own case. I think that renaming that function would solve all the issues and make devâs life easier and userâs life safer.