token-cli: withdraw-withheld-tokens silently fails

[joncinque]

Copied from anza-xyz/agave#2224 cc @tapirtoken

Problem

If you look at this transaction:

4N1GvNc27DFhKK7Y8dcjMhrDuze4LMfM1qhoG6nkFG2EWTMPEcuevXVGEXmwt6gR55VEskQ9X37Z8RKkP1HhgL8i

You'll notice that instruction 5 (on Solscan) is HarvestWithheldTokensToMint. However, the Withdraw Authority is Null so this operation should normally fail. But this is an exceptional case in which the tokens are being immediately burned (by instruction 6) in the same atomic transaction, so the harvest is allowed in order to permit the wallet owner to recover the rent.

Nevertheless we wanted to make absolutely sure that there was no way to withdraw withheld tokens, so we tried to withdraw our own:

spl-token withdraw-withheld-tokens 8QhMP4HVfUd78hSjwzRXmifYCWAYB2nYiM6Emd7fTstd --fee-payer AirDpo61FVsNDoWSBoBX3K4gcBiTVnkfZRAZk7bKfRtX.json --url mainnet-beta --output json --program-id TokenzQdBNbLqP5VEhdkAS6EPFLC1PHnBqCXEpPxuEb --withdraw-withheld-authority AirDpo61FVsNDoWSBoBX3K4gcBiTVnkfZRAZk7bKfRtX.json

This command just terminates as though it succeeded. It definitely didn't succeed because our Withheld Amount on explorer.solana.com is unchanged:

8QhMP4HVfUd78hSjwzRXmifYCWAYB2nYiM6Emd7fTstd

This is correct behavior but withdraw-withheld-tokens should have returned an error message.

Proposed Solution

Just check if the operation succeeded and report to the user.

[zzau13]

@joncinque I understand that to check if the operation has gone well, you only have to check the amount of the destination is equal before and after the operation and launch an error message with eprintln. Simply asking for the status of the destination when the transaction ends and checking the equality in the amount. If so, can you assign it to me? What error message should I return?

[joncinque]

This issue is a bit of a misunderstanding of how withdraw-withheld-tokens is supposed to work, but shows that it should be clearer. The account provided 8QhMP4HVfUd78hSjwzRXmifYCWAYB2nYiM6Emd7fTstd is actually the destination for the withheld tokens, and then the command takes in a list of pubkeys to withdraw from, and the --include-mint flag if the tokens should be withdrawn from the mint.

In this case, neither a list of pubkeys nor the --include-mint flag is passed, which passes validation, and then does nothing. So we should probably return an error if the number of source accounts is 0 and the --include-mint flag is not present, saying something like:

No source accounts for withheld tokens provided. Please include a list of pubkeys to withdraw from, or the --include-mint flag to withdraw tokens withheld in the mint.

It might be easiest to put include_mint and source from

solana-program-library/token/cli/src/clap_app.rs

Lines 2244 to 2258 in 209362f

	.arg(
	Arg::with_name("source")
	.validator(is_valid_pubkey)
	.value_name("ACCOUNT_ADDRESS")
	.takes_value(true)
	.multiple(true)
	.min_values(0u64)
	.help("The token accounts to withdraw from")
	)
	.arg(
	Arg::with_name("include_mint")
	.long("include-mint")
	.takes_value(false)
	.help("Also withdraw withheld tokens from the mint"),
	)

in a required ArgGroup with multiple(true) https://docs.rs/clap/2.34.0/clap/struct.ArgGroup.html, and then we don't need to worry about a custom error message.

And I don't assign issues to outside contributors, but if you put in a PR, I will happily review it!

[tapirtoken]

Would just like to add that "mint" is an ambiguous term, as in HarvestWithheldTokensToMint. It's not immediately clear whether it refers to the mint address or the mint authority or "the account provided". Might want to make that explicit in the help text.

Test case: what happens if this "mint" has its own Withheld Amount (if that's even possible) and you HarvestWithheldTokensToMint? (Only applies if "mint" can mean mint address or mint authority.)

There are use cases where one would want to withdraw Withheld Amount back to the mint authority (as taxes) and other use cases where one would simply want to free some percentage of them within the same token account. And just so no readers make the mistake: mint authority and mint address must be separate in the "tax" case because mint addresses cannot spend or move SOL.

[joncinque]

Would just like to add that "mint" is an ambiguous term, as in HarvestWithheldTokensToMint. It's not immediately clear whether it refers to the mint address or the mint authority or "the account provided". Might want to make that explicit in the help text.

Maybe I'm misunderstanding, but where do you suggest making it clearer that "mint" does not refer to the mint authority? I'd probably find that more confusing, to be honest.

Test case: what happens if this "mint" has its own Withheld Amount (if that's even possible) and you HarvestWithheldTokensToMint? (Only applies if "mint" can mean mint address or mint authority.)

The mint does have its own withheld amount, but since it's not a token account, you can't harvest from it. We could rename the instruction to HarvestWithheldTokensFromTokenAccountToMint, but it's a bit wordy.

Either way, token accounts can actually be owned by mint accounts -- if you keep the keypair for the mint account, then you can still sign for it, same as any token account owner. Fun fact: check out all of the token accounts owned by the USDC mint account, EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v:

Token                                         Balance
------------------------------------------------------------
EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v  104369.386161

That's $100k, completely bricked 😅

[tapirtoken]

@joncinque Well "HarvestWithheldTokensToMint" could (naively) mean:

1. Harvest withheld tokens from the token account in the source wallet and send them to the token account owned by the mint address wallet. (In hindsight this doesn't make a lot of sense since mint address wallets are barred from spending SOL.)

2. Harvest withheld tokens from the token account in the source wallet and send them to the token account owned by the mint authority wallet. This is the most logical interpretation, and if I'm understanding you, the correct one.

3. Harvest the withheld tokens "(in order) to mint (them again, in effect)", all within the same source wallet.

"RevertWithheldToMintAuthority" is almost impossible to misinterpret. Ah semantics!

LOL $104K!

[joncinque]

@tapirtoken it's actually none of those! HarvestWithheldTokensToMint moves withheld tokens from a token account to the mint itself, and not to any token account.