From f46cfc04b959279a86d7e709194af734f3d73bd3 Mon Sep 17 00:00:00 2001
From: Bowen Wang <bowenwang1996@users.noreply.github.com>
Date: Tue, 19 May 2020 20:40:13 -0700
Subject: [PATCH] fix: delete account after unstake should not cause node to
 crash (#2688)

As described in #2687, there is a critical vulnerability caused by a situation where accounts are deleted after they unstake but we still try to return 0 stake to the account. This PR submits an easy fix to the issue by not requiring account to exist when the returned stake is no more than 0. Please note that this is not the optimal way to fix it. The better way would be to change epoch manager behavior so that it will only record accounts in `stake_change` if it is needed. In the case of unstaking, it means that a stake change should only be recorded if it is a change from nonzero (validator or fishermen) to zero. However, this PR is enough to quickly patch the issue so that we can spend more time ironing out the proper and better fix. Resolves #2687.
---
 neard/src/runtime.rs       | 59 +++++++++++++++++++++++++++++++++++++-
 runtime/runtime/src/lib.rs |  4 ++-
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/neard/src/runtime.rs b/neard/src/runtime.rs
index 0ea1ecd440d..ed4b9240587 100644
--- a/neard/src/runtime.rs
+++ b/neard/src/runtime.rs
@@ -1257,7 +1257,9 @@ mod test {
     use near_chain::{ReceiptResult, Tip};
     use near_crypto::{InMemorySigner, KeyType, Signer};
     use near_logger_utils::init_test_logger;
-    use near_primitives::transaction::{Action, CreateAccountAction, StakeAction};
+    use near_primitives::transaction::{
+        Action, CreateAccountAction, DeleteAccountAction, StakeAction,
+    };
     use near_primitives::types::{BlockHeightDelta, Nonce, ValidatorId, ValidatorKickoutReason};
     use near_primitives::validator_signer::{InMemoryValidatorSigner, ValidatorSigner};
     use near_primitives::views::{
@@ -2478,4 +2480,59 @@ mod test {
             TESTING_INIT_BALANCE + protocol_treasury_reward
         );
     }
+
+    #[test]
+    fn test_delete_account_after_unstake() {
+        init_test_logger();
+        let num_nodes = 2;
+        let validators = (0..num_nodes).map(|i| format!("test{}", i + 1)).collect::<Vec<_>>();
+        let mut env = TestEnv::new(
+            "test_validator_delete_account",
+            vec![validators.clone()],
+            4,
+            vec![],
+            vec![],
+            false,
+        );
+        let block_producers: Vec<_> = validators
+            .iter()
+            .map(|id| InMemoryValidatorSigner::from_seed(id, KeyType::ED25519, id))
+            .collect();
+        let signers: Vec<_> = validators
+            .iter()
+            .map(|id| InMemorySigner::from_seed(id, KeyType::ED25519, id))
+            .collect();
+
+        let staking_transaction1 = stake(1, &signers[1], &block_producers[1], 0);
+        env.step_default(vec![staking_transaction1]);
+        let account = env.view_account(&block_producers[1].validator_id());
+        assert_eq!(account.amount, TESTING_INIT_BALANCE - TESTING_INIT_STAKE);
+        assert_eq!(account.locked, TESTING_INIT_STAKE);
+        for _ in 2..=5 {
+            env.step_default(vec![]);
+        }
+        let staking_transaction2 = stake(2, &signers[1], &block_producers[1], 1);
+        env.step_default(vec![staking_transaction2]);
+        for _ in 7..=13 {
+            env.step_default(vec![]);
+        }
+        let account = env.view_account(&block_producers[1].validator_id());
+        assert_eq!(account.locked, 0);
+
+        let delete_account_transaction = SignedTransaction::from_actions(
+            4,
+            signers[1].account_id.clone(),
+            signers[1].account_id.clone(),
+            &signers[1] as &dyn Signer,
+            vec![Action::DeleteAccount(DeleteAccountAction {
+                beneficiary_id: signers[0].account_id.clone(),
+            })],
+            // runtime does not validate block history
+            CryptoHash::default(),
+        );
+        env.step_default(vec![delete_account_transaction]);
+        for _ in 15..=17 {
+            env.step_default(vec![]);
+        }
+    }
 }
diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs
index dd0ca2324f3..3ec34bcebc6 100644
--- a/runtime/runtime/src/lib.rs
+++ b/runtime/runtime/src/lib.rs
@@ -854,7 +854,9 @@ impl Runtime {
                     .ok_or_else(|| RuntimeError::UnexpectedIntegerOverflow)?;
 
                 set_account(state_update, account_id.clone(), &account);
-            } else {
+            } else if *max_of_stakes > 0 {
+                // if max_of_stakes > 0, it means that the account must have locked balance
+                // and therefore must exist
                 return Err(StorageError::StorageInconsistentState(format!(
                     "Account {} with max of stakes {} is not found",
                     account_id, max_of_stakes