feat(vault_transaction_execute,batch_execute_transaction): enforce ms accounts readonly in CPI instead of reentrancy checks

Author: vovacodes

programs/multisig/src/errors.rs

@@ -32,8 +32,6 @@ pub enum MultisigError {
     InvalidNumberOfAccounts,
     #[msg("Invalid account provided")]
     InvalidAccount,
-    #[msg("`transaction_execute` reentrancy is forbidden")]
-    ExecuteReentrancy,
     #[msg("Cannot remove last member")]
     RemoveLastMember,
     #[msg("Members don't include any voters")]
@@ -62,4 +60,6 @@ pub enum MultisigError {
     DecimalsMismatch,
     #[msg("Member has unknown permission")]
     UnknownPermission,
+    #[msg("Account is protected, it cannot be passed into a CPI as writable")]
+    ProtectedAccount,
 }

programs/multisig/src/instructions/batch_execute_transaction.rs

@@ -104,7 +104,7 @@ impl BatchExecuteTransaction<'_> {
 
     /// Execute a transaction from the batch.
     #[access_control(ctx.accounts.validate())]
-    pub fn batch_execute_transaction(ctx: Context<BatchExecuteTransaction>) -> Result<()> {
+    pub fn batch_execute_transaction(ctx: Context<Self>) -> Result<()> {
         let multisig = &mut ctx.accounts.multisig;
         let proposal = &mut ctx.accounts.proposal;
         let batch = &mut ctx.accounts.batch;
@@ -146,11 +146,7 @@ impl BatchExecuteTransaction<'_> {
             &ephemeral_signer_keys,
         )?;
 
-        let current_status = proposal.status.clone();
-        // Set the proposal state to Executing to prevent reentrancy attacks (e.g. cancelling proposal) in the middle of execution.
-        proposal.status = ProposalStatus::Executing;
-        let proposal_account_info = proposal.to_account_info();
-        proposal.try_serialize(&mut &mut proposal_account_info.data.borrow_mut()[..])?;
+        let protected_accounts = &[proposal.key(), batch_key];
 
         // Execute the transaction message instructions one-by-one.
         executable_message.execute_message(
@@ -159,11 +155,9 @@ impl BatchExecuteTransaction<'_> {
                 .map(|seed| seed.to_vec())
                 .collect::<Vec<Vec<u8>>>(),
             &ephemeral_signer_seeds,
+            protected_accounts,
         )?;
 
-        // Restore the proposal status after execution.
-        proposal.status = current_status;
-
         // Increment the executed transaction index.
         batch.executed_transaction_index = batch
             .executed_transaction_index

programs/multisig/src/instructions/vault_transaction_execute.rs

@@ -28,7 +28,6 @@ pub struct VaultTransactionExecute<'info> {
 
     /// The transaction to execute.
     #[account(
-        mut,
         seeds = [
             SEED_PREFIX,
             multisig.key().as_ref(),
@@ -39,7 +38,6 @@ pub struct VaultTransactionExecute<'info> {
     )]
     pub transaction: Account<'info, VaultTransaction>,
 
-    #[account(mut)]
     pub member: Signer<'info>,
     // `remaining_accounts` must include the following accounts in the exact order:
     // 1. AddressLookupTable accounts in the order they appear in `message.address_table_lookups`.
@@ -128,10 +126,7 @@ impl VaultTransactionExecute<'_> {
             &ephemeral_signer_keys,
         )?;
 
-        // Set the proposal state to Executing to prevent reentrancy attacks (e.g. cancelling proposal) in the middle of execution.
-        proposal.status = ProposalStatus::Executing;
-        let proposal_account_info = proposal.to_account_info();
-        proposal.try_serialize(&mut &mut proposal_account_info.data.borrow_mut()[..])?;
+        let protected_accounts = &[proposal.key()];
 
         // Execute the transaction message instructions one-by-one.
         executable_message.execute_message(
@@ -140,6 +135,7 @@ impl VaultTransactionExecute<'_> {
                 .map(|seed| seed.to_vec())
                 .collect::<Vec<Vec<u8>>>(),
             &ephemeral_signer_seeds,
+            protected_accounts,
         )?;
 
         // Mark the proposal as executed.

programs/multisig/src/state/proposal.rs

@@ -137,6 +137,9 @@ pub enum ProposalStatus {
     /// Proposal has been approved and is pending execution.
     Approved { timestamp: i64 },
     /// Proposal is being executed. This is a transient state that always transitions to `Executed` in the span of a single transaction.
+    #[deprecated(
+        note = "This status used to be used to prevent reentrancy attacks. It is no longer needed."
+    )]
     Executing,
     /// Proposal has been executed.
     Executed { timestamp: i64 },

programs/multisig/src/utils/executable_transaction_message.rs

@@ -4,11 +4,9 @@ use std::convert::From;
 use anchor_lang::prelude::*;
 use anchor_lang::solana_program::instruction::Instruction;
 use anchor_lang::solana_program::program::invoke_signed;
-use anchor_lang::Discriminator;
 use solana_address_lookup_table_program::state::AddressLookupTable;
 
 use crate::errors::*;
-use crate::id;
 use crate::state::*;
 
 /// Sanitized and validated combination of a `MsTransactionMessage` and `AccountInfo`s it references.
@@ -171,21 +169,23 @@ impl<'a, 'info> ExecutableTransactionMessage<'a, 'info> {
         })
     }
 
+    /// Executes all instructions in the message via CPI calls.
+    /// # Arguments
+    /// * `vault_seeds` - Seeds for the vault PDA.
+    /// * `ephemeral_signer_seeds` - Seeds for the ephemeral signer PDAs.
+    /// * `protected_accounts` - Accounts that must not be passed as writable to the CPI calls to prevent potential reentrancy attacks.
     pub fn execute_message(
         &self,
         vault_seeds: &[Vec<u8>],
         ephemeral_signer_seeds: &[Vec<Vec<u8>>],
+        protected_accounts: &[Pubkey],
     ) -> Result<()> {
         for (ix, account_infos) in self.to_instructions_and_accounts().iter() {
-            // Make sure we don't allow reentrancy of transaction_execute.
-            if ix.program_id == id() {
-                require!(
-                    ix.data[..8] != crate::instruction::VaultTransactionExecute::DISCRIMINATOR,
-                    MultisigError::ExecuteReentrancy
-                );
+            // Make sure we don't pass protected accounts as writable to CPI calls.
+            for account_meta in ix.accounts.iter().filter(|m| m.is_writable) {
                 require!(
-                    ix.data[..8] != crate::instruction::BatchExecuteTransaction::DISCRIMINATOR,
-                    MultisigError::ExecuteReentrancy
+                    !protected_accounts.contains(&account_meta.pubkey),
+                    MultisigError::ProtectedAccount
                 );
             }
 

sdk/multisig/idl/multisig.json

@@ -396,15 +396,15 @@
         },
         {
           "name": "transaction",
-          "isMut": true,
+          "isMut": false,
           "isSigner": false,
           "docs": [
             "The transaction to execute."
           ]
         },
         {
           "name": "member",
-          "isMut": true,
+          "isMut": false,
           "isSigner": true
         }
       ],
@@ -2113,78 +2113,78 @@
     },
     {
       "code": 6015,
-      "name": "ExecuteReentrancy",
-      "msg": "`transaction_execute` reentrancy is forbidden"
-    },
-    {
-      "code": 6016,
       "name": "RemoveLastMember",
       "msg": "Cannot remove last member"
     },
     {
-      "code": 6017,
+      "code": 6016,
       "name": "NoVoters",
       "msg": "Members don't include any voters"
     },
     {
-      "code": 6018,
+      "code": 6017,
       "name": "NoProposers",
       "msg": "Members don't include any proposers"
     },
     {
-      "code": 6019,
+      "code": 6018,
       "name": "NoExecutors",
       "msg": "Members don't include any executors"
     },
     {
-      "code": 6020,
+      "code": 6019,
       "name": "InvalidStaleTransactionIndex",
       "msg": "`stale_transaction_index` must be <= `transaction_index`"
     },
     {
-      "code": 6021,
+      "code": 6020,
       "name": "NotSupportedForControlled",
       "msg": "Instruction not supported for controlled multisig"
     },
     {
-      "code": 6022,
+      "code": 6021,
       "name": "TimeLockNotReleased",
       "msg": "Proposal time lock has not been released"
     },
     {
-      "code": 6023,
+      "code": 6022,
       "name": "NoActions",
       "msg": "Config transaction must have at least one action"
     },
     {
-      "code": 6024,
+      "code": 6023,
       "name": "MissingAccount",
       "msg": "Missing account"
     },
     {
-      "code": 6025,
+      "code": 6024,
       "name": "InvalidMint",
       "msg": "Invalid mint"
     },
     {
-      "code": 6026,
+      "code": 6025,
       "name": "InvalidDestination",
       "msg": "Invalid destination"
     },
     {
-      "code": 6027,
+      "code": 6026,
       "name": "SpendingLimitExceeded",
       "msg": "Spending limit exceeded"
     },
     {
-      "code": 6028,
+      "code": 6027,
       "name": "DecimalsMismatch",
       "msg": "Decimals don't match the mint"
     },
     {
-      "code": 6029,
+      "code": 6028,
       "name": "UnknownPermission",
       "msg": "Member has unknown permission"
+    },
+    {
+      "code": 6029,
+      "name": "ProtectedAccount",
+      "msg": "Account is protected, it cannot be passed into a CPI as writable"
     }
   ],
   "metadata": {