Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Bad actors can abuse the privilege extension feature for Cross-Program Invocations via system_instruction::transfer, spl_token::instruction::approve, spl_token::instruction::transfer #17762

Closed
max-block opened this issue Jun 5, 2021 · 5 comments
Labels
stale [bot only] Added to stale content; results in auto-close after a week.

Comments

@max-block
Copy link
Contributor

Problem

A bad actor can create on-chain Solana programs which can steal SOL and SPL tokens. Solana wallets don't inform users about it.

Here is a simple program for example:

/// Accounts expected:
/// 0. `[signer, writable]` Debit lamports from this account
/// 1. `[writable]` Credit lamports to this account
/// 2. `[]` System program
pub fn process_instruction(
    _program_id: &Pubkey,
    accounts: &[AccountInfo],
    input: &[u8],
) -> ProgramResult {
    let acc_iter = &mut accounts.iter();
    let alice_info = next_account_info(acc_iter)?;
    let bob_info = next_account_info(acc_iter)?;
    // the third account is SystemProgram. Don't forget it in a client

    let amount = input
        .get(..8)
        .and_then(|slice| slice.try_into().ok())
        .map(u64::from_le_bytes)
        .ok_or(ProgramError::InvalidInstructionData)?;

    invoke(
        &system_instruction::transfer(alice_info.key, bob_info.key, amount),
        &[alice_info.clone(), bob_info.clone()],
    )?;
    msg!(
        "transfer {} lamports from {:?} to {:?}: done",
        amount,
        alice_info.key,
        bob_info.key
    );
    Ok(())
}

A bad actor can create such a program and to lie users about its behavior. For example, he can say that it a simple game, just for fun.

The problem that Solana wallets don't inform users at all that their SOL or SPL tokens can be transferred.
I've made such a program and here are screenshots of Phantom and Sollet wallets, how they ask users to approve this dangerous tx:

Phantom
phantom
Phantom wallet doesn't give any chance to a user to realize that his SOL can be stolen.

Sollet
sollet
Sollet wallet noticed that 11111111111111111111111111111111 account is used, but nothing about that lamports can be transferred.

Here is a repo with this demo: https://github.com/max-block/spl-examples/tree/v0.1.0/02__transfer-lamports
Here is a tx when SOL were transferred via this program: https://explorer.solana.com/tx/4kXivZpzNMD3ZZgFjRrfnDZ8KH1RGC8gCyZoEw2SPzqTGYyoFVmHgzo5s2fTs9ASxp6YeDsKxBK8Bn95TPr6LqSt?cluster=testnet

https://docs.solana.com/developing/programming-model/calling-between-programs#instructions-that-require-privileges
Docs says: This privilege extension relies on the fact that programs are immutable.

But it's not true:

  • Most on-chain Solana programs are mutable
  • Most on-chain Solana programs don't have a public github repo with open sources

I think it's a dangerous situation when it's so easy to confuse users and steal all their SOL and SPL tokens.

Proposed Solution

Maybe Solana wallets should analyze instruction's accounts and show a warning message to users if there are such addresses as 11111111111111111111111111111111, TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA? A warning message like this: "Warning! All your SOL tokens can be transferred! Do you trust this program?"

Just now there is no way for Solana wallets to inform users how much SOL or SPL tokens can be transferred. Maybe it's possible to modify AccountInfo struct and to add some additional parameters.

Here is an example how additional parameters can looks like:

const ix = new TransactionInstruction({
    keys: [
      { pubkey: userPubkey, isSigner: true, isWritable: true, max_transfer: 5000 },
      { pubkey: bobPubkey, isSigner: false, isWritable: true },
      { pubkey: SystemProgram.programId, isSigner: false, isWritable: false },
    ],
    programId: programId,
    data: data,
  })

This additional parameter max_transfer: 5000 gives possibility to Sollet wallets to inform users like this: "This transaction can transfer up to 5000 lamports from XXXX account".

If it's impossible to change AccountInfo struct, maybe it's possible to add one more parameter to entrypoint function:pub fn process_instruction(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8], meta: Meta); This meta parameter can have info about max lamports or max SPL tokens which can be transferred or approved for specified accounts.

So the idea is to add some additional data for instruction about limits for SOL and SPL token transfer and approve operations. Which can be used on both sides: solana_program and on clients.

This additional data about transfer/approve limits can be optional. If a programmer use it, a Solana wallet will inform user something like this: "Transfer up to 0.5 SOL from ABCDDDDDDD addess". But if a programmer don't use this additional data, a Solana wallet should show a warning: "Attention! All you SOL can be transferred. Do you trust this program?".

@jon-chuang
Copy link
Contributor

jon-chuang commented Jun 6, 2021

Thanks for the detailed writeup.

I'm not sure this problem has a general solution. For instance, even if you solve it for lamports, you haven't solved it for SPL tokens. Even if you've solved it for SPL tokens, you haven't solved it for NFTs and other more subtle transactions/interactions.

Ultimately, you're trusting the program at a particular address. More so if that program is updateable - which is the case for many programs still in development - as you've pointed out.

But I definitely agree that identifying SOL and SPL transfers would go a long way to help with "arbitrary transaction" readability.

You've suggested enforcing a limit. I think this makes sense. Changes to tx data and to the SPL token program itself to enforce that txs obey such limits might be pretty hard, however. That's because right now, programs are "turing complete" with respect to SOL transfers, while SPL token transfers are governed by an immutable program - thus changes would require migration.

Solutions

  1. I think a workaround could be to split up your main account from "dummy accounts" which inherently have limited SOL/SPL tokens, especially when dealing with untrusted code.

  2. Wallets can also add a "trusted programs" opt-in whitelist (with some default suggestions). When invoking a program that is not trusted, the wallet can warn that the instruction involves accounts with e.g. "1000 SOL, 50,000 USDC". "Are you sure you'd like to proceed with this untrusted program?" etc.

  3. Yet another thing that can be done would be if wallets could allow for "simulating" transactions - there already is an RPC call for this. Then, based off the result of the simulation, one can tell if SOL/SPL tokens would be transferred. I think this is the nicest solution presently, from the perspective of UX.
    Of course, an attacker can make the transfer actually happen only 1/10 times based on some variable that is updated constantly. So the transfer won't show up in simulation, but would happen 1/10 times if run for real. Unfortunately, simulation can't combat this. But it would make it harder for an attacker.

  4. I was also thinking of the possibility of making a transaction valid iff the state it references has not changed since the simulation. Unfortunately, this is hard as account data is not hashed every time a transaction mutating it is performed. More details here: [Security] Optional transaction metadata - tx valid iff account data hashes have not changed - allowing deterministic tx simulation #17766
    But this seems very feasible, and the cost of hashing data is not particularly high.

@ryoqun
Copy link
Contributor

ryoqun commented Jun 7, 2021

3\. Yet another thing that can be done would be if wallets could allow for "simulating" transactions - there already is an RPC call for this. Then, based off the result of the simulation, one can tell if SOL/SPL tokens would be transferred. I think this is the nicest solution presently, from the perspective of UX.
    Of course, an attacker can make the transfer actually happen only 1/10 times based on some variable that is updated constantly. So the transfer won't show up in simulation, but would happen 1/10 times if run for real. Unfortunately, simulation can't combat this. But it would make it harder for an attacker.

@jon-chuang yeah, your idea is good and very similar to mine (https://github.com/solana-labs/solana/pull/17796/files#r646735912, still not written properly...). actually you can add assertion instruction at end of tx passing expected SOL/spl-token balance changes collected from the simulation. So, the tx will just fail if the program has lied. ;)

@max-block
Copy link
Contributor Author

There is a relatively simple solution how to protect users from abusing the privilege extension feature.

  • Solana wallets should analyze all instructions for dangerous Accounts: 11111111111111111111111111111111, TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA
  • There will be a list of trusted programs: all programs from solana-labs, Serum, etc. These programs will be able to use these dangerous accounts inside their code.
  • For all other programs (which are developed by unknown programmers) must be a warning message to users: "Your SOL / SPL tokens can be stolen. Do you trust this program?"

So, this feature of Sollet wallets will force programmers of custom smart contracts not to transfer/approve tokens inside their smart contracts. In most cases, it's possible to solve the task using a separate instruction.

@Standaa
Copy link

Standaa commented Jun 18, 2021

This should imo be closed. An attacker can bypass/game any transaction security flow if the person at the other end of the spectrum trusts compromised code.

There will be a list of trusted programs: all programs from solana-labs, Serum, etc. These programs will be able to use these dangerous accounts inside their code.

E.g Anchor framework has fairly strict policies when it comes to passing account in CPIs and encourages the use of associated accounts. And good/known programs will prevent anyone from passing in random accounts.
Either way, this is a program side problem and has not much to do with the solana sdk.

For all other programs (which are developed by unknown programmers) must be a warning message to users: "Your SOL / SPL tokens can be stolen. Do you trust this program?"

Who whitelists ? A reputable org ? Seems pretty centralised.

Solana wallets should analyze all instructions for dangerous Accounts

Feel free to fork / build something better than say Phantom.

Is the rationale here to say that a bad program can do bad things ? Or that bugs can provoke loss of funds ?

@ryoqun
Copy link
Contributor

ryoqun commented Sep 7, 2021

ref #17796

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 20, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

No branches or pull requests

4 participants