system-program: Remove zero lamport check on transfers #17726

joncinque · 2021-06-03T23:20:00Z

Problem

It's possible to bypass signature checks if transferring 0 lamports, causing potential unclear / misleading behavior.

Summary of Changes

Perform all checks on transfers, even for 0 lamports.

Fixes #

jstarry · 2021-06-03T23:38:14Z

runtime/src/system_instruction_processor.rs

+
+    if invoke_context.is_feature_active(&feature_set::system_transfer_zero_check::id())
+        && lamports == 0
+    {
+        return Ok(());
+    }
+


this looks unnecessary. The lamports check below will be bypassed when lamports == 0

Yeah it looks a bit silly, but it avoids unnecessary calls to try_account_ref_mut() later on. If there's no possible issues with doing those, I'm happy to get rid of this

yeah no possible issues with that, let's get rid of it

runtime/src/system_instruction_processor.rs

t-nelson · 2021-06-04T04:02:28Z

runtime/src/system_instruction_processor.rs

@@ -1103,7 +1125,7 @@ mod tests {
            0,
            &MockInvokeContext::new(vec![]),
        )
-        .is_ok(),);
+        .is_err(),);


Can we assert_eq!(transfer(), Err(InstructionError::MissingRequiredSignature)); here instead? Reordering of errors will break consensus and we should be able to catch it.

Pull request has been modified.

joncinque · 2021-06-04T13:20:41Z

@jstarry @t-nelson Could I ask for another quick check? The stake split instruction uses the create_account with 0 lamports assumption, which makes one test fail in particular at

solana/runtime/tests/stake.rs

Line 399 in be957f2

assert!(bank_client

. In the test, we're splitting from a stake that is auto-authorized, so when we try to split from it, we get an error because an account with data is trying to transfer lamports. I've updated the split instruction to allocate + assign separately. Since this could break people using split with auto-authorized stakes in 1.6, I'll remove that backport and only backport to 1.7. Let me know what you think.

mvines · 2021-06-04T15:50:42Z

runtime/src/system_instruction_processor.rs

@@ -179,6 +179,7 @@ fn transfer_verified(
        ic_msg!(invoke_context, "Transfer: `from` must not carry data");
        return Err(InstructionError::InvalidArgument);
    }
+


nit: avoid change here

t-nelson · 2021-06-04T15:52:39Z

programs/stake/src/stake_instruction.rs

-            &id(),
-        ),
+        system_instruction::allocate(split_stake_pubkey, std::mem::size_of::<StakeState>() as u64),
+        system_instruction::assign(split_stake_pubkey, &id()),


This will break the Ledger app's parser. Splits will need to be blind signed until it's updated

Looks like it used to be allocate + assign, then something changed https://github.com/LedgerHQ/app-solana/blob/1c72216edf4e5358f719b164a8d1b6100988b34d/libsol/transaction_printers.c#L29

Ah right. Good ole lazy past-Trent saves the day 😉

That's a rarity. I don't think past me has ever helped me that much

Pull request has been modified.

t-nelson · 2021-06-04T19:48:15Z

programs/stake/src/stake_instruction.rs

@@ -288,7 +288,6 @@ pub fn split_with_seed(
            std::mem::size_of::<StakeState>() as u64,
            &id(),
        ),
-        system_instruction::assign_with_seed(split_stake_pubkey, base, seed, &id()),


Landmines everywhere!

solana/runtime/src/system_instruction_processor.rs

Lines 391 to 404 in 7bb5f24

SystemInstruction::AllocateWithSeed {

base,

seed,

space,

owner,

} => {

let keyed_account = keyed_account_at_index(keyed_accounts, 0)?;

let mut account = keyed_account.try_account_ref_mut()?;

let address = Address::create(

keyed_account.unsigned_key(),

Some((&base, &seed, &owner)),

invoke_context,

)?;

allocate_and_assign(

Seriously. You should see my most recent commit 😅

joncinque · 2021-06-04T21:40:46Z

@t-nelson @jstarry sorry, just one more check please on bdd6da6 -- with these changes, it's also possible to split a stake into an account that already has lamports, which is currently supported by the stake program. For some reason, there's an existing test to make sure that this isn't possible, even though this behavior is reasonable. As such, I've updated the test from a failure to a success.

jstarry · 2021-06-04T21:57:04Z

It's usually a mistake to split into an account which already has lamports. However, this is a recoverable mistake because a split account can have its extra lamports withdrawn and it can also be merged and closed completely if needed. I think that the "existing account" check should be a client side check in the cli rather than in the transaction. This looks good to me

t-nelson · 2021-06-04T22:44:07Z

It's usually a mistake to split into an account which already has lamports. However, this is a recoverable mistake because a split account can have its extra lamports withdrawn and it can also be merged and closed completely if needed. I think that the "existing account" check should be a client side check in the cli rather than in the transaction. This looks good to me

Agreed. This is no worse than doing a system transfer into a delegated stake account. Which is annoying, but no funds are at risk past a bit of dust for fees

codecov · 2021-06-05T01:18:41Z

Codecov Report

Merging #17726 (bdd6da6) into master (708bbcb) will decrease coverage by 0.0%.
The diff coverage is 40.7%.

@@            Coverage Diff            @@
##           master   #17726     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         431      431             
  Lines      120590   120610     +20     
=========================================
+ Hits        99839    99841      +2     
- Misses      20751    20769     +18

* system-program: Move lamports == 0 check on transfers * Address feedback * Update stake split to explicitly allocate + assign * Update stake tests referring to split instruction * Revert whitespace * Update split instruction index in test * Remove unnecessary `assign_with_seed` from `split_with_seed` * Fix stake instruction parser * Update test to allow splitting into account with lamports (cherry picked from commit 8f5e773) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/src/feature_set.rs

* system-program: Move lamports == 0 check on transfers * Address feedback * Update stake split to explicitly allocate + assign * Update stake tests referring to split instruction * Revert whitespace * Update split instruction index in test * Remove unnecessary `assign_with_seed` from `split_with_seed` * Fix stake instruction parser * Update test to allow splitting into account with lamports (cherry picked from commit 8f5e773)

* system-program: Move lamports == 0 check on transfers * Address feedback * Update stake split to explicitly allocate + assign * Update stake tests referring to split instruction * Revert whitespace * Update split instruction index in test * Remove unnecessary `assign_with_seed` from `split_with_seed` * Fix stake instruction parser * Update test to allow splitting into account with lamports (cherry picked from commit 8f5e773) Co-authored-by: Jon Cinque <jon.cinque@gmail.com>

…) (#17763) * system-program: Remove zero lamport check on transfers (#17726) * system-program: Move lamports == 0 check on transfers * Address feedback * Update stake split to explicitly allocate + assign * Update stake tests referring to split instruction * Revert whitespace * Update split instruction index in test * Remove unnecessary `assign_with_seed` from `split_with_seed` * Fix stake instruction parser * Update test to allow splitting into account with lamports (cherry picked from commit 8f5e773) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/src/feature_set.rs * Resolve merge conflicts Co-authored-by: Jon Cinque <jon.cinque@gmail.com>

system-program: Move lamports == 0 check on transfers

9c2fe69

joncinque requested review from jstarry and mvines June 3, 2021 23:20

jstarry reviewed Jun 3, 2021

View reviewed changes

runtime/src/system_instruction_processor.rs Show resolved Hide resolved

jstarry previously approved these changes Jun 3, 2021

View reviewed changes

joncinque added v1.7 labels Jun 3, 2021

t-nelson reviewed Jun 4, 2021

View reviewed changes

joncinque changed the title ~~system-program: Move zero lamport check on transfers~~ system-program: Remove zero lamport check on transfers Jun 4, 2021

Address feedback

813a8b1

Update stake split to explicitly allocate + assign

9223c3e

joncinque force-pushed the transfer-check branch from 1a3149e to 9223c3e Compare June 4, 2021 13:16

joncinque removed the v1.6 label Jun 4, 2021

Update stake tests referring to split instruction

b35c317

mvines reviewed Jun 4, 2021

View reviewed changes

mvines previously approved these changes Jun 4, 2021

View reviewed changes

mvines added the v1.6 label Jun 4, 2021

t-nelson reviewed Jun 4, 2021

View reviewed changes

t-nelson previously approved these changes Jun 4, 2021

View reviewed changes

joncinque added 3 commits June 4, 2021 18:06

Revert whitespace

655204f

Update split instruction index in test

33aec08

Remove unnecessary assign_with_seed from split_with_seed

3980e3f

Fix stake instruction parser

d7a9ff1

t-nelson reviewed Jun 4, 2021

View reviewed changes

Update test to allow splitting into account with lamports

bdd6da6

t-nelson approved these changes Jun 4, 2021

View reviewed changes

joncinque merged commit 8f5e773 into solana-labs:master Jun 5, 2021

joncinque deleted the transfer-check branch June 5, 2021 23:45

mergify bot mentioned this pull request Jun 5, 2021

system-program: Remove zero lamport check on transfers (backport #17726) #17763

Merged

mergify bot mentioned this pull request Jun 5, 2021

system-program: Remove zero lamport check on transfers (backport #17726) #17764

Merged

This was referenced Aug 23, 2021

backport 19361 v16 #19379

Closed

backport 19361 v17 #19380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

system-program: Remove zero lamport check on transfers #17726

system-program: Remove zero lamport check on transfers #17726

joncinque commented Jun 3, 2021

jstarry Jun 3, 2021

joncinque Jun 3, 2021

jstarry Jun 3, 2021

t-nelson Jun 4, 2021

joncinque commented Jun 4, 2021

mvines Jun 4, 2021

t-nelson Jun 4, 2021

joncinque Jun 4, 2021

t-nelson Jun 4, 2021

joncinque Jun 4, 2021

t-nelson Jun 4, 2021

joncinque Jun 4, 2021

joncinque commented Jun 4, 2021

jstarry commented Jun 4, 2021

t-nelson commented Jun 4, 2021

codecov bot commented Jun 5, 2021

	SystemInstruction::AllocateWithSeed {
	base,
	seed,
	space,
	owner,
	} => {
	let keyed_account = keyed_account_at_index(keyed_accounts, 0)?;
	let mut account = keyed_account.try_account_ref_mut()?;
	let address = Address::create(
	keyed_account.unsigned_key(),
	Some((&base, &seed, &owner)),
	invoke_context,
	)?;
	allocate_and_assign(

system-program: Remove zero lamport check on transfers #17726

system-program: Remove zero lamport check on transfers #17726

Conversation

joncinque commented Jun 3, 2021

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Jun 4, 2021

jstarry commented Jun 4, 2021

t-nelson commented Jun 4, 2021

codecov bot commented Jun 5, 2021

Codecov Report