feat(executor): validate no confined account is modified (#740)

Author: bmuddha

Cargo.lock

@@ -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,18 @@ 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);
 
+        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 +361,50 @@ 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()
+                        && !acc.privileged()
                 })
                 .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(),
     }
 }

magicblock-processor/src/executor/processing.rs

@@ -1,7 +1,6 @@
 use std::{collections::HashSet, time::Duration};
 
 use guinea::GuineaInstruction;
-use magicblock_core::traits::AccountsBank;
 use solana_account::{ReadableAccount, WritableAccount};
 use solana_keypair::Keypair;
 use solana_program::{
@@ -51,7 +50,7 @@ async fn test_insufficient_fee() {
     let env = ExecutionTestEnv::new();
     let mut payer = env.get_payer();
     payer.set_lamports(ExecutionTestEnv::BASE_FEE - 1);
-    payer.commmit();
+    payer.commit();
 
     let (ix, _) =
         setup_guinea_instruction(&env, &GuineaInstruction::PrintSizes, false);
@@ -105,7 +104,7 @@ async fn test_non_delegated_payer_rejection() {
     let mut payer = env.get_payer();
     payer.set_delegated(false); // Mark the payer as not delegated
     let fee_payer_initial_balance = payer.lamports();
-    payer.commmit();
+    payer.commit();
 
     let (ix, _) =
         setup_guinea_instruction(&env, &GuineaInstruction::PrintSizes, false);
@@ -133,7 +132,7 @@ async fn test_escrowed_payer_success() {
     payer.set_lamports(ExecutionTestEnv::BASE_FEE - 1);
     payer.set_delegated(false);
     let escrow = ephemeral_balance_pda_from_payer(&payer.pubkey);
-    payer.commmit();
+    payer.commit();
 
     env.fund_account(escrow, LAMPORTS_PER_SOL); // Fund the escrow PDA
 
@@ -220,7 +219,7 @@ async fn test_escrow_charged_for_failed_transaction() {
     payer.set_lamports(0);
     payer.set_delegated(false);
     let escrow = ephemeral_balance_pda_from_payer(&payer.pubkey);
-    payer.commmit();
+    payer.commit();
     let account = env
         .create_account_with_config(LAMPORTS_PER_SOL, 0, guinea::ID) // Account with no data
         .pubkey();
@@ -266,7 +265,7 @@ async fn test_transaction_gasless_mode() {
     payer.set_lamports(1); // Not enough to cover standard fee
     payer.set_delegated(false); // Explicitly set the payer as NON-delegated.
     let initial_balance = payer.lamports();
-    payer.commmit();
+    payer.commit();
 
     let ix = Instruction::new_with_bincode(
         guinea::ID,
@@ -312,7 +311,7 @@ async fn test_transaction_gasless_mode_with_not_existing_account() {
     payer.set_lamports(1); // Not enough to cover standard fee
     payer.set_delegated(false); // Explicitly set the payer as NON-delegated.
     let initial_balance = payer.lamports();
-    payer.commmit();
+    payer.commit();
 
     let ix = Instruction::new_with_bincode(
         guinea::ID,
@@ -351,51 +350,3 @@ async fn test_transaction_gasless_mode_with_not_existing_account() {
         "payer balance should not change in gasless mode"
     );
 }
-
-/// Verifies that in zero-fee ("gasless") mode, transactions are processed
-/// successfully even when the fee payer does not exists.
-#[tokio::test]
-async fn test_transaction_gasless_mode_not_existing_feepayer() {
-    // Initialize the environment with a base fee of 0.
-    let env = ExecutionTestEnv::new_with_config(0);
-    let payer = env.get_payer().pubkey;
-    env.accountsdb.remove_account(&payer);
-
-    // Simple noop instruction that does not touch the fee payer account
-    let ix = Instruction::new_with_bincode(
-        guinea::ID,
-        &GuineaInstruction::PrintSizes,
-        vec![],
-    );
-    let txn = env.build_transaction(&[ix]);
-    let signature = txn.signatures[0];
-
-    // In a normal fee-paying mode, this execution would fail.
-    env.execute_transaction(txn)
-        .await
-        .expect("transaction should succeed in gasless mode");
-
-    // Verify the transaction was fully processed and broadcast successfully.
-    let status = env
-        .dispatch
-        .transaction_status
-        .recv_timeout(Duration::from_millis(100))
-        .expect("should receive a transaction status update");
-
-    assert_eq!(status.signature, signature);
-    assert!(
-        status.result.result.is_ok(),
-        "Transaction execution should be successful"
-    );
-
-    // Verify that the payer balance is zero (or doesn't exist)
-    let final_balance = env
-        .accountsdb
-        .get_account(&payer)
-        .unwrap_or_default()
-        .lamports();
-    assert_eq!(
-        final_balance, 0,
-        "payer balance of a not existing feepayer should be 0 in gasless mode"
-    );
-}

magicblock-processor/tests/fees.rs

@@ -0,0 +1,122 @@
+use guinea::GuineaInstruction;
+use solana_account::{ReadableAccount, WritableAccount};
+use solana_program::{
+    instruction::{AccountMeta, Instruction},
+    native_token::LAMPORTS_PER_SOL,
+};
+use solana_signer::Signer;
+use solana_transaction_error::TransactionError;
+use test_kit::ExecutionTestEnv;
+
+/// Verifies that in gasless mode (0 fees), a transaction fails
+/// if it modifies an undelegated fee payer.
+#[tokio::test]
+async fn test_gasless_undelegated_feepayer_modification_fails() {
+    let env = ExecutionTestEnv::new_with_config(0);
+
+    {
+        let mut account = env.get_payer();
+        account.set_owner(guinea::ID);
+        account.set_delegated(false);
+        account.commit();
+    }
+    env.advance_slot();
+
+    // 3. Create a recipient (also owned by Guinea for the Transfer instruction compatibility)
+    let recipient = env.create_account_with_config(100, 42, guinea::ID);
+
+    // 4. Create Transfer instruction: Payer -> Recipient
+    // This modifies the payer's lamports.
+    let ix = Instruction::new_with_bincode(
+        guinea::ID,
+        &GuineaInstruction::Transfer(1000),
+        vec![
+            AccountMeta::new(env.payer.pubkey(), true),
+            AccountMeta::new(recipient.pubkey(), false),
+        ],
+    );
+
+    // 5. Build transaction using the default payer
+    let txn = env.build_transaction(&[ix]);
+
+    // 6. Execute and Assert Failure
+    let result = env.execute_transaction(txn).await;
+    assert!(
+        result.is_err(),
+        "Modification of undelegated fee payer in gasless mode should fail"
+    );
+    assert_eq!(
+        result.unwrap_err(),
+        TransactionError::InvalidAccountForFee,
+        "Expected InvalidAccountForFee error"
+    );
+}
+
+/// Verifies that modifying the lamports of a confined
+/// account causes the transaction to fail.
+#[tokio::test]
+async fn test_confined_account_lamport_modification_fails() {
+    let env = ExecutionTestEnv::new(); // Standard fees
+
+    let confined_sender = env.create_account_with_config(100, 42, guinea::ID);
+    {
+        let mut account = env.get_account(confined_sender.pubkey());
+        account.set_confined(true);
+        account.commit();
+    }
+    env.advance_slot();
+
+    let recipient = env.create_account_with_config(100, 42, guinea::ID);
+
+    let ix = Instruction::new_with_bincode(
+        guinea::ID,
+        &GuineaInstruction::Transfer(100),
+        vec![
+            AccountMeta::new(confined_sender.pubkey(), false), // Writable, Not Signer
+            AccountMeta::new(recipient.pubkey(), false),       // Writable
+        ],
+    );
+
+    let txn = env.build_transaction(&[ix]);
+
+    let result = env.execute_transaction(txn).await;
+
+    assert!(
+        result.is_err(),
+        "Modifying lamports of a confined account should fail"
+    );
+    assert_eq!(result.unwrap_err(), TransactionError::UnbalancedTransaction);
+}
+
+/// Verifies that modifying ONLY the DATA of a confined account SUCCEEDS.
+#[tokio::test]
+async fn test_confined_account_data_modification_succeeds() {
+    let env = ExecutionTestEnv::new();
+
+    let confined_account =
+        env.create_account_with_config(LAMPORTS_PER_SOL, 42, guinea::ID);
+    {
+        let mut account = env.get_account(confined_account.pubkey());
+        account.set_confined(true);
+        account.commit();
+    }
+    env.advance_slot();
+
+    let ix = Instruction::new_with_bincode(
+        guinea::ID,
+        &GuineaInstruction::WriteByteToData(42),
+        vec![AccountMeta::new(confined_account.pubkey(), false)],
+    );
+
+    let txn = env.build_transaction(&[ix]);
+
+    let result = env.execute_transaction(txn).await;
+
+    assert!(
+        result.is_ok(),
+        "Data-only modification of confined account should succeed"
+    );
+
+    let acc = env.get_account(confined_account.pubkey());
+    assert_eq!(acc.data()[0], 42, "Data should be updated");
+}

magicblock-processor/tests/security.rs

@@ -74,7 +74,7 @@ rayon = "1.10.0"
 schedulecommit-client = { path = "schedulecommit/client" }
 serde = "1.0.217"
 serial_test = "3.2.0"
-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-loader-v2-interface = "2.2"
 solana-loader-v3-interface = "4.0"
 solana-loader-v4-interface = "2.1"
@@ -106,5 +106,5 @@ url = "2.5.0"
 # and we use protobuf-src v2.1.1. Otherwise compilation fails
 solana-storage-proto = { path = "../storage-proto" }
 # same reason as above
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
-rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }
+rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "57158728" }