Associated token account owner is changeable
ADBalici opened this issue · 24 comments
Should the owner of an associated token account be changeable by a call to setAuthority in the SPL Token Program? Otherwise why have the associated token account program in the first place?
I believe it breaks the pattern of deterministically finding a token account given a mint key and a "wallet", and it forces developers to endlessly create multiple token accounts.
Can any check be added to prevent this?
👍
This would make so much sense to implement
The idea is that a wallet should never allow the authority of the ATA program be changed. Wallets that do this are broken and users should walk away from such a wallet
@mvines All of them basically allow for this. At least the major ones I know of, such as Phantom. Perhaps tickets have to be raised with them?
I don't see where Phantom allows me to transfer my ATA to another user? Can you show me the workflow there please, sorry for being dense
Sure, I would be happy to explain the problem.
I've discovered this problem in the context of listing NFTs for sale on secondary marketplaces. For example, this is a transaction which was generated after listing an item.
The token is 3jEHeB1kLYNiTAg1rtP9pLEunSjewxiksmXkrM7bFM5W (Metaplex token Frakt-2347).
My wallet (which initiated the listing) is AeHmkgcz9dGs2sXrjEMrAcDaktUJyVTKknG3CQ9jKRyn and the associated token account was 2nkqqfBHkoAinKbNEKDAi6NqGwtikpXEnQ5NF2QE1QA6.
If you look in the transaction there is a Token Program: Set Authority which sets the new authority to GUfCR9mK6azb9vcpsxgXyj7XRPAKJd4KMHTTVvtncGgp (which is a PDA).
I've done this listing through Phantom and the transaction was allowed with no errors.
Now I realise that my original phrase "All of them basically allow for this" is a bit misleading. Should have been "All of them basically allow for this, indirectly" :D
Oh I see. You approved a transaction in Phantom from a shady NTF site and they stole your ATA
@joncinque / @CriesofCarrots - we could add a token extension that prohibits changing the account owner authority that the new ATA program sets. That'd prevent this kind of ATA stealing attack going forward at least
If by shady you mean one of the biggest NFT marketplaces, then yes xD. I was actually aware of the issue beforehand. The transaction was a purposely made one. It didn't affect me per se, but I believe you are right: wallets should block this type of behaviour, as it breaks the ecosystem and other developers are forced to take this into account.
If by shady you mean one of the biggest NFT marketplaces, then yes
Size and shadiness can be independent variables :)
Yeah, seems like we can do something here to protect ATAs in the future
@bartosz-lipinski - would making ATAs non-transferable break NTFs somehow?
We could easily add this as default behavior -- if the token account's address is an ATA, forbid owner authority transfer. Are there any concerns with that? It's an antipattern to change ATA ownership, so seems like it's OK to break them
My only hesitation is I don’t know enough about how Metaplex has been cobbled together, that change might break the NTF world
we could add a token extension that prohibits changing the account owner authority that the new ATA program sets.
There was one wallet + exchange that I know of that was explicitly exploring changing ATA ownership as a way to give users direct transfer control over their own tokens, while retaining exchange control over the main wallet/deposit address. However, I don't think they ever landed on a solid/viable design.
So I'm leaning toward feeling okay about this.
If it's an extension, it'd be opt-in, yes? By whom?
If it's opt-in, and the ATA opts in, then we'll break all future offenders instead of all total offenders. Is that the middle-ground we're looking for?
Seems reasonable to me, but I don't know much about the Metaplex impl either.
If it's opt-in, and the ATA opts in, then we'll break all future offenders instead of all total offenders. Is that the middle-ground we're looking for?
Yep that's what I was thinking. This particular extension doesn't even need any data, just the tag to indicate that "bit" is set. We've clearly documented since day 1 that ATA ownership must not be transferred, so I don't feel too sad about breaking misusers: https://spl.solana.com/token#associated-token-account-ownership
I agree it is bad if ATA is owned by someone else. Some programs may transfer tokens to an ATA without checking the owner if main account is provided as a target. Does spl-token transfer CLI command checks the owner btw?
On the other hand, if ATA is being created, you can still assign the wrong owner to it, so the inability to change the owner will worsen things in such cases. Like in the case above, there is another transaction that resets authority to the proper owner.
On the other hand, if ATA is being created, you can still assign the wrong owner to it.
You can only do that with the SetAuthority instruction, though. The ATA program will only ever initialize an account with the main wallet address as owner.
This is quite a big potential attack vector: assigning the PDA of a contract ownership of an ATA (or even making it a delegate signer via the approve() function of the SPL token account). Somebody bakes that in a candy machine or a contract, and they can easily start stealing tokens (incl. stuff like USDC) from other people (especially that wallets don’t block/show this type of transactions, and most people won’t check the details of the transaction in the explorer). There are projects exploiting this already and people have no idea.
Below is a screenshot of what Phantom shows (in the details pane) when I'm approving a transaction which will result in the ATA being assigned a new authority (which can potentially steal all my tokens).
You can only do that with the SetAuthority instruction, though. The ATA program will only ever initialize an account with the main wallet address as owner.
oh, that's good, for some reason I thought one could just create ATA with system_instruction::create_account and then call spl_token::instruction::initialize_account with random owner. But if ATA is a PDA of associated token program that obviously won't work.
Are there any updates regarding this issue? It's been more than a month.
We have the token-2022 extension framework far enough along now that it'd be easily to add a new token extension which indicates that the token account's owner authority is fixed. Once this extension exists, it cannot be removed.
The ATA program would create and set this extension for all the ATA token accounts it creates. This will result in a slightly higher rent fee for all future ATA accounts. Ownership of ATA token accounts would remain transferable since we can't go back and add the flag to all existing ATA accounts