Merge pull request from GHSA-93gq-39cq-f735

* Fix delete_key storage_usage and switch to saturating_sub

* Rename protocol version constant

* Update to `if let Some()`

* Bump version to 1.16.2

core/primitives/src/version.rs

@@ -18,7 +18,7 @@ pub const DB_VERSION: DbVersion = 13;
 pub type ProtocolVersion = u32;
 
 /// Current latest version of the protocol.
-pub const PROTOCOL_VERSION: ProtocolVersion = 39;
+pub const PROTOCOL_VERSION: ProtocolVersion = 40;
 /// Oldest supported version by this client.
 pub const OLDEST_BACKWARD_COMPATIBLE_PROTOCOL_VERSION: ProtocolVersion = 34;
 
@@ -44,3 +44,6 @@ pub const UPGRADABILITY_FIX_PROTOCOL_VERSION: ProtocolVersion = 37;
 
 /// Updates the way receipt ID, data ID and random seeds are constructed.
 pub const CREATE_HASH_PROTOCOL_VERSION: ProtocolVersion = 38;
+
+/// Fix the storage usage of the delete key action.
+pub const DELETE_KEY_STORAGE_USAGE_PROTOCOL_VERSION: ProtocolVersion = 40;

neard/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "neard"
-version = "1.16.1"
+version = "1.16.2"
 authors = ["Near Inc <[email protected]>"]
 edition = "2018"
 default-run = "neard"

neard/res/genesis_config.json

@@ -1,5 +1,5 @@
 {
-  "protocol_version": 39,
+  "protocol_version": 40,
   "genesis_time": "1970-01-01T00:00:00.000000000Z",
   "chain_id": "sample",
   "genesis_height": 0,
@@ -236,4 +236,4 @@
   "fishermen_threshold": "10000000000000000000000000",
   "minimum_stake_divisor": 10,
   "records": []
-}
+}

runtime/runtime/src/actions.rs

@@ -29,7 +29,10 @@ use crate::ext::RuntimeExt;
 use crate::{ActionResult, ApplyState};
 use near_crypto::PublicKey;
 use near_primitives::errors::{ActionError, ActionErrorKind, ExternalError, RuntimeError};
-use near_primitives::version::{ProtocolVersion, IMPLICIT_ACCOUNT_CREATION_PROTOCOL_VERSION};
+use near_primitives::version::{
+    ProtocolVersion, DELETE_KEY_STORAGE_USAGE_PROTOCOL_VERSION,
+    IMPLICIT_ACCOUNT_CREATION_PROTOCOL_VERSION,
+};
 use near_runtime_configs::AccountCreationConfig;
 use near_vm_errors::{CompilationError, FunctionCallError};
 use near_vm_runner::VMError;
@@ -363,13 +366,7 @@ pub(crate) fn action_deploy_contract(
     let code = ContractCode::new(deploy_contract.code.clone(), None);
     let prev_code = get_code(state_update, account_id, Some(account.code_hash))?;
     let prev_code_length = prev_code.map(|code| code.code.len() as u64).unwrap_or_default();
-    account.storage_usage =
-        account.storage_usage.checked_sub(prev_code_length).ok_or_else(|| {
-            StorageError::StorageInconsistentState(format!(
-                "Storage usage integer underflow for account {}",
-                account_id
-            ))
-        })?;
+    account.storage_usage = account.storage_usage.checked_sub(prev_code_length).unwrap_or(0);
     account.storage_usage =
         account.storage_usage.checked_add(code.code.len() as u64).ok_or_else(|| {
             StorageError::StorageInconsistentState(format!(
@@ -411,32 +408,31 @@ pub(crate) fn action_delete_key(
     result: &mut ActionResult,
     account_id: &AccountId,
     delete_key: &DeleteKeyAction,
+    current_protocol_version: ProtocolVersion,
 ) -> Result<(), StorageError> {
     let access_key = get_access_key(state_update, &account_id, &delete_key.public_key)?;
-    if access_key.is_none() {
+    if let Some(access_key) = access_key {
+        let storage_usage_config = &fee_config.storage_usage_config;
+        let storage_usage = if current_protocol_version >= DELETE_KEY_STORAGE_USAGE_PROTOCOL_VERSION
+        {
+            delete_key.public_key.try_to_vec().unwrap().len() as u64
+                + access_key.try_to_vec().unwrap().len() as u64
+                + storage_usage_config.num_extra_bytes_record
+        } else {
+            delete_key.public_key.try_to_vec().unwrap().len() as u64
+                + Some(access_key).try_to_vec().unwrap().len() as u64
+                + storage_usage_config.num_extra_bytes_record
+        };
+        // Remove access key
+        remove_access_key(state_update, account_id.clone(), delete_key.public_key.clone());
+        account.storage_usage = account.storage_usage.checked_sub(storage_usage).unwrap_or(0);
+    } else {
         result.result = Err(ActionErrorKind::DeleteKeyDoesNotExist {
             public_key: delete_key.public_key.clone(),
             account_id: account_id.clone(),
         }
         .into());
-        return Ok(());
     }
-    // Remove access key
-    remove_access_key(state_update, account_id.clone(), delete_key.public_key.clone());
-    let storage_usage_config = &fee_config.storage_usage_config;
-    account.storage_usage = account
-        .storage_usage
-        .checked_sub(
-            delete_key.public_key.try_to_vec().unwrap().len() as u64
-                + access_key.try_to_vec().unwrap().len() as u64
-                + storage_usage_config.num_extra_bytes_record,
-        )
-        .ok_or_else(|| {
-            StorageError::StorageInconsistentState(format!(
-                "Storage usage integer underflow for account {}",
-                account_id
-            ))
-        })?;
     Ok(())
 }
 

runtime/runtime/src/lib.rs

@@ -435,6 +435,7 @@ impl Runtime {
                     &mut result,
                     account_id,
                     delete_key,
+                    apply_state.current_protocol_version,
                 )?;
             }
             Action::DeleteAccount(delete_account) => {
@@ -1437,7 +1438,9 @@ mod tests {
     use near_primitives::errors::ReceiptValidationError;
     use near_primitives::hash::hash;
     use near_primitives::test_utils::{account_new, MockEpochInfoProvider};
-    use near_primitives::transaction::{FunctionCallAction, TransferAction};
+    use near_primitives::transaction::{
+        AddKeyAction, DeleteKeyAction, FunctionCallAction, TransferAction,
+    };
     use near_primitives::types::MerkleHash;
     use near_primitives::version::PROTOCOL_VERSION;
     use near_store::test_utils::create_tries;
@@ -1498,6 +1501,8 @@ mod tests {
 
         let mut initial_state = tries.new_trie_update(0, root);
         let mut initial_account = account_new(initial_balance, hash(&[]));
+        // For the account and a full access key
+        initial_account.storage_usage = 182;
         initial_account.locked = initial_locked;
         set_account(&mut initial_state, account_id.clone(), &initial_account);
         set_access_key(
@@ -2196,4 +2201,107 @@ mod tests {
         // Burnt all the fees + all prepaid gas.
         assert_eq!(result.stats.tx_burnt_amount, total_receipt_cost);
     }
+
+    #[test]
+    fn test_delete_key_add_key() {
+        let initial_locked = to_yocto(500_000);
+        let (runtime, tries, root, apply_state, signer, epoch_info_provider) =
+            setup_runtime(to_yocto(1_000_000), initial_locked, 10u64.pow(15));
+
+        let state_update = tries.new_trie_update(0, root);
+        let initial_account_state = get_account(&state_update, &alice_account()).unwrap().unwrap();
+
+        let actions = vec![
+            Action::DeleteKey(DeleteKeyAction { public_key: signer.public_key() }),
+            Action::AddKey(AddKeyAction {
+                public_key: signer.public_key(),
+                access_key: AccessKey::full_access(),
+            }),
+        ];
+
+        let receipts = vec![Receipt {
+            predecessor_id: alice_account(),
+            receiver_id: alice_account(),
+            receipt_id: CryptoHash::default(),
+            receipt: ReceiptEnum::Action(ActionReceipt {
+                signer_id: alice_account(),
+                signer_public_key: signer.public_key(),
+                gas_price: GAS_PRICE,
+                output_data_receivers: vec![],
+                input_data_ids: vec![],
+                actions,
+            }),
+        }];
+
+        let apply_result = runtime
+            .apply(
+                tries.get_trie_for_shard(0),
+                root,
+                &None,
+                &apply_state,
+                &receipts,
+                &[],
+                &epoch_info_provider,
+            )
+            .unwrap();
+        let (store_update, root) = tries.apply_all(&apply_result.trie_changes, 0).unwrap();
+        store_update.commit().unwrap();
+
+        let state_update = tries.new_trie_update(0, root);
+        let final_account_state = get_account(&state_update, &alice_account()).unwrap().unwrap();
+
+        assert_eq!(initial_account_state.storage_usage, final_account_state.storage_usage);
+    }
+
+    #[test]
+    fn test_delete_key_underflow() {
+        let initial_locked = to_yocto(500_000);
+        let (runtime, tries, root, apply_state, signer, epoch_info_provider) =
+            setup_runtime(to_yocto(1_000_000), initial_locked, 10u64.pow(15));
+
+        let mut state_update = tries.new_trie_update(0, root);
+        let mut initial_account_state =
+            get_account(&state_update, &alice_account()).unwrap().unwrap();
+        initial_account_state.storage_usage = 10;
+        set_account(&mut state_update, alice_account(), &initial_account_state);
+        state_update.commit(StateChangeCause::InitialState);
+        let trie_changes = state_update.finalize().unwrap().0;
+        let (store_update, root) = tries.apply_all(&trie_changes, 0).unwrap();
+        store_update.commit().unwrap();
+
+        let actions = vec![Action::DeleteKey(DeleteKeyAction { public_key: signer.public_key() })];
+
+        let receipts = vec![Receipt {
+            predecessor_id: alice_account(),
+            receiver_id: alice_account(),
+            receipt_id: CryptoHash::default(),
+            receipt: ReceiptEnum::Action(ActionReceipt {
+                signer_id: alice_account(),
+                signer_public_key: signer.public_key(),
+                gas_price: GAS_PRICE,
+                output_data_receivers: vec![],
+                input_data_ids: vec![],
+                actions,
+            }),
+        }];
+
+        let apply_result = runtime
+            .apply(
+                tries.get_trie_for_shard(0),
+                root,
+                &None,
+                &apply_state,
+                &receipts,
+                &[],
+                &epoch_info_provider,
+            )
+            .unwrap();
+        let (store_update, root) = tries.apply_all(&apply_result.trie_changes, 0).unwrap();
+        store_update.commit().unwrap();
+
+        let state_update = tries.new_trie_update(0, root);
+        let final_account_state = get_account(&state_update, &alice_account()).unwrap().unwrap();
+
+        assert_eq!(final_account_state.storage_usage, 0);
+    }
 }