Secure account close

[jon-chuang]

Problem

Account close does not instantly relinquish account to system program. This means that an external party can hijack that account or permanently keep it in limbo by funding the account with enough rent - which is irreversible if not handled correctly..

Solution

Force a relinquishment of the account ownership to the system program upon account defunding and closure.

TODO

• Tests that this actually works, and that the current account close close_deprecated results in account address hijacks, including PDAs.
• Determine if using the syscall sol_memset_ would be more efficient.

On hindsight, surely sol_memset is required if the buffer is of len 1 MB. Hopefully, the rust compiler converts per byte data writing in the syscall to an OS memset under the hood.

[armaniferrante]

An alternative solution after discussing this with @jstarry is to

• Zero the account data.
• Mark the account with a tombstone "closed" account discriminator as the first 8 bytes.
• Upon account initialization, assert the discriminator is either zero or a tombstone. In either case allow initialization.

Zeroing the data is nice to have but isn't strictly necessary due to the tombstone discriminator and funding an account after it has been set with the closed discriminator shouldn't keep it in limbo with the above.

In the event of a PDA where a seed is associated with a user, a hijack should be prevented by the application logic doing proper authorization on initialization.

So in short, I think the preferred changes here is to leave the is_closed tombstone but with the following adjustments

• zeroize bytes 8-end
• change initialization logic to allow re-initialization when the tombstone in present

Also, I don't think a hijack can happen with a PDA with the current logic (though please correct me if I'm wrong). However, addresses are dead if someone funds the account's rent exemption SOL after closing in the same transaction in a subsequent instruction.

[armaniferrante]

@jstarry what do you think about resetting the owner to the system program. After thinking about this some more, it seems like this is preferred. You previously mentioned that it may be problematic since the account data can turn into a nonce. Can you elaborate?

[jon-chuang]

Another solution was to check that the close instruction is the sole instruction in the transaction and is at invocation depth 0. This ensures the account would be reclaimed by the system program and cannot be modified further prior to being reclaimed.

However, this restriction may be too inflexible.

Alternately, exposing a signer-less DefundTombstoneAccount instruction could be the way to prevent limboed tombstone accounts.

Forcing tombstones to be zeroed except for the account type marker also seems valid, if we want to allow re-initialization

[jstarry]

You previously mentioned that it may be problematic since the account data can turn into a nonce. Can you elaborate?

Any system account that has at least 72 bytes allocated can be initialized into a nonce by anyone

[jstarry]

Main concern with that is that a PDA could have non-zero data which is not set by the program. Properly written programs shouldn't have an issue with that but most devs just wouldn't expect that to be possible

[jon-chuang]

Any system account that has at least 72 bytes allocated can be initialized into a nonce by anyone

Ooh, that's not good. Then it could be permanently locked.

Ok then, I think tombstone, no reinitialization from tombstone, and having a force-defund instruction to prevent limboed tombstones seems a valid solution prior to the existence of a SystemInstruction::Deallocate instruction that sets the account data to empty vec.

The whole point of a tombstone is to prevent reinitialization. Reinitialization from tombstone is still liable to address hijacking, unless initialization requires account pubkey to be signer.

[jstarry]

Ooh, that's not good. Then it could be permanently locked.

Ah yeah, forgot to mention that part, thanks!

no reinitialization from tombstone

Should be optional, but need to opt-in to allow reinitialization

prior to the existence of a SystemInstruction::Deallocate instruction that sets the account data to empty vec

Feel free to open an issue to kick off discussion on this. I was also thinking the runtime could just allows programs to deallocate the accounts that they own.

[jackcmay]

This should help: solana-labs/solana#19475

[Arrowana]

#2169 for a light version following the token program.

[Henry-E]

We now have the canonical closing pattern in anchor thanks to realloc and @Arrowana