Proposal: Programs should only be permitted to transfer lamports from accounts they own

[dvcrn]

Problem

When signing a transaction, a program is permitted to do anything it wants with the users signature, without the user knowing upfront what he signs off to.

This is especially problematic when dealing with SOL/lamports as programs are able to transfer all the SOL a user holds at will.

For example, my solana onchain program is doing

        invoke_signed(
            &transfer(user_account.key, my_wallet.key, user_account.lamports()),
            &[user_account.clone(), my_wallet.clone()],
            &[],
        )
        .unwrap();

This is a invocation that transfers all the SOL a user owns into my wallet. When asking for a signature it looks like this:

The user has no way to know what will happen once he clicks this button, but once he does, the program will drain his entire wallet. This makes signing any transaction on Solana incredibly risky and requires usage of burner wallets in case the other side is malicious.

Further, Solana allows upgrades of onchain programs, so while a instruction was harmless in the past, the developers could have updated it since to steal SOL instead.

This problem is actively exploited in the wild with the NFT craze, where malicious NFT projects lure users with a handful of images just to then drains all user funds once approved.

Proposed Solution

Disallow programs from transferring lamports from accounts that it does not own.

Programs can still ask the user to pay for things, but the user needs to explicitly move the funds into a PDA owned by the target program first, which will show up as a separate transfer() instruction with explicit lamport amount listed in the same dialog, in any wallet such as Phantom.

  const transaction = new web3.Transaction();
  transaction.add(transferSOLToPDAInstruction); // <-----
  transaction.add(doStuff);

results in this dialog (see the explicit lamports transfer instruction)

This won't add any extra complexity since the transfer() instruction can still be in the same transaction as the actual logic, but this explicit step will tell the user clearly that he's about to sign n amount of SOL away.

[zicklag]

I'm not engaged enough with Solana yet to know for sure the implications of such a design, but I like the concept. It definitely addresses a valid concern.

One thought that might be a problem for the design was if a contract for some reason needed to make multiple transfers from the users wallet. Would it be annoying if the the user had to approve a bunch of separate transfers for some reason? Or should that just be a design consideration when developing contracts, to move any funds that they need from the user to a temporary account that it owns if there needs to be several transfers done for some reason. So that the user just has to approve the one withdraw.

[k--chow]

@dvcrn This seems really important. It reminds me of the tx.origin problem in ethereum (phishing contracts that when called, call other contracts that rely on the authority of tx.origin) https://solidity-by-example.org/hacks/phishing-with-tx-origin/. Basically the problems associated with use of unrestricted authorized signature across the board. Is there more info on this?

[KartikSoneji]

Hmm I would argue that this is an issue with wallets.
simulateTransaction can optionally return the state of accounts after a transaction, which includes the account balance.

For example:

> solana balance --lamports tNgujxWFmMkaKWmw3WuEG6mT9r58XpXcF8CKPCMq4Ns
1000000000 lamports

> curl "https://api.devnet.solana.com/" \
    -H "Content-Type: application/json" \
    --data '{
        "jsonrpc": "2.0",
        "id": "0",
        "method": "simulateTransaction",
        "params": [
            "AQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAEDDSkk4YgyuqymSfk5TNBkMEdFld5T4jG8kwDqPe1oHoAAM5ByjTQRYHm9yRG//wDb1E0uzcz3nKbhADjhAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAty46Iz8xz3HffatTKhZd453tzBCq27aWjkI9G3VXgP0BAgIAAQwCAAAAAGXNHQAAAAA=",
            {
                "encoding": "base64",
                "commitment": "confirmed",
                "replaceRecentBlockhash": true,
                "accounts": {
                    "addresses": ["tNgujxWFmMkaKWmw3WuEG6mT9r58XpXcF8CKPCMq4Ns"]
                }
            }
        ]
    }'

returns

{
    "jsonrpc": "2.0",
    "id": "0",
    "result": {
        "context": { "slot": 106103378 },
        "value": {
            "accounts": [
                {
                    "data": ["", "base64"],
                    "executable": false,
                    "lamports": 499995000,
                    "owner": "11111111111111111111111111111111",
                    "rentEpoch": 245
                }
            ],
            "err": null,
            "logs": [
                "Program 11111111111111111111111111111111 invoke [1]",
                "Program 11111111111111111111111111111111 success"
            ]
        }
    }
}

where you can see the balance.

Phantom already shows this info, other wallets should and will follow.