fix: check for confined accounts modification (#747)

Author: bmuddha

Cargo.lock

@@ -38,7 +38,7 @@ resolver = "2"
 
 [workspace.package]
 # Solana Version (2.2.x)
-version = "0.4.1"
+version = "0.4.2"
 authors = ["MagicBlock Maintainers <maintainers@magicblock.xyz>"]
 repository = "https://github.com/magicblock-labs/ephemeral-validator"
 homepage = "https://www.magicblock.xyz"
@@ -131,7 +131,7 @@ serde_json = "1.0"
 serde_with = "3.16"
 serial_test = "3.2"
 sha3 = "0.10.8"
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "57158728" }
 solana-account-decoder = { version = "2.2" }
 solana-account-decoder-client-types = { version = "2.2" }
 solana-account-info = { version = "2.2" }
@@ -216,11 +216,10 @@ features = ["lz4"]
 # some solana dependencies have solana-storage-proto as dependency
 # we need to patch them with our version, because they use protobuf-src v1.1.0
 # and we use protobuf-src v2.1.1. Otherwise compilation fails
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "57158728" }
 solana-storage-proto = { path = "./storage-proto" }
 solana-svm = { git = "https://github.com/magicblock-labs/magicblock-svm.git", rev = "3e9456ec4" }
 # Fork is used to enable `disable_manual_compaction` usage
 # Fork is based on commit d4e9e16 of rocksdb (parent commit of 0.23.0 release)
 # without patching update isn't possible due to conflict with solana deps
-# TODO(edwin): remove once solana deps upgraded and are using rust-rocksdb 0.25.0(likely)
-rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }
+rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }

Cargo.toml

@@ -101,10 +101,10 @@ impl AccountsDb {
                 // atomic counter. New readers will see the latest update.
                 acc.commit();
                 // check whether the account's owner has changed
-                if !acc.owner_changed {
+                if !acc.owner_changed() {
                     return;
                 }
-                // and perform some index bookkeeping to ensure correct owner
+                // and perform some index bookkeeping to ensure a correct owner
                 let _ = self
                     .index
                     .ensure_correct_owner(pubkey, account.owner())

magicblock-accounts-db/src/lib.rs

@@ -341,16 +341,23 @@ impl super::TransactionExecutor {
 
     /// Ensure that no post execution account state violations occurred:
     /// 1. No modification of the non-delegated feepayer in gasless mode
-    /// 2. No illegal account resizing when the balance is zero
+    /// 2. No lamport modification of confined accounts
     fn verify_account_states(&self, processed: &mut ProcessedTransaction) {
         let ProcessedTransaction::Executed(executed) = processed else {
             return;
         };
         let txn = &executed.loaded_transaction;
         let feepayer = txn.accounts.first();
-        let rollback_lamports =
-            rollback_feepayer_lamports(&txn.rollback_accounts);
+        // If the feepayer is priveleged we don't enforce any checks, as those
+        // are internal operations, that might violate some of those rules
+        if feepayer.as_ref().map(|a| a.1.privileged()).unwrap_or(false) {
+            return;
+        }
 
+        let logs = executed
+            .execution_details
+            .log_messages
+            .get_or_insert_default();
         let gasless = self.environment.fee_lamports_per_signature == 0;
         if gasless {
             // If we are running in the gasless mode, we should not allow
@@ -359,40 +366,49 @@ impl super::TransactionExecutor {
             // from undelegated feepayers to delegated accounts, which would
             // result in validator loosing funds upon balance settling.
             let undelegated_feepayer_was_modified = feepayer
-                .map(|acc| {
-                    (acc.1.is_dirty()
-                        && !self.is_auto_airdrop_lamports_enabled
-                        && (acc.1.lamports() != 0 || rollback_lamports != 0))
-                        && !acc.1.delegated()
-                        && !acc.1.privileged()
+                .map(|(_, acc)| {
+                    let mutated = acc
+                        .as_borrowed()
+                        .map(|a| a.lamports_changed())
+                        // NOTE: this branch can be taken only, if the account
+                        // has been upgraded to the Owned variant, which indicates
+                        // that it has been resized, which in turn is a clear
+                        // violation of the feepayer immutability rule in this mode
+                        .unwrap_or(true);
+                    !self.is_auto_airdrop_lamports_enabled
+                        && mutated
+                        && !acc.delegated()
                 })
                 .unwrap_or_default();
             if undelegated_feepayer_was_modified {
                 executed.execution_details.status =
                     Err(TransactionError::InvalidAccountForFee);
-                let logs = executed
-                    .execution_details
-                    .log_messages
-                    .get_or_insert_default();
-                let msg = "Feepayer balance has been modified illegally".into();
+                let msg = "Feepayer balance has been illegally modified".into();
                 logs.push(msg);
+                return;
             }
         }
-    }
-}
+        for (pubkey, acc) in &txn.accounts {
+            if !acc.confined() {
+                continue;
+            }
+            // If the confined account was modified in any way that affected its lamport
+            // balance, then an corresponding marker must have been set, in which case we
+            // fail the transaction, since this is regarded as a validator draining attack
+            let balance_changed = acc
+                .as_borrowed()
+                .map(|a| a.lamports_changed())
+                .unwrap_or(true);
 
-// A utility to extract the rollback lamports of the feepayer
-fn rollback_feepayer_lamports(rollback: &RollbackAccounts) -> u64 {
-    match rollback {
-        RollbackAccounts::FeePayerOnly { fee_payer_account } => {
-            fee_payer_account.lamports()
-        }
-        RollbackAccounts::SameNonceAndFeePayer { nonce } => {
-            nonce.account().lamports()
+            if balance_changed {
+                executed.execution_details.status =
+                    Err(TransactionError::UnbalancedTransaction);
+                let msg = format!(
+                    "Confined account {pubkey} has been illegally modified"
+                );
+                logs.push(msg);
+                break;
+            }
         }
-        RollbackAccounts::SeparateNonceAndFeePayer {
-            fee_payer_account,
-            ..
-        } => fee_payer_account.lamports(),
     }
 }