diff --git a/CHANGELOG.md b/CHANGELOG.md index 461301d3aa..92d5c8d692 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## v0.16.0 (TBD) ### Changes +- Added a default-threshold floor for output notes in `AuthMultisig` and the smart multisig, so a low per-procedure override cannot authorize tx-script-created output notes (which are not tracked by `was_procedure_called`) below `default_threshold` ([#3220](https://github.com/0xMiden/protocol/pull/3220)). - Split `account_id::validate` into `account_id::validate_structure` (version-independent structural checks) and `account_id::validate` (structure and the version check) ([#3188](https://github.com/0xMiden/protocol/pull/3188)). - Changed the default `LocalTransactionProver` hash function from `BLAKE3` to `Poseidon2`, added ECDSA variants for every signature-authenticated transaction benchmark, and restructured the time counting benchmark IDs to encode the signing scheme and proving hash function (e.g. `poseidon2/falcon/single-p2id-note`) ([#3152](https://github.com/0xMiden/protocol/pull/3152)). - [BREAKING] Renamed `AssetId` to `AssetClass`, the identifier that distinguishes assets within a faucet ([#3079](https://github.com/0xMiden/protocol/issues/3079)). diff --git a/crates/miden-standards/asm/standards/auth/multisig.masm b/crates/miden-standards/asm/standards/auth/multisig.masm index 5b7b457575..d704a485da 100644 --- a/crates/miden-standards/asm/standards/auth/multisig.masm +++ b/crates/miden-standards/asm/standards/auth/multisig.masm @@ -5,6 +5,7 @@ use miden::protocol::active_account use {AUTH_UNAUTHORIZED_EVENT} from miden::protocol::auth use miden::protocol::native_account +use miden::protocol::tx use miden::standards::auth use miden::standards::auth::signature use miden::core::word @@ -446,6 +447,15 @@ proc compute_transaction_threshold(default_threshold: u32) -> u32 drop # => [transaction_threshold] + # Output notes are not attributed to any tracked native procedure, so a + # low per-procedure override must not authorize them below default_threshold. + exec.tx::get_num_output_notes neq.0 + # => [has_output_notes, transaction_threshold] + if.true + loc_load.DEFAULT_THRESHOLD_LOC u32max + end + # => [transaction_threshold] + loc_load.DEFAULT_THRESHOLD_LOC # => [default_threshold, transaction_threshold] diff --git a/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm b/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm index 2e43d721d9..26c72d36f1 100644 --- a/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm +++ b/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm @@ -1,5 +1,6 @@ use miden::protocol::active_account use miden::protocol::native_account +use miden::protocol::tx use {AUTH_UNAUTHORIZED_EVENT} from miden::protocol::auth use miden::standards::auth use miden::standards::auth::multisig @@ -177,12 +178,26 @@ end #! - transaction_threshold is the effective minimum number of signatures required. #! #! Invocation: exec +@locals(1) proc compute_tx_threshold(default_threshold: u32, policy_threshold: u32) -> u32 + # save default_threshold for the output-note floor below + dup loc_store.0 + # => [default_threshold, policy_threshold] + dup.1 eq.0 # => [is_policy_zero, default_threshold, policy_threshold] cdrop # => [effective_transaction_threshold] + + # Output notes are not attributed to any tracked native procedure, so a + # low per-procedure policy must not authorize them below default_threshold. + exec.tx::get_num_output_notes neq.0 + # => [has_output_notes, effective_transaction_threshold] + if.true + loc_load.0 u32max + end + # => [effective_transaction_threshold] end #! Computes a single procedure's contribution to the threshold max-accumulator and the diff --git a/crates/miden-testing/tests/auth/multisig.rs b/crates/miden-testing/tests/auth/multisig.rs index 9c4a9afb18..f46b1ccb30 100644 --- a/crates/miden-testing/tests/auth/multisig.rs +++ b/crates/miden-testing/tests/auth/multisig.rs @@ -10,11 +10,19 @@ use miden_protocol::account::{ StorageMapKey, }; use miden_protocol::asset::{AssetId, FungibleAsset}; -use miden_protocol::note::{Note, NoteType}; +use miden_protocol::note::{ + Note, + NoteAssets, + NoteRecipient, + NoteStorage, + NoteType, + PartialNoteMetadata, +}; use miden_protocol::testing::account_id::{ ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET, ACCOUNT_ID_REGULAR_PUBLIC_ACCOUNT_UPDATABLE_CODE, }; +use miden_protocol::testing::note::DEFAULT_NOTE_SCRIPT; use miden_protocol::transaction::{RawOutputNote, TransactionScript}; use miden_protocol::vm::AdviceMap; use miden_protocol::{Felt, Hasher, Word}; @@ -1232,6 +1240,122 @@ async fn test_multisig_proc_threshold_overrides( Ok(()) } +/// Regression test for the audit finding "Per-Procedure Threshold Overrides Can Reduce Required +/// Signatures for Untracked Transaction Effects". +/// +/// A low per-procedure override (`receive_asset` = 1) must not lower the required signatures for +/// output notes created directly by the transaction script. Such notes are not attributed to any +/// tracked native procedure (`was_procedure_called` never fires for them), so they must stay bound +/// to the default threshold. Consuming a P2ID note (tracked `receive_asset`, override 1) while the +/// tx script also creates an output note must therefore require the default threshold (2), not 1. +#[rstest] +#[case::ecdsa(AuthScheme::EcdsaK256Keccak)] +#[case::falcon(AuthScheme::Falcon512Poseidon2)] +#[tokio::test] +async fn test_multisig_proc_threshold_override_does_not_lower_untracked_output_notes( + #[case] auth_scheme: AuthScheme, +) -> anyhow::Result<()> { + let (_secret_keys, auth_schemes, public_keys, authenticators) = + setup_keys_and_authenticators_with_scheme(2, 2, auth_scheme)?; + + // receive_asset is overridden to require only 1 signature. + let proc_threshold_map = vec![(BasicWallet::receive_asset_root(), 1)]; + + let approvers = public_keys + .iter() + .zip(auth_schemes.iter()) + .map(|(pk, scheme)| (pk.clone(), *scheme)) + .collect::>(); + + let multisig_account = create_multisig_account(2, &approvers, 10, proc_threshold_map)?; + + let mut mock_chain_builder = + MockChainBuilder::with_accounts([multisig_account.clone()]).unwrap(); + + // P2ID note consumed by the multisig: triggers the tracked `receive_asset` (override 1). + let input_note = mock_chain_builder.add_p2id_note( + multisig_account.id(), + multisig_account.id(), + &[FungibleAsset::mock(1)], + NoteType::Public, + )?; + + let mock_chain = mock_chain_builder.build()?; + + // Assetless output note created directly by the tx script via `output_note::create`, which is + // NOT tracked by `was_procedure_called`. It carries no assets, so it stays balance-neutral and + // needs no (tracked) `move_asset_to_note`. + let note_script = CodeBuilder::default().compile_note_script(DEFAULT_NOTE_SCRIPT)?; + let output_note = Note::new( + NoteAssets::new(vec![])?, + PartialNoteMetadata::new(multisig_account.id(), NoteType::Public), + NoteRecipient::new( + Word::from([1_u32, 2_u32, 3_u32, 4_u32]), + note_script.clone(), + NoteStorage::default(), + ), + ); + + let create_output_note_script = CodeBuilder::default().compile_tx_script(format!( + "use miden::protocol::output_note\n@transaction_script\npub proc main\n push.{recipient}\n push.{note_type}\n push.{tag}\n exec.output_note::create\n drop\nend", + recipient = output_note.recipient().digest(), + note_type = NoteType::Public as u8, + tag = Felt::from(output_note.metadata().tag()), + ))?; + + let salt = Word::from([Felt::new_unchecked(9); 4]); + let tx_context_builder = mock_chain + .build_tx_context(multisig_account.id(), &[input_note.id()], &[])? + .tx_script(create_output_note_script) + .add_note_script(note_script) + .extend_expected_output_notes(vec![RawOutputNote::Full(output_note)]) + .auth_args(salt); + + // Execute unsigned to obtain the tx summary to sign. + let tx_summary = tx_context_builder + .clone() + .build()? + .execute() + .await + .unwrap_err() + .unwrap_unauthorized_err(); + let msg = tx_summary.as_ref().to_commitment(); + let tx_summary_signing = SigningInputs::TransactionSummary(tx_summary); + + // 1 signature must FAIL: the untracked output note keeps the threshold at the default (2), + // even though the only tracked procedure (`receive_asset`) is overridden to 1. + let sig_1 = authenticators[0] + .get_signature(public_keys[0].to_commitment(), &tx_summary_signing) + .await?; + let one_sig_result = tx_context_builder + .clone() + .add_signature(public_keys[0].to_commitment(), msg, sig_1) + .build()? + .execute() + .await; + one_sig_result.unwrap_err().unwrap_unauthorized_err(); + + // 2 signatures (the default threshold) must SUCCEED. + let sig_1 = authenticators[0] + .get_signature(public_keys[0].to_commitment(), &tx_summary_signing) + .await?; + let sig_2 = authenticators[1] + .get_signature(public_keys[1].to_commitment(), &tx_summary_signing) + .await?; + let two_sig_result = tx_context_builder + .add_signature(public_keys[0].to_commitment(), msg, sig_1) + .add_signature(public_keys[1].to_commitment(), msg, sig_2) + .build()? + .execute() + .await; + assert!( + two_sig_result.is_ok(), + "creating an output note must require the default threshold, not the receive_asset override" + ); + + Ok(()) +} + /// Tests setting a per-procedure threshold override and clearing it via `proc_threshold == 0`. #[rstest] #[case::ecdsa(AuthScheme::EcdsaK256Keccak)]