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

added a flag to check if the recipient address is correct #3085

Merged
merged 11 commits into from
Apr 21, 2022

Conversation

Saviour1001
Copy link
Contributor

Fix for #3078 .

  • Added a checker to check if the recipient address of a token transfer is owned by System Program
  • The checker can be avoided by using --yolo

@mergify mergify bot added the community Community contribution label Apr 14, 2022
@Saviour1001
Copy link
Contributor Author

@joncinque can you check this out !

Comment on lines 653 to 660
let recipient_account = config.rpc_client.get_account(&recipient).unwrap();
if recipient_account.owner != system_program::id() && !yolo {
return Err(
("Recipient is not a Account owned by System Program. Use --yolo if you know what you doing").to_string()
.into(),
);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be moved into the block at ln705 where recipient_is_token_account is resolved taking into account:

  1. No RPC calls are allowed in sign-only mode. Probably fine because the account is already queried in that block, so you can just reuse it.
  2. The recipient may have been specified as a token account directly. In which case, it is owned by the SPL Token program and this check should not be performed at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got the changes done!

@Saviour1001 Saviour1001 requested a review from t-nelson April 19, 2022 14:16
@Saviour1001 Saviour1001 marked this pull request as ready for review April 19, 2022 14:48
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really close! Since the testing framework landed in #3070, how about a test to make sure this works as intended?

token/cli/src/main.rs Outdated Show resolved Hide resolved
token/cli/src/main.rs Outdated Show resolved Hide resolved
@mvines
Copy link
Contributor

mvines commented Apr 19, 2022

What if we just always denied the transfer if the account owner is the token program. No need for a user flag to override at all, this is always wrong.

@joncinque
Copy link
Contributor

@mvines the current behavior is to allow people to specify the token account address as well, e.g. if your wallet is vines.... and your ATA for USDC is usdcvines...., I can transfer USDC to you by either specifying spl-token transfer usdc_mint 1 usdcvines.... or spl-token transfer usdc_mint 1 vines...., which means that it's valid if the account owner is the token program in the first case. We'll need to keep that behavior for non-ATAs. Let me know if I misunderstood what you meant.

@mvines
Copy link
Contributor

mvines commented Apr 19, 2022

I think I'm being dense, what's an example of a spl-token transfer command-line that gets the user into this situation?

@joncinque
Copy link
Contributor

It's possible for someone to do spl-transfer usdc_mint 10 usdc_mint --fund-recipient and create an ATA owned by the USDC mint, which can (and has been) accidentally done. This is meant to add one more guard rail, to check that the owning account is a system account.

@mvines
Copy link
Contributor

mvines commented Apr 19, 2022

ah, k. if the account owner is a token account, require that the account not be a mint?

@joncinque
Copy link
Contributor

I was thinking we could cover a larger surface area by checking the program owner is the system account, and that way we also protect against things like:

spl-token transfer usdc_mint 10 raydium_pool --fund-recipient

or

spl-token transfer usdc_mint 10 stake_account --fund-recipient

@Saviour1001 Saviour1001 requested a review from joncinque April 20, 2022 22:35
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for the contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants