diff --git a/api/src/tests/multisig_transactions_test.rs b/api/src/tests/multisig_transactions_test.rs index b02c5b0c976fb..5b11faf347b36 100644 --- a/api/src/tests/multisig_transactions_test.rs +++ b/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!()); diff --git a/aptos-move/aptos-release-builder/src/components/feature_flags.rs b/aptos-move/aptos-release-builder/src/components/feature_flags.rs index 4ad2ebfd80995..f19e53681363f 100644 --- a/aptos-move/aptos-release-builder/src/components/feature_flags.rs +++ b/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 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 for FeatureFlag { FeatureFlag::DefaultToConcurrentFungibleBalance }, AptosFeatureFlag::LIMIT_VM_TYPE_SIZE => FeatureFlag::LimitVMTypeSize, + AptosFeatureFlag::ABORT_IF_MULTISIG_PAYLOAD_MISMATCH => { + FeatureFlag::AbortIfMultisigPayloadMismatch + }, } } } diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index a3405da02ddfc..e5f0206c23709 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/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![]).map_err(|_| invariant_violation_error())? + if self + .features() + .is_abort_if_multisig_payload_mismatch_enabled() + { + vec![] + } else { + bcs::to_bytes::>(&vec![]).map_err(|_| invariant_violation_error())? + } }; // Failures here will be propagated back. let payload_bytes: Vec> = session @@ -2353,6 +2360,7 @@ impl AptosVM { session, txn_data, multisig_payload, + self.features(), log_context, traversal_context, ) diff --git a/aptos-move/aptos-vm/src/errors.rs b/aptos-move/aptos-vm/src/errors.rs index 0b95668fb4ef5..629c576875b37 100644 --- a/aptos-move/aptos-vm/src/errors.rs +++ b/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); diff --git a/aptos-move/aptos-vm/src/transaction_validation.rs b/aptos-move/aptos-vm/src/transaction_validation.rs index a45a45d757087..9448713eb34b9 100644 --- a/aptos-move/aptos-vm/src/transaction_validation.rs +++ b/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![]).map_err(|_| unreachable_error)? + if features.is_abort_if_multisig_payload_mismatch_enabled() { + vec![] + } else { + bcs::to_bytes::>(&vec![]).map_err(|_| unreachable_error)? + } }; session diff --git a/aptos-move/framework/aptos-framework/doc/multisig_account.md b/aptos-move/framework/aptos-framework/doc/multisig_account.md index ad1a376c19d47..a5fcc8ab9b835 100644 --- a/aptos-move/framework/aptos-framework/doc/multisig_account.md +++ b/aptos-move/framework/aptos-framework/doc/multisig_account.md @@ -1413,6 +1413,16 @@ Transaction payload cannot be empty. + + +Provided target function does not match the payload stored in the on-chain transaction. + + +
const EPAYLOAD_DOES_NOT_MATCH: u64 = 2010;
+
+ + + 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. 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); @@ -3037,7 +3047,7 @@ Transaction payload is optional if it's already stored on chain for the transact 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), @@ -3061,6 +3071,19 @@ Transaction payload is optional if it's already stored on chain for the transact 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), + ); + } } @@ -3195,7 +3218,7 @@ This function is private so no other code can call this beside the VM itself as 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 { @@ -3272,7 +3295,7 @@ This function is private so no other code can call this beside the VM itself as 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) diff --git a/aptos-move/framework/aptos-framework/sources/multisig_account.move b/aptos-move/framework/aptos-framework/sources/multisig_account.move index c2b2484f49ad0..6ea72d7e0ed0d 100644 --- a/aptos-move/framework/aptos-framework/sources/multisig_account.move +++ b/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(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); diff --git a/aptos-move/framework/move-stdlib/doc/features.md b/aptos-move/framework/move-stdlib/doc/features.md index 4b1cd8181e625..d35bb996c55c8 100644 --- a/aptos-move/framework/move-stdlib/doc/features.md +++ b/aptos-move/framework/move-stdlib/doc/features.md @@ -127,6 +127,8 @@ return true. - [Function `concurrent_fungible_balance_enabled`](#0x1_features_concurrent_fungible_balance_enabled) - [Function `get_default_to_concurrent_fungible_balance_feature`](#0x1_features_get_default_to_concurrent_fungible_balance_feature) - [Function `default_to_concurrent_fungible_balance_enabled`](#0x1_features_default_to_concurrent_fungible_balance_enabled) +- [Function `get_abort_if_multisig_payload_mismatch_feature`](#0x1_features_get_abort_if_multisig_payload_mismatch_feature) +- [Function `abort_if_multisig_payload_mismatch_enabled`](#0x1_features_abort_if_multisig_payload_mismatch_enabled) - [Function `change_feature_flags`](#0x1_features_change_feature_flags) - [Function `change_feature_flags_internal`](#0x1_features_change_feature_flags_internal) - [Function `change_feature_flags_for_next_epoch`](#0x1_features_change_feature_flags_for_next_epoch) @@ -143,6 +145,7 @@ return true. - [Function `periodical_reward_rate_decrease_enabled`](#@Specification_1_periodical_reward_rate_decrease_enabled) - [Function `partial_governance_voting_enabled`](#@Specification_1_partial_governance_voting_enabled) - [Function `module_event_enabled`](#@Specification_1_module_event_enabled) + - [Function `abort_if_multisig_payload_mismatch_enabled`](#@Specification_1_abort_if_multisig_payload_mismatch_enabled) - [Function `change_feature_flags_internal`](#@Specification_1_change_feature_flags_internal) - [Function `change_feature_flags_for_next_epoch`](#@Specification_1_change_feature_flags_for_next_epoch) - [Function `on_new_epoch`](#@Specification_1_on_new_epoch) @@ -221,6 +224,19 @@ On epoch change, the updates take effect and this buffer is cleared. ## Constants + + +Whether the multisig v2 fix is enabled. Once enabled, the multisig transaction execution will explicitly +abort if the provided payload does not match the payload stored on-chain. + +Lifetime: transient + + +
const ABORT_IF_MULTISIG_PAYLOAD_MISMATCH: u64 = 70;
+
+ + + @@ -3090,6 +3106,52 @@ Lifetime: transient + + + + +## Function `get_abort_if_multisig_payload_mismatch_feature` + + + +
public fun get_abort_if_multisig_payload_mismatch_feature(): u64
+
+ + + +
+Implementation + + +
public fun get_abort_if_multisig_payload_mismatch_feature(): u64 { ABORT_IF_MULTISIG_PAYLOAD_MISMATCH }
+
+ + + +
+ + + +## Function `abort_if_multisig_payload_mismatch_enabled` + + + +
public fun abort_if_multisig_payload_mismatch_enabled(): bool
+
+ + + +
+Implementation + + +
public fun abort_if_multisig_payload_mismatch_enabled(): bool acquires Features {
+    is_enabled(ABORT_IF_MULTISIG_PAYLOAD_MISMATCH)
+}
+
+ + +
@@ -3531,6 +3593,35 @@ Helper to check whether a feature flag is enabled. + + + + +
fun spec_abort_if_multisig_payload_mismatch_enabled(): bool {
+   spec_is_enabled(ABORT_IF_MULTISIG_PAYLOAD_MISMATCH)
+}
+
+ + + + + +### Function `abort_if_multisig_payload_mismatch_enabled` + + +
public fun abort_if_multisig_payload_mismatch_enabled(): bool
+
+ + + + +
pragma opaque;
+aborts_if [abstract] false;
+ensures [abstract] result == spec_abort_if_multisig_payload_mismatch_enabled();
+
+ + + ### Function `change_feature_flags_internal` diff --git a/aptos-move/framework/move-stdlib/sources/configs/features.move b/aptos-move/framework/move-stdlib/sources/configs/features.move index 986634e735d1c..a1e55fc0653d0 100644 --- a/aptos-move/framework/move-stdlib/sources/configs/features.move +++ b/aptos-move/framework/move-stdlib/sources/configs/features.move @@ -570,6 +570,18 @@ module std::features { is_enabled(DEFAULT_TO_CONCURRENT_FUNGIBLE_BALANCE) } + /// Whether the multisig v2 fix is enabled. Once enabled, the multisig transaction execution will explicitly + /// abort if the provided payload does not match the payload stored on-chain. + /// + /// Lifetime: transient + const ABORT_IF_MULTISIG_PAYLOAD_MISMATCH: u64 = 70; + + public fun get_abort_if_multisig_payload_mismatch_feature(): u64 { ABORT_IF_MULTISIG_PAYLOAD_MISMATCH } + + public fun abort_if_multisig_payload_mismatch_enabled(): bool acquires Features { + is_enabled(ABORT_IF_MULTISIG_PAYLOAD_MISMATCH) + } + // ============================================================================================ // Feature Flag Implementation diff --git a/aptos-move/framework/move-stdlib/sources/configs/features.spec.move b/aptos-move/framework/move-stdlib/sources/configs/features.spec.move index 20d8890694551..2823108154016 100644 --- a/aptos-move/framework/move-stdlib/sources/configs/features.spec.move +++ b/aptos-move/framework/move-stdlib/sources/configs/features.spec.move @@ -96,6 +96,16 @@ spec std::features { ensures [abstract] result == spec_module_event_enabled(); } + spec fun spec_abort_if_multisig_payload_mismatch_enabled(): bool { + spec_is_enabled(ABORT_IF_MULTISIG_PAYLOAD_MISMATCH) + } + + spec abort_if_multisig_payload_mismatch_enabled { + pragma opaque; + aborts_if [abstract] false; + ensures [abstract] result == spec_abort_if_multisig_payload_mismatch_enabled(); + } + spec on_new_epoch(framework: &signer) { requires @std == signer::address_of(framework); let features_pending = global(@std).features; diff --git a/third_party/move/move-core/types/src/vm_status.rs b/third_party/move/move-core/types/src/vm_status.rs index 965883162eb1e..7abbe6461bcdf 100644 --- a/third_party/move/move-core/types/src/vm_status.rs +++ b/third_party/move/move-core/types/src/vm_status.rs @@ -586,11 +586,12 @@ pub enum StatusCode { INSUFFICIENT_BALANCE_FOR_REQUIRED_DEPOSIT = 37, GAS_PARAMS_MISSING = 38, REQUIRED_DEPOSIT_INCONSISTENT_WITH_TXN_MAX_GAS = 39, + MULTISIG_TRANSACTION_PAYLOAD_DOES_NOT_MATCH = 40, // Reserved error code for future use - RESERVED_VALIDATION_ERROR_5 = 40, RESERVED_VALIDATION_ERROR_6 = 41, RESERVED_VALIDATION_ERROR_7 = 42, RESERVED_VALIDATION_ERROR_8 = 43, + RESERVED_VALIDATION_ERROR_9 = 44, // When a code module/script is published it is verified. These are the // possible errors that can arise from the verification process. diff --git a/types/src/on_chain_config/aptos_features.rs b/types/src/on_chain_config/aptos_features.rs index 35b4b5c06806d..af11392ea6007 100644 --- a/types/src/on_chain_config/aptos_features.rs +++ b/types/src/on_chain_config/aptos_features.rs @@ -85,6 +85,7 @@ pub enum FeatureFlag { CONCURRENT_FUNGIBLE_BALANCE = 67, DEFAULT_TO_CONCURRENT_FUNGIBLE_BALANCE = 68, LIMIT_VM_TYPE_SIZE = 69, + ABORT_IF_MULTISIG_PAYLOAD_MISMATCH = 70, } impl FeatureFlag { @@ -150,6 +151,7 @@ impl FeatureFlag { FeatureFlag::AGGREGATOR_V2_IS_AT_LEAST_API, FeatureFlag::CONCURRENT_FUNGIBLE_BALANCE, // FeatureFlag::LIMIT_VM_TYPE_SIZE, // TODO: Enable when type builder rolls out + FeatureFlag::ABORT_IF_MULTISIG_PAYLOAD_MISMATCH, ] } } @@ -293,6 +295,10 @@ impl Features { self.is_enabled(FeatureFlag::LIMIT_VM_TYPE_SIZE) } + pub fn is_abort_if_multisig_payload_mismatch_enabled(&self) -> bool { + self.is_enabled(FeatureFlag::ABORT_IF_MULTISIG_PAYLOAD_MISMATCH) + } + pub fn get_max_identifier_size(&self) -> u64 { if self.is_enabled(FeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH) { IDENTIFIER_SIZE_MAX