Fix pay-to-self Accounts bug

[CriesofCarrots]

Problem

Top level bug: Given an account with a positive token balance, it is possible to magically increase your balance by paying tokens to yourself (bank processes a SystemTransaction::new_move or SystemTransaction::new_account, where the signing keypair and recipient pubkey are the same account).

Root cause: When the same account is listed as both the "from" keypair and "to" pubkey, the account is loaded into the tx accounts vector twice. When runtime::execute_instruction is called, the pre_total:post_total check passes, because the debit is applied to one instance of the account, and the credit to the other. Then when the accounts are re-stored, the 2nd account instance overwrites the first, saving the credited account.

Summary of Changes

Add check for unique keys when loading accounts for a discrete transaction.

... There are other potential ways to solve this bug, but I have yet to think of any legitimate reasons why one transaction would need to load the same account twice. I believe that is why Instructions index into the accounts array, after all.

Fixes #

[codecov]

Codecov Report

Merging #2682 into master will increase coverage by <.1%.
The diff coverage is 96.2%.

@@           Coverage Diff            @@
##           master   #2682     +/-   ##
========================================
+ Coverage    76.6%   76.7%   +<.1%     
========================================
  Files         113     113             
  Lines       18383   18431     +48     
========================================
+ Hits        14090   14137     +47     
- Misses       4293    4294      +1

[aeyakovenko]

How does this get pass the account lock code?

[aeyakovenko]

@CriesofCarrots, so the bug is in Accounts::lock_accounts
It should be doing something like:

• accumulate the keys in a local hashset
• check for dups between the global and the local
• commit the loca set for the tx if all keys are valid

The reason for the two step commit is because if the tx fails all the keys for the tx need to be reverted. That might be another option.

[CriesofCarrots]

@aeyakovenko , actually, I think you mean Accounts::lock_account

solana/src/accounts.rs

Line 283 in cdb2a7b

fn lock_account(

I assumed that was desirable (I'm not clear on the assumptions made about Transaction slices in banking_stage). If it's not, another option would be to remove lines 294-295

[garious]

The double loop in lock_account is why the invalid transaction doesn't report AccountInUse. Merging those loops would catch this invalid transaction, but would make that error ambiguous. It's suppose to mean "try again later", not "invalid transaction - duplicate keys"

[garious]

I prefer the fix in this PR

[CriesofCarrots]

I'm running with it 👼

[CriesofCarrots]

FYI, @TristanDebrunner , thanks!

[mvines]

Looks like this broke master

[mvines]

https://buildkite.com/solana-labs/solana/builds/1598

[CriesofCarrots]

@mvines #2692 🙇‍♀️

[aeyakovenko]

@garious, we generally want to fail as early as possible. In this case all the accounts remain locked for this failed tx until the entire batch is unlocked.

[garious]

@aeyakovenko, I agree, we should fail as early as possible. But in this case, that means we should verify the individual transaction before we do the cross-transaction AccountInUse check. We do other individual tx checks earlier, like sigverify.