fix(runtime) DeleteAccount action must be the last action (#2829)

In order to avoid dDos we disallow to have DeleteAccount to be not a last action in Receipt.

@vgrichina this contains a new rpc error type to handle

fixes #2599

chain/jsonrpc/res/rpc_errors_schema.json

@@ -460,6 +460,7 @@
     "ActionsValidationError": {
       "name": "ActionsValidationError",
       "subtypes": [
+        "DeleteActionMustBeFinal",
         "TotalPrepaidGasExceeded",
         "TotalNumberOfActionsExceeded",
         "AddKeyMethodNamesNumberOfBytesExceeded",
@@ -553,6 +554,11 @@
         "account_id": ""
       }
     },
+    "DeleteActionMustBeFinal": {
+      "name": "DeleteActionMustBeFinal",
+      "subtypes": [],
+      "props": {}
+    },
     "DeleteKeyDoesNotExist": {
       "name": "DeleteKeyDoesNotExist",
       "subtypes": [],

core/primitives/src/errors.rs

@@ -173,6 +173,8 @@ pub enum InvalidAccessKeyError {
     BorshSerialize, BorshDeserialize, Serialize, Deserialize, Debug, Clone, PartialEq, Eq, RpcError,
 )]
 pub enum ActionsValidationError {
+    /// The delete action must be a final aciton in transaction
+    DeleteActionMustBeFinal,
     /// The total prepaid gas (for all given actions) exceeded the limit.
     TotalPrepaidGasExceeded { total_prepaid_gas: Gas, limit: Gas },
     /// The number of actions exceeded the given limit.
@@ -253,6 +255,9 @@ impl Display for ReceiptValidationError {
 impl Display for ActionsValidationError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
         match self {
+            ActionsValidationError::DeleteActionMustBeFinal => {
+                write!(f, "The delete action must be the last action in transaction")
+            }
             ActionsValidationError::TotalPrepaidGasExceeded { total_prepaid_gas, limit } => {
                 write!(f, "The total prepaid gas {} exceeds the limit {}", total_prepaid_gas, limit)
             }

core/primitives/src/version.rs

@@ -17,6 +17,6 @@ pub const DB_VERSION: DbVersion = 1;
 pub type ProtocolVersion = u32;
 
 /// Current latest version of the protocol.
-pub const PROTOCOL_VERSION: ProtocolVersion = 22;
+pub const PROTOCOL_VERSION: ProtocolVersion = 23;
 
 pub const FIRST_BACKWARD_COMPATIBLE_PROTOCOL_VERSION: ProtocolVersion = PROTOCOL_VERSION;

neard/res/genesis_config.json

@@ -1,5 +1,5 @@
 {
-  "protocol_version": 22,
+  "protocol_version": 23,
   "genesis_time": "1970-01-01T00:00:00.000000000Z",
   "chain_id": "sample",
   "genesis_height": 0,

runtime/runtime/src/verifier.rs

@@ -260,7 +260,13 @@ pub(crate) fn validate_actions(
         });
     }
 
-    for action in actions {
+    let mut iter = actions.iter().peekable();
+    while let Some(action) = iter.next() {
+        if let Action::DeleteAccount(_) = action {
+            if iter.peek().is_some() {
+                return Err(ActionsValidationError::DeleteActionMustBeFinal);
+            }
+        }
         validate_action(limit_config, action)?;
     }
 
@@ -1305,6 +1311,39 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_validate_delete_must_be_final() {
+        let mut limit_config = VMLimitConfig::default();
+        limit_config.max_actions_per_receipt = 3;
+        assert_eq!(
+            validate_actions(
+                &limit_config,
+                &vec![
+                    Action::DeleteAccount(DeleteAccountAction { beneficiary_id: "bob".into() }),
+                    Action::CreateAccount(CreateAccountAction {}),
+                ]
+            )
+            .expect_err("Expected an error"),
+            ActionsValidationError::DeleteActionMustBeFinal,
+        );
+    }
+
+    #[test]
+    fn test_validate_delete_must_work_if_its_final() {
+        let mut limit_config = VMLimitConfig::default();
+        limit_config.max_actions_per_receipt = 3;
+        assert_eq!(
+            validate_actions(
+                &limit_config,
+                &vec![
+                    Action::CreateAccount(CreateAccountAction {}),
+                    Action::DeleteAccount(DeleteAccountAction { beneficiary_id: "bob".into() }),
+                ]
+            ),
+            Ok(()),
+        );
+    }
+
     // Individual actions
 
     #[test]

scripts/migrations/23-delete_action_last.py

@@ -0,0 +1,19 @@
+"""
+Adds an assert for the Action::DeleteAccount to be the last action in Receipt
+"""
+
+import sys
+import os
+import json
+from collections import OrderedDict
+
+home = sys.argv[1]
+output_home = sys.argv[2]
+
+config = json.load(open(os.path.join(home, 'output.json')), object_pairs_hook=OrderedDict)
+
+assert config['protocol_version'] == 22
+
+config['protocol_version'] = 23
+
+json.dump(config, open(os.path.join(output_home, 'output.json'), 'w'), indent=2)