Fails the multisig transaction upon the mismatch between provided and…

… onchain payloads (#13160)

Updates the `validate_multisig_transaction ` function, so that the multisig transaction fails if the provided payload does not match to the onchain payload. This fixes #12929.

api/src/tests/multisig_transactions_test.rs

@@ -333,6 +333,74 @@ async fn test_multisig_transaction_with_payload_not_matching_hash() {
         .await;
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_multisig_transaction_with_matching_payload() {
+    let mut context = new_test_context(current_function_name!());
+    let owner_account = &mut context.create_account().await;
+    let multisig_account = context
+        .create_multisig_account(owner_account, vec![], 1, 1000)
+        .await;
+    assert_eq!(1000, context.get_apt_balance(multisig_account).await);
+    let multisig_payload = construct_multisig_txn_transfer_payload(owner_account.address(), 1000);
+    context
+        .create_multisig_transaction(owner_account, multisig_account, multisig_payload.clone())
+        .await;
+    context
+        .execute_multisig_transaction_with_payload(
+            owner_account,
+            multisig_account,
+            "0x1::aptos_account::transfer",
+            &[],
+            &[&owner_account.address().to_hex_literal(), "1000"],
+            202,
+        )
+        .await;
+
+    // The multisig tx that transfers away 1000 APT should have succeeded.
+    assert_multisig_tx_executed(&mut context, multisig_account, multisig_payload, 1).await;
+    assert_eq!(0, context.get_apt_balance(multisig_account).await);
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_multisig_transaction_with_mismatching_payload() {
+    let mut context = new_test_context(current_function_name!());
+    let owner_account = &mut context.create_account().await;
+    let multisig_account = context
+        .create_multisig_account(owner_account, vec![], 1, 1000)
+        .await;
+    let multisig_payload = construct_multisig_txn_transfer_payload(owner_account.address(), 1000);
+    context
+        .create_multisig_transaction(owner_account, multisig_account, multisig_payload.clone())
+        .await;
+
+    // The multisig transaction execution should fail due to the payload mismatch
+    // amount being different (1000 vs 2000).
+    context
+        .execute_multisig_transaction_with_payload(
+            owner_account,
+            multisig_account,
+            "0x1::aptos_account::transfer",
+            &[],
+            &[&owner_account.address().to_hex_literal(), "2000"],
+            400,
+        )
+        .await;
+    // Balance didn't change since the target transaction failed.
+    assert_eq!(1000, context.get_apt_balance(multisig_account).await);
+
+    // Excuting the transaction with the correct payload should succeed.
+    context
+        .execute_multisig_transaction_with_payload(
+            owner_account,
+            multisig_account,
+            "0x1::aptos_account::transfer",
+            &[],
+            &[&owner_account.address().to_hex_literal(), "1000"],
+            202,
+        )
+        .await;
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_multisig_transaction_simulation() {
     let mut context = new_test_context(current_function_name!());

aptos-move/aptos-release-builder/src/components/feature_flags.rs

@@ -120,6 +120,7 @@ pub enum FeatureFlag {
     ConcurrentFungibleBalance,
     DefaultToConcurrentFungibleBalance,
     LimitVMTypeSize,
+    AbortIfMultisigPayloadMismatch,
 }
 
 fn generate_features_blob(writer: &CodeWriter, data: &[u64]) {
@@ -312,6 +313,9 @@ impl From<FeatureFlag> for AptosFeatureFlag {
                 AptosFeatureFlag::DEFAULT_TO_CONCURRENT_FUNGIBLE_BALANCE
             },
             FeatureFlag::LimitVMTypeSize => AptosFeatureFlag::LIMIT_VM_TYPE_SIZE,
+            FeatureFlag::AbortIfMultisigPayloadMismatch => {
+                AptosFeatureFlag::ABORT_IF_MULTISIG_PAYLOAD_MISMATCH
+            },
         }
     }
 }
@@ -433,6 +437,9 @@ impl From<AptosFeatureFlag> for FeatureFlag {
                 FeatureFlag::DefaultToConcurrentFungibleBalance
             },
             AptosFeatureFlag::LIMIT_VM_TYPE_SIZE => FeatureFlag::LimitVMTypeSize,
+            AptosFeatureFlag::ABORT_IF_MULTISIG_PAYLOAD_MISMATCH => {
+                FeatureFlag::AbortIfMultisigPayloadMismatch
+            },
         }
     }
 }

aptos-move/aptos-vm/src/aptos_vm.rs

@@ -1091,7 +1091,14 @@ impl AptosVM {
             bcs::to_bytes(&payload).map_err(|_| invariant_violation_error())?
         } else {
             // Default to empty bytes if payload is not provided.
-            bcs::to_bytes::<Vec<u8>>(&vec![]).map_err(|_| invariant_violation_error())?
+            if self
+                .features()
+                .is_abort_if_multisig_payload_mismatch_enabled()
+            {
+                vec![]
+            } else {
+                bcs::to_bytes::<Vec<u8>>(&vec![]).map_err(|_| invariant_violation_error())?
+            }
         };
         // Failures here will be propagated back.
         let payload_bytes: Vec<Vec<u8>> = session
@@ -2353,6 +2360,7 @@ impl AptosVM {
                         session,
                         txn_data,
                         multisig_payload,
+                        self.features(),
                         log_context,
                         traversal_context,
                     )

aptos-move/aptos-vm/src/errors.rs

@@ -48,6 +48,8 @@ const EMULTISIG_TRANSACTION_NOT_FOUND: u64 = 2006;
 const EMULTISIG_PAYLOAD_DOES_NOT_MATCH_HASH: u64 = 2008;
 // Multisig transaction has not received enough approvals to be executed.
 const EMULTISIG_NOT_ENOUGH_APPROVALS: u64 = 2009;
+// Provided target function does not match the payload stored in the on-chain transaction.
+const EPAYLOAD_DOES_NOT_MATCH: u64 = 2010;
 
 const INVALID_ARGUMENT: u8 = 0x1;
 const LIMIT_EXCEEDED: u8 = 0x2;
@@ -88,6 +90,9 @@ pub fn convert_prologue_error(
                 (INVALID_ARGUMENT, EMULTISIG_PAYLOAD_DOES_NOT_MATCH_HASH) => {
                     StatusCode::MULTISIG_TRANSACTION_PAYLOAD_DOES_NOT_MATCH_HASH
                 },
+                (INVALID_ARGUMENT, EPAYLOAD_DOES_NOT_MATCH) => {
+                    StatusCode::MULTISIG_TRANSACTION_PAYLOAD_DOES_NOT_MATCH
+                },
                 (category, reason) => {
                     let err_msg = format!("[aptos_vm] Unexpected prologue Move abort: {:?}::{:?} (Category: {:?} Reason: {:?})",
                     location, code, category, reason);

aptos-move/aptos-vm/src/transaction_validation.rs

@@ -188,6 +188,7 @@ pub(crate) fn run_multisig_prologue(
     session: &mut SessionExt,
     txn_data: &TransactionMetadata,
     payload: &Multisig,
+    features: &Features,
     log_context: &AdapterLogSchema,
     traversal_context: &mut TraversalContext,
 ) -> Result<(), VMStatus> {
@@ -196,7 +197,11 @@ pub(crate) fn run_multisig_prologue(
         bcs::to_bytes(&payload).map_err(|_| unreachable_error.clone())?
     } else {
         // Default to empty bytes if payload is not provided.
-        bcs::to_bytes::<Vec<u8>>(&vec![]).map_err(|_| unreachable_error)?
+        if features.is_abort_if_multisig_payload_mismatch_enabled() {
+            vec![]
+        } else {
+            bcs::to_bytes::<Vec<u8>>(&vec![]).map_err(|_| unreachable_error)?
+        }
     };
 
     session

aptos-move/framework/aptos-framework/doc/multisig_account.md

@@ -1413,6 +1413,16 @@ Transaction payload cannot be empty.
 
 
 
+<a id="0x1_multisig_account_EPAYLOAD_DOES_NOT_MATCH"></a>
+
+Provided target function does not match the payload stored in the on-chain transaction.
+
+
+<pre><code><b>const</b> <a href="multisig_account.md#0x1_multisig_account_EPAYLOAD_DOES_NOT_MATCH">EPAYLOAD_DOES_NOT_MATCH</a>: u64 = 2010;
+</code></pre>
+
+
+
 <a id="0x1_multisig_account_EPAYLOAD_DOES_NOT_MATCH_HASH"></a>
 
 Provided target function does not match the hash stored in the on-chain transaction.
@@ -2938,7 +2948,7 @@ Remove the next transaction if it has sufficient owner rejections.
 
     <b>let</b> sequence_number = <a href="multisig_account.md#0x1_multisig_account_last_resolved_sequence_number">last_resolved_sequence_number</a>(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>) + 1;
     <b>let</b> owner_addr = address_of(owner);
-    <b>if</b>(<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
+    <b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
         // Implicitly vote for rejection <b>if</b> the owner <b>has</b> not voted for rejection yet.
         <b>if</b> (!<a href="multisig_account.md#0x1_multisig_account_has_voted_for_rejection">has_voted_for_rejection</a>(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>, sequence_number, owner_addr)) {
             <a href="multisig_account.md#0x1_multisig_account_reject_transaction">reject_transaction</a>(owner, <a href="multisig_account.md#0x1_multisig_account">multisig_account</a>, sequence_number);
@@ -3037,7 +3047,7 @@ Transaction payload is optional if it's already stored on chain for the transact
     <b>let</b> sequence_number = <a href="multisig_account.md#0x1_multisig_account_last_resolved_sequence_number">last_resolved_sequence_number</a>(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>) + 1;
     <a href="multisig_account.md#0x1_multisig_account_assert_transaction_exists">assert_transaction_exists</a>(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>, sequence_number);
 
-    <b>if</b>(<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
+    <b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
         <b>assert</b>!(
             <a href="multisig_account.md#0x1_multisig_account_can_execute">can_execute</a>(address_of(owner), <a href="multisig_account.md#0x1_multisig_account">multisig_account</a>, sequence_number),
             <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="multisig_account.md#0x1_multisig_account_ENOT_ENOUGH_APPROVALS">ENOT_ENOUGH_APPROVALS</a>),
@@ -3061,6 +3071,19 @@ Transaction payload is optional if it's already stored on chain for the transact
             <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="multisig_account.md#0x1_multisig_account_EPAYLOAD_DOES_NOT_MATCH_HASH">EPAYLOAD_DOES_NOT_MATCH_HASH</a>),
         );
     };
+
+    // If the transaction payload is stored on chain and there is a provided payload,
+    // verify that the provided payload matches the stored payload.
+    <b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_abort_if_multisig_payload_mismatch_enabled">features::abort_if_multisig_payload_mismatch_enabled</a>()
+        && <a href="../../aptos-stdlib/../move-stdlib/doc/option.md#0x1_option_is_some">option::is_some</a>(&transaction.payload)
+        && !<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_is_empty">vector::is_empty</a>(&payload)
+    ) {
+        <b>let</b> stored_payload = <a href="../../aptos-stdlib/../move-stdlib/doc/option.md#0x1_option_borrow">option::borrow</a>(&transaction.payload);
+        <b>assert</b>!(
+            payload == *stored_payload,
+            <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="multisig_account.md#0x1_multisig_account_EPAYLOAD_DOES_NOT_MATCH">EPAYLOAD_DOES_NOT_MATCH</a>),
+        );
+    }
 }
 </code></pre>
 
@@ -3195,7 +3218,7 @@ This function is private so no other code can call this beside the VM itself as
     <b>let</b> multisig_account_resource = <b>borrow_global_mut</b>&lt;<a href="multisig_account.md#0x1_multisig_account_MultisigAccount">MultisigAccount</a>&gt;(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>);
     <b>let</b> (num_approvals, _) = <a href="multisig_account.md#0x1_multisig_account_remove_executed_transaction">remove_executed_transaction</a>(multisig_account_resource);
 
-    <b>if</b>(<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>() && implicit_approval) {
+    <b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>() && implicit_approval) {
         <b>if</b> (std::features::module_event_migration_enabled()) {
             emit(
                 <a href="multisig_account.md#0x1_multisig_account_Vote">Vote</a> {
@@ -3272,7 +3295,7 @@ This function is private so no other code can call this beside the VM itself as
     <a href="multisig_account.md#0x1_multisig_account">multisig_account</a>: <b>address</b>,
     transaction: <a href="multisig_account.md#0x1_multisig_account_MultisigTransaction">MultisigTransaction</a>
 ) {
-    <b>if</b>(<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
+    <b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/features.md#0x1_features_multisig_v2_enhancement_feature_enabled">features::multisig_v2_enhancement_feature_enabled</a>()) {
         <b>assert</b>!(
             <a href="multisig_account.md#0x1_multisig_account_available_transaction_queue_capacity">available_transaction_queue_capacity</a>(<a href="multisig_account.md#0x1_multisig_account">multisig_account</a>) &gt; 0,
             <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_state">error::invalid_state</a>(<a href="multisig_account.md#0x1_multisig_account_EMAX_PENDING_TRANSACTIONS_EXCEEDED">EMAX_PENDING_TRANSACTIONS_EXCEEDED</a>)

aptos-move/framework/aptos-framework/sources/multisig_account.move

@@ -75,6 +75,8 @@ module aptos_framework::multisig_account {
     const EPAYLOAD_DOES_NOT_MATCH_HASH: u64 = 2008;
     /// Transaction has not received enough approvals to be executed.
     const ENOT_ENOUGH_APPROVALS: u64 = 2009;
+    /// Provided target function does not match the payload stored in the on-chain transaction.
+    const EPAYLOAD_DOES_NOT_MATCH: u64 = 2010;
     /// Transaction has not received enough rejections to be officially rejected.
     const ENOT_ENOUGH_REJECTIONS: u64 = 10;
     /// Number of signatures required must be more than zero and at most the total number of owners.
@@ -1018,7 +1020,7 @@ module aptos_framework::multisig_account {
 
         let sequence_number = last_resolved_sequence_number(multisig_account) + 1;
         let owner_addr = address_of(owner);
-        if(features::multisig_v2_enhancement_feature_enabled()) {
+        if (features::multisig_v2_enhancement_feature_enabled()) {
             // Implicitly vote for rejection if the owner has not voted for rejection yet.
             if (!has_voted_for_rejection(multisig_account, sequence_number, owner_addr)) {
                 reject_transaction(owner, multisig_account, sequence_number);
@@ -1079,7 +1081,7 @@ module aptos_framework::multisig_account {
         let sequence_number = last_resolved_sequence_number(multisig_account) + 1;
         assert_transaction_exists(multisig_account, sequence_number);
 
-        if(features::multisig_v2_enhancement_feature_enabled()) {
+        if (features::multisig_v2_enhancement_feature_enabled()) {
             assert!(
                 can_execute(address_of(owner), multisig_account, sequence_number),
                 error::invalid_argument(ENOT_ENOUGH_APPROVALS),
@@ -1103,6 +1105,19 @@ module aptos_framework::multisig_account {
                 error::invalid_argument(EPAYLOAD_DOES_NOT_MATCH_HASH),
             );
         };
+
+        // If the transaction payload is stored on chain and there is a provided payload,
+        // verify that the provided payload matches the stored payload.
+        if (features::abort_if_multisig_payload_mismatch_enabled()
+            && option::is_some(&transaction.payload)
+            && !vector::is_empty(&payload)
+        ) {
+            let stored_payload = option::borrow(&transaction.payload);
+            assert!(
+                payload == *stored_payload,
+                error::invalid_argument(EPAYLOAD_DOES_NOT_MATCH),
+            );
+        }
     }
 
     /// Post-execution cleanup for a successful multisig transaction execution.
@@ -1179,7 +1194,7 @@ module aptos_framework::multisig_account {
         let multisig_account_resource = borrow_global_mut<MultisigAccount>(multisig_account);
         let (num_approvals, _) = remove_executed_transaction(multisig_account_resource);
 
-        if(features::multisig_v2_enhancement_feature_enabled() && implicit_approval) {
+        if (features::multisig_v2_enhancement_feature_enabled() && implicit_approval) {
             if (std::features::module_event_migration_enabled()) {
                 emit(
                     Vote {
@@ -1217,7 +1232,7 @@ module aptos_framework::multisig_account {
         multisig_account: address,
         transaction: MultisigTransaction
     ) {
-        if(features::multisig_v2_enhancement_feature_enabled()) {
+        if (features::multisig_v2_enhancement_feature_enabled()) {
             assert!(
                 available_transaction_queue_capacity(multisig_account) > 0,
                 error::invalid_state(EMAX_PENDING_TRANSACTIONS_EXCEEDED)
@@ -1481,7 +1496,7 @@ module aptos_framework::multisig_account {
     fun setup() {
         let framework_signer = &create_signer(@0x1);
         features::change_feature_flags_for_testing(
-            framework_signer, vector[features::get_multisig_accounts_feature(), features::get_multisig_v2_enhancement_feature()], vector[]);
+            framework_signer, vector[features::get_multisig_accounts_feature(), features::get_multisig_v2_enhancement_feature(), features::get_abort_if_multisig_payload_mismatch_feature()], vector[]);
         timestamp::set_time_has_started_for_testing(framework_signer);
         chain_id::initialize_for_test(framework_signer, 1);
         let (burn, mint) = aptos_coin::initialize_for_test(framework_signer);