lang: Fix unexpected account substitution in InterfaceAccount

[acheroncrypto]

Problem

InterfaceAccount allows account substitution between unexpected types.

PoC

Passing AnotherAccount to an account typed as InterfaceAccount<ExpectedAccount>:

• Program
• Test

Details

The PR titled "feat(account): Check Owner on Reload" #3837 changed InterfaceAccount::try_from from:

https://github.com/solana-foundation/anchor/blob/2da41204735c944686a39bd18a50a0ff173425bb/lang/src/accounts/account.rs#L303-L313

to:

https://github.com/solana-foundation/anchor/blob/3a799e2d32bd708f0b280c60baecac76418248b1/lang/src/accounts/interface_account.rs#L243-L252

The author assumes InterfaceAccount is only intended to work with programs that "do not have Anchor discriminators". However, similar to Account, the InterfaceAccount implementation does not actually make any assumptions about discriminators. As its name suggests, it's for accounts that share the same interface between different programs; or in other words, it's the same as the Account type, but instead of checking only a single owner via the Owner trait, it allows multiple owners via the Owners trait.

Similar to the Account type, InterfaceAccount's documentation even has a section called Using InterfaceAccount with non-anchor programs that starts with:

https://github.com/solana-foundation/anchor/blob/3a799e2d32bd708f0b280c60baecac76418248b1/lang/src/accounts/interface_account.rs#L85

which in itself should be enough to suggest that InterfaceAccount can also be used with Anchor program accounts, or generally, accounts that have discriminators.

The same erroneous understanding also resulted in changing the reload method of AccountInterface from:

https://github.com/solana-foundation/anchor/blob/2da41204735c944686a39bd18a50a0ff173425bb/lang/src/accounts/interface_account.rs#L188

which used Account::reload with checked account deserialization (AccountDeserialize::try_deserialize):

https://github.com/solana-foundation/anchor/blob/2da41204735c944686a39bd18a50a0ff173425bb/lang/src/accounts/account.rs#L271

to an implementation that uses unchecked account deserialization (AccountDeserialize::try_deserialize_unchecked):

https://github.com/solana-foundation/anchor/blob/3a799e2d32bd708f0b280c60baecac76418248b1/lang/src/accounts/interface_account.rs#L206

The misconception might have arisen from the fact that InterfaceAccount was initially added (in #2386) in order to make handling SPL Token and SPL Token 2022 accounts easier. However, its implementation is fully generic, just like the Account type. In fact, in its implementation PR, the first commit is titled 'Add "interface" and "interface account" concept', and it does not even touch anchor-spl.

The reason why anchor-spl types such as token::Mint and token_interface::Mint only implement try_deserialize_unchecked is because try_deserialize defaults to try_deserialize_unchecked (in this case they are all checked in reality):

https://github.com/solana-foundation/anchor/blob/3a799e2d32bd708f0b280c60baecac76418248b1/lang/src/lib.rs#L361-L370

This means changing try_deserialize to try_deserialize_unchecked here in the best case has no benefits (same impl), and in the worst case allows bypassing account checks.

Summary of changes

• Add tests that test the basic security checks of InterfaceAccount, including unexpected account substitution
• Fix InterfaceAccount::try_from (revert to how it was before)
• Fix InterfaceAccount::reload (use checked account deserialization)
• Allow multiple owners in InterfaceAccount::reload (see lang: Fix unexpected account substitution in InterfaceAccount #4139 (comment))

[vercel]

@acheroncrypto is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[acheroncrypto]

Before the fix commit (CI):

✔ Allows old exptected accounts (48ms)
✔ Allows new exptected accounts
✔ Doesn't allow accounts owned by other programs
1) Doesn't allow unexpected accounts owned by the expected programs

After (CI):

✔ Allows old exptected accounts (49ms)
✔ Allows new exptected accounts
✔ Doesn't allow accounts owned by other programs
✔ Doesn't allow unexpected accounts owned by the expected programs

[acheroncrypto]

Another related problem: the owner check in InterfaceAccount::reload seems too restrictive:

https://github.com/solana-foundation/anchor/blob/3a799e2d32bd708f0b280c60baecac76418248b1/lang/src/accounts/interface_account.rs#L198-L202

I think this behavior is inconsistent with InterfaceAccount because the initial account validation via InterfaceAccount::try_from allows multiple owners, but reloading doesn't allow multiple owners. To fix, we can replace the owner check above with:

https://github.com/solana-foundation/anchor/blob/2da41204735c944686a39bd18a50a0ff173425bb/lang/src/accounts/interface_account.rs#L223

Perhaps we could add another method (something like reload_strict) that enforces the owner to be the same as the one validated at the start of the ix, but I think it's best to keep things simple and not add unnecessary code.