diff --git a/CHANGELOG.md b/CHANGELOG.md index 463e16ffda..14259d0bb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,7 @@ - [BREAKING] Moved asset callback flag from asset vault key to account ID, making it immutable ([#3167](https://github.com/0xMiden/protocol/pull/3167)). - [BREAKING] Added `@account_procedure` attribute to mark which procedures should be included in the account component interface ([#3171](https://github.com/0xMiden/protocol/pull/3171)). - Documented that the `ecdsa_k256_keccak` authentication scheme discloses the signer's public key and signature at proving time via precompile calldata ([#3178](https://github.com/0xMiden/protocol/pull/3178)). +- Optimized the multisig auth component MASM (unconditional loop entry where the counter is guaranteed non-zero, single-step `scheme_id` extraction in `get_signer_at`, and a stack read instead of a local reload in `update_signers_and_threshold`), and documented that growing the signer set does not re-scale existing per-procedure threshold overrides ([#3211](https://github.com/0xMiden/protocol/pull/3211)). - Renamed the `Authority` config value slot, expressed the authority kind as a MASM `enum Authority : u8`, and enforced the canonical config-word encoding on read ([#3209](https://github.com/0xMiden/protocol/pull/3209)). - Unified procedure ordering and document sender-based access control's authentication assumption in the `ownable2step` and `rbac` access control modules ([#3205](https://github.com/0xMiden/protocol/pull/3205)). - [BREAKING] Updated `BlockHeader` to support multiple validator keys and added `ValidatorKeys` and `BlockSignatures` types ([#3174](https://github.com/0xMiden/protocol/pull/3174)). @@ -96,6 +97,7 @@ - [BREAKING] Fixed batch ID being serialized/deserialized and potentially not matching the serialized transaction headers ([#3061](https://github.com/0xMiden/protocol/pull/3061)). - Simplified the `ownable2step` ownership transitions ([#3170](https://github.com/0xMiden/protocol/pull/3170)). - Fixed `note_script_allowlist::assert_all_input_notes_allowed` and `tx_script_allowlist::assert_tx_script_allowed` to read the allowlist from the transaction's initial storage state via `active_account::get_initial_map_item` ([#3182](https://github.com/0xMiden/protocol/pull/3182)). +- Fixed `set_procedure_threshold` now asserts `PROC_ROOT` is one of the account's procedures (`ERR_PROC_ROOT_NOT_IN_ACCOUNT`) before storing an override, and corrected the inaccurate `assert_new_tx`, `update_signers_and_threshold`, and `get_signer_at` stack-layout and advice-map comments ([#3211](https://github.com/0xMiden/protocol/pull/3211)). ## v0.15.2 (2026-06-05) diff --git a/crates/miden-standards/asm/components/auth/guarded_multisig/guarded_multisig.masm b/crates/miden-standards/asm/components/auth/guarded_multisig/guarded_multisig.masm index fd7c125609..6a93bdde61 100644 --- a/crates/miden-standards/asm/components/auth/guarded_multisig/guarded_multisig.masm +++ b/crates/miden-standards/asm/components/auth/guarded_multisig/guarded_multisig.masm @@ -32,6 +32,6 @@ pub proc auth_tx_guarded_multisig(salt: word) exec.guardian::verify_signature # => [TX_SUMMARY_COMMITMENT] - exec.multisig::assert_new_tx + exec.multisig::record_and_assert_new_tx # => [] end diff --git a/crates/miden-standards/asm/components/auth/multisig/multisig.masm b/crates/miden-standards/asm/components/auth/multisig/multisig.masm index 00c14866a1..2cef169ff3 100644 --- a/crates/miden-standards/asm/components/auth/multisig/multisig.masm +++ b/crates/miden-standards/asm/components/auth/multisig/multisig.masm @@ -23,6 +23,6 @@ pub proc auth_tx_multisig(salt: word) exec.multisig::auth_tx # => [TX_SUMMARY_COMMITMENT] - exec.multisig::assert_new_tx + exec.multisig::record_and_assert_new_tx # => [] end diff --git a/crates/miden-standards/asm/components/auth/multisig_smart/multisig_smart.masm b/crates/miden-standards/asm/components/auth/multisig_smart/multisig_smart.masm index bcfce77a96..90d2bbd28d 100644 --- a/crates/miden-standards/asm/components/auth/multisig_smart/multisig_smart.masm +++ b/crates/miden-standards/asm/components/auth/multisig_smart/multisig_smart.masm @@ -24,6 +24,6 @@ pub proc auth_tx_multisig_smart(salt: word) exec.multisig_smart::auth_tx # => [TX_SUMMARY_COMMITMENT] - exec.multisig::assert_new_tx + exec.multisig::record_and_assert_new_tx # => [] end diff --git a/crates/miden-standards/asm/components/auth/network_account/network_account.masm b/crates/miden-standards/asm/components/auth/network_account/network_account.masm index 0237e8996e..4bdcdae78b 100644 --- a/crates/miden-standards/asm/components/auth/network_account/network_account.masm +++ b/crates/miden-standards/asm/components/auth/network_account/network_account.masm @@ -33,7 +33,7 @@ const ALLOWED_TX_SCRIPTS_SLOT = word("miden::standards::auth::network_account::a #! If both checks pass, the nonce is incremented when the account state changed or the account is #! new, matching the behavior of the NoAuth and SingleSig components. #! -#! Inputs: [pad(16)] +#! Inputs: [AUTH_ARGS, pad(12)] #! Outputs: [pad(16)] #! #! Invocation: call diff --git a/crates/miden-standards/asm/components/auth/no_auth/no_auth.masm b/crates/miden-standards/asm/components/auth/no_auth/no_auth.masm index d0b2c86f26..f5c6bd6681 100644 --- a/crates/miden-standards/asm/components/auth/no_auth/no_auth.masm +++ b/crates/miden-standards/asm/components/auth/no_auth/no_auth.masm @@ -10,8 +10,10 @@ use miden::core::word #! This avoids unnecessary nonce increments for transactions that don't modify #! the account state. #! -#! Inputs: [pad(16)] +#! Inputs: [AUTH_ARGS, pad(12)] #! Outputs: [pad(16)] +#! +#! Invocation: call @auth_script pub proc auth_no_auth # check if the account state has changed by comparing initial and final commitments diff --git a/crates/miden-standards/asm/components/auth/singlesig/singlesig.masm b/crates/miden-standards/asm/components/auth/singlesig/singlesig.masm index 5065a06b7e..774ab6accc 100644 --- a/crates/miden-standards/asm/components/auth/singlesig/singlesig.masm +++ b/crates/miden-standards/asm/components/auth/singlesig/singlesig.masm @@ -42,13 +42,13 @@ pub proc auth_tx(auth_args: word) # --------------------------------------------------------------------------------------------- push.PUBLIC_KEY_SLOT[0..2] exec.active_account::get_initial_item - # => [PUB_KEY, pad(16)] + # => [PK_COMM, pad(16)] push.SCHEME_ID_SLOT[0..2] exec.active_account::get_initial_item - # => [scheme_id, 0, 0, 0, PUB_KEY, pad(16)] + # => [scheme_id, 0, 0, 0, PK_COMM, pad(16)] movdn.7 drop drop drop - # => [PUB_KEY, scheme_id, pad(16)] + # => [PK_COMM, scheme_id, pad(16)] exec.signature::authenticate_transaction # => [pad(16)] diff --git a/crates/miden-standards/asm/components/auth/singlesig_acl/singlesig_acl.masm b/crates/miden-standards/asm/components/auth/singlesig_acl/singlesig_acl.masm index 3f107c1db5..87e97fb033 100644 --- a/crates/miden-standards/asm/components/auth/singlesig_acl/singlesig_acl.masm +++ b/crates/miden-standards/asm/components/auth/singlesig_acl/singlesig_acl.masm @@ -108,14 +108,14 @@ pub proc auth_tx_acl(auth_args: word) if.true # Fetch public key from storage. push.PUBLIC_KEY_SLOT[0..2] exec.active_account::get_initial_item - # => [PUB_KEY, pad(16)] + # => [PK_COMM, pad(16)] # Fetch scheme_id from storage push.SCHEME_ID_SLOT[0..2] exec.active_account::get_initial_item - # => [[scheme_id, 0, 0, 0], PUB_KEY, pad(16)] + # => [[scheme_id, 0, 0, 0], PK_COMM, pad(16)] movdn.7 drop drop drop - # => [PUB_KEY, scheme_id, pad(16)] + # => [PK_COMM, scheme_id, pad(16)] exec.signature::authenticate_transaction else diff --git a/crates/miden-standards/asm/standards/auth/guardian.masm b/crates/miden-standards/asm/standards/auth/guardian.masm index 4fff6934d2..222f1312ab 100644 --- a/crates/miden-standards/asm/standards/auth/guardian.masm +++ b/crates/miden-standards/asm/standards/auth/guardian.masm @@ -77,7 +77,7 @@ pub proc update_guardian_public_key(new_guardian_scheme_id: felt, new_guardian_p # => [GUARDIAN_MAP_KEY, NEW_GUARDIAN_PUBLIC_KEY] push.GUARDIAN_PUBLIC_KEYS_SLOT[0..2] - # => [guardian_pubkeys_slot_prefix, guardian_pubkeys_slot_suffix, + # => [guardian_pubkeys_slot_suffix, guardian_pubkeys_slot_prefix, # GUARDIAN_MAP_KEY, NEW_GUARDIAN_PUBLIC_KEY] exec.native_account::set_map_item @@ -97,7 +97,7 @@ pub proc update_guardian_public_key(new_guardian_scheme_id: felt, new_guardian_p # => [GUARDIAN_MAP_KEY, NEW_GUARDIAN_SCHEME_ID_WORD] push.GUARDIAN_SCHEME_ID_SLOT[0..2] - # => [guardian_scheme_slot_prefix, guardian_scheme_slot_suffix, + # => [guardian_scheme_slot_suffix, guardian_scheme_slot_prefix, # GUARDIAN_MAP_KEY, NEW_GUARDIAN_SCHEME_ID_WORD] exec.native_account::set_map_item @@ -140,11 +140,11 @@ pub proc verify_signature(msg: word) # => [1, MSG] push.GUARDIAN_PUBLIC_KEYS_SLOT[0..2] - # => [guardian_pubkeys_slot_prefix, guardian_pubkeys_slot_suffix, 1, MSG] + # => [guardian_pubkeys_slot_suffix, guardian_pubkeys_slot_prefix, 1, MSG] push.GUARDIAN_SCHEME_ID_SLOT[0..2] - # => [guardian_scheme_slot_prefix, guardian_scheme_slot_suffix, - # guardian_pubkeys_slot_prefix, guardian_pubkeys_slot_suffix, 1, MSG] + # => [guardian_scheme_slot_suffix, guardian_scheme_slot_prefix, + # guardian_pubkeys_slot_suffix, guardian_pubkeys_slot_prefix, 1, MSG] exec.signature::verify_signatures # => [num_verified_signatures, MSG] diff --git a/crates/miden-standards/asm/standards/auth/multisig.masm b/crates/miden-standards/asm/standards/auth/multisig.masm index 5b7b457575..705f7754dc 100644 --- a/crates/miden-standards/asm/standards/auth/multisig.masm +++ b/crates/miden-standards/asm/standards/auth/multisig.masm @@ -85,11 +85,20 @@ const ERR_NUM_APPROVERS_OR_PROC_THRESHOLD_NOT_U32 = "number of approvers and pro const ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS = "procedure threshold exceeds new number of approvers" -#! Remove old approver public keys and the corresponding scheme ids -#! from the approver public key and scheme id mappings. +const ERR_PROC_ROOT_NOT_IN_ACCOUNT = "cannot set procedure threshold for procedure root that is not one of the account's procedures" + +#! Remove old approver public keys and the corresponding scheme ids from the approver public key and +#! scheme id mappings. +#! +#! This procedure clears the approver entries in the range [new_num_of_approvers, +#! init_num_of_approvers) from APPROVER_PUBLIC_KEYS_SLOT and APPROVER_SCHEME_ID_SLOT. It does not +#! touch THRESHOLD_CONFIG_SLOT or PROC_THRESHOLD_ROOTS_SLOT. #! -#! This procedure cleans up the storage by removing public keys and signature scheme ids of approvers -#! that are no longer part of the multisig configuration. +#! Caller contract: before calling this, the caller must have already updated THRESHOLD_CONFIG_SLOT +#! so that the threshold is still reachable with the reduced signer set, i.e. threshold <= +#! new_num_of_approvers. Otherwise the account is left with a threshold that no remaining quorum can +#! reach, making it permanently unable to authenticate. In-tree this is guaranteed by +#! `update_signers_and_threshold`, which calls `assert_threshold_is_reachable` before this procedure. #! #! Inputs: [init_num_of_approvers, new_num_of_approvers] #! Outputs: [] @@ -99,8 +108,7 @@ const ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS = "procedure threshold exceeds ne #! - new_num_of_approvers is the new number of approvers after the update #! #! Panics if: -#! - init_num_of_approvers is not a u32 value. -#! - new_num_of_approvers is not a u32 value. +#! - init_num_of_approvers or new_num_of_approvers is not a u32 value. pub proc cleanup_pubkey_and_scheme_id_mapping(init_num_of_approvers: u32, new_num_of_approvers: u32) dup.1 dup.1 u32assert2.err=ERR_APPROVER_COUNTS_NOT_U32 @@ -153,6 +161,20 @@ pub proc cleanup_pubkey_and_scheme_id_mapping(init_num_of_approvers: u32, new_nu drop drop end +#! Asserts the threshold is reachable with the given number of approvers, i.e. threshold is not +#! greater than num_approvers. +#! +#! Inputs: [num_approvers, threshold] +#! Outputs: [] +#! +#! Panics if: +#! - num_approvers or threshold is not a u32 value. +#! - threshold > num_approvers. +pub proc assert_threshold_is_reachable + u32assert2.err=ERR_MALFORMED_MULTISIG_CONFIG + u32gt assertz.err=ERR_MALFORMED_MULTISIG_CONFIG +end + #! Asserts that all configured per-procedure threshold overrides are less than or equal to #! number of approvers. #! @@ -165,7 +187,8 @@ proc assert_proc_thresholds_lte_num_approvers(num_approvers: u32) exec.active_account::get_num_procedures # => [num_procedures, num_approvers] - dup neq.0 + # num_procedures is always >= MIN_NUM_PROCEDURES (2), so enter the loop unconditionally + push.1 # => [should_continue, num_procedures, num_approvers] while.true sub.1 dup @@ -203,22 +226,32 @@ proc assert_proc_thresholds_lte_num_approvers(num_approvers: u32) end #! Update threshold config, add & remove approvers, and update the approver scheme ids -#! +#! +#! Note: per-procedure threshold overrides in `PROC_THRESHOLD_ROOTS_SLOT` are NOT re-scaled by this +#! procedure. They are absolute signature counts, not ratios, and the only cross-check is +#! `assert_proc_thresholds_lte_num_approvers`, which enforces `override <= num_approvers`. Growing +#! the signer set therefore leaves every override unchanged and silently lowers its effective signing +#! ratio (e.g. a 2-of-2 override becomes 2-of-N). To preserve the intended security level, operators +#! must re-evaluate and, where appropriate, raise the affected overrides via `set_procedure_threshold` +#! in the same transaction that grows the signer set; this procedure neither performs nor prompts it. +#! #! Inputs: #! Operand stack: [MULTISIG_CONFIG_HASH, pad(12)] #! Advice map: { -#! MULTISIG_CONFIG_HASH => -#! [ -#! CONFIG, -#! PUB_KEY_N, PUB_KEY_N-1, ..., PUB_KEY_0, -#! SCHEME_ID_N, SCHEME_ID_N-1, ..., SCHEME_ID_0 +#! MULTISIG_CONFIG_HASH => +#! [ +#! CONFIG, +#! PUB_KEY_N, SCHEME_ID_N, +#! PUB_KEY_N-1, SCHEME_ID_N-1, +#! ..., +#! PUB_KEY_0, SCHEME_ID_0 #! ] #! } #! Outputs: #! Operand stack: [] #! #! Where: -#! - MULTISIG_CONFIG_HASH is the hash of the threshold, +#! - MULTISIG_CONFIG_HASH is the hash of the threshold, #! new public key vector, and the corresponding scheme identifiers #! - MULTISIG_CONFIG is [threshold, num_approvers, 0, 0] #! - PUB_KEY_i is the public key of the i-th signer @@ -232,6 +265,11 @@ end @locals(2) @account_procedure pub proc update_signers_and_threshold(multisig_config_hash: word) + # The signer configuration is read from the advice provider without an inline hash check. Every + # value written below via set_map_item is folded into ACCOUNT_DELTA_COMMITMENT, which is part of + # the TX_SUMMARY_COMMITMENT the current signers sign, so a substituted configuration produces a + # different delta commitment and thus a summary the current signers never signed. The same + # applies to the per-signer loads in the loop below. adv.push_mapval # => [MULTISIG_CONFIG_HASH, pad(12)] @@ -245,9 +283,8 @@ pub proc update_signers_and_threshold(multisig_config_hash: word) dup dup.2 # => [num_approvers, threshold, MULTISIG_CONFIG, pad(12)] - # make sure that the threshold is smaller than the number of approvers - u32assert2.err=ERR_MALFORMED_MULTISIG_CONFIG - u32gt assertz.err=ERR_MALFORMED_MULTISIG_CONFIG + # make sure that the threshold is reachable with the number of approvers + exec.assert_threshold_is_reachable # => [MULTISIG_CONFIG, pad(12)] dup dup.2 @@ -258,7 +295,8 @@ pub proc update_signers_and_threshold(multisig_config_hash: word) eq.0 assertz.err=ERR_ZERO_IN_MULTISIG_CONFIG # => [MULTISIG_CONFIG, pad(12)] - loc_load.NEW_NUM_OF_APPROVERS_LOC + # MULTISIG_CONFIG is [threshold, num_approvers, 0, 0], so num_approvers is at index 1 + dup.1 # => [num_approvers, MULTISIG_CONFIG, pad(12)] # make sure that all existing procedure threshold overrides remain reachable @@ -278,7 +316,8 @@ pub proc update_signers_and_threshold(multisig_config_hash: word) loc_load.NEW_NUM_OF_APPROVERS_LOC # => [num_approvers] - dup neq.0 + # num_approvers was already asserted non-zero above, so enter the loop unconditionally + push.1 while.true sub.1 # => [i-1, pad(12)] @@ -380,7 +419,8 @@ proc compute_transaction_threshold(default_threshold: u32) -> u32 # => [num_procedures, transaction_threshold] # 2. iterate through all account procedures - dup neq.0 + # num_procedures is always >= MIN_NUM_PROCEDURES (2), so enter the loop unconditionally + push.1 # => [should_continue, num_procedures, transaction_threshold] while.true sub.1 @@ -474,12 +514,14 @@ end #! Returns the current threshold and num_approvers from `THRESHOLD_CONFIG_SLOT`. #! +#! This is the internal `exec` helper shared by internal callers and the public `call` +#! wrapper `get_threshold_and_num_approvers`. +#! #! Inputs: [] #! Outputs: [default_threshold, num_approvers] #! -#! Invocation: call -@account_procedure -pub proc get_threshold_and_num_approvers +#! Invocation: exec +proc get_current_threshold_and_num_approvers push.THRESHOLD_CONFIG_SLOT[0..2] exec.active_account::get_item # => [default_threshold, num_approvers, 0, 0] @@ -488,6 +530,22 @@ pub proc get_threshold_and_num_approvers # => [default_threshold, num_approvers] end +#! Returns the current threshold and num_approvers from `THRESHOLD_CONFIG_SLOT`. +#! +#! Inputs: [pad(16)] +#! Outputs: [default_threshold, num_approvers, pad(14)] +#! +#! Invocation: call +@account_procedure +pub proc get_threshold_and_num_approvers + exec.get_current_threshold_and_num_approvers + # => [default_threshold, num_approvers, pad(16)] + + # truncate the stack + movup.2 drop movup.2 drop + # => [default_threshold, num_approvers, pad(14)] +end + #! Sets or clears a per-procedure threshold override. #! #! Inputs: [proc_threshold, PROC_ROOT] @@ -503,11 +561,12 @@ end #! - proc_threshold is not a u32 value. #! - current num_approvers is not a u32 value. #! - proc_threshold > current num_approvers. +#! - PROC_ROOT is not one of the account's procedures. #! #! Invocation: call @account_procedure pub proc set_procedure_threshold - exec.get_threshold_and_num_approvers + exec.get_current_threshold_and_num_approvers # => [default_threshold, num_approvers, proc_threshold, PROC_ROOT] drop @@ -520,6 +579,16 @@ pub proc set_procedure_threshold u32gt assertz.err=ERR_PROC_THRESHOLD_EXCEEDS_NUM_APPROVERS # => [proc_threshold, PROC_ROOT] + movdn.4 dupw + # => [PROC_ROOT, PROC_ROOT, proc_threshold] + + exec.active_account::has_procedure + assert.err=ERR_PROC_ROOT_NOT_IN_ACCOUNT + # => [PROC_ROOT, proc_threshold] + + movup.4 + # => [proc_threshold, PROC_ROOT] + # Store [proc_threshold, 0, 0, 0] = PROC_THRESHOLD_WORD, where proc_threshold == 0 acts as clear. push.0.0.0 movup.3 @@ -538,8 +607,8 @@ end #! Returns signer public key at index i #! -#! Inputs: [index] -#! Outputs: [PUB_KEY, scheme_id] +#! Inputs: [index, pad(15)] +#! Outputs: [PUB_KEY, scheme_id, pad(11)] #! #! Panics if: #! - index is not a u32 value. @@ -557,7 +626,7 @@ pub proc get_signer_at # => [APPROVER_MAP_KEY, index] push.APPROVER_PUBLIC_KEYS_SLOT[0..2] - # => [APPROVER_PUBLIC_KEYS_SLOT, APPROVER_MAP_KEY, index] + # => [pub_key_slot_suffix, pub_key_slot_prefix, APPROVER_MAP_KEY, index] exec.active_account::get_initial_map_item # => [PUB_KEY, index] @@ -569,23 +638,24 @@ pub proc get_signer_at # => [APPROVER_MAP_KEY, PUB_KEY] push.APPROVER_SCHEME_ID_SLOT[0..2] - # => [APPROVER_SCHEME_ID_SLOT, APPROVER_MAP_KEY, PUB_KEY] + # => [scheme_id_slot_suffix, scheme_id_slot_prefix, APPROVER_MAP_KEY, PUB_KEY] exec.active_account::get_initial_map_item - # => [SCHEME_ID_WORD, PUB_KEY] + # => [[scheme_id, 0, 0, 0], PUB_KEY] - movdn.3 drop drop drop - # => [scheme_id, PUB_KEY] + movdn.7 drop drop drop + # => [PUB_KEY, scheme_id, pad(15)] - movdn.4 - # => [PUB_KEY, scheme_id] + # truncate the stack + repeat.4 movup.5 drop end + # => [PUB_KEY, scheme_id, pad(11)] end #! Returns 1 if PUB_KEY is a current signer, else 0. #! -#! Inputs: [PUB_KEY] -#! Outputs: [is_signer] +#! Inputs: [PUB_KEY, pad(12)] +#! Outputs: [is_signer, pad(15)] #! Locals: #! 0: is_signer_found #! 1: current_signer_index @@ -646,14 +716,21 @@ pub proc is_signer(pub_key: word) -> felt end drop dropw - # => [] + # => [pad(16)] loc_load.IS_SIGNER_FOUND_LOC - # => [is_signer] + # => [is_signer, pad(16)] + + # truncate the stack + swap drop + # => [is_signer, pad(15)] end -#! Check if transaction has already been executed and add it to executed transactions for replay protection, and -#! finalizes multisig authentication. +#! Records the transaction as executed and asserts it was not already executed, providing replay +#! protection and finalizing multisig authentication. +#! +#! Writes IS_EXECUTED_FLAG into EXECUTED_TXS_SLOT keyed by MSG via set_map_item, then asserts the +#! slot did not already hold the flag. #! #! Inputs: [MSG] #! Outputs: [] @@ -662,9 +739,9 @@ end #! - the same transaction has already been executed #! #! Invocation: exec -pub proc assert_new_tx(msg: word) +pub proc record_and_assert_new_tx(msg: word) push.IS_EXECUTED_FLAG - # => [[0, 0, 0, is_executed], MSG] + # => [[is_executed, 0, 0, 0], MSG] swapw # => [TX_SUMMARY_COMMITMENT, IS_EXECUTED_FLAG] @@ -674,7 +751,7 @@ pub proc assert_new_tx(msg: word) # Set the key value pair in the map to mark transaction as executed exec.native_account::set_map_item - # => [[0, 0, 0, is_executed]] + # => [[is_executed, 0, 0, 0]] movdn.3 drop drop drop # => [is_executed] @@ -706,9 +783,11 @@ end #! Operand stack: [TX_SUMMARY_COMMITMENT] #! #! Where: -#! - SALT is a cryptographically random nonce that enables multiple concurrent -#! multisig transactions while maintaining replay protection. Each transaction -#! must use a unique SALT value to ensure transaction uniqueness. +#! - SALT is a cryptographically random nonce folded into the signed message, and thus into the +#! TX_SUMMARY_COMMITMENT, so that otherwise-identical transactions produce distinct commitments and +#! can run concurrently. That commitment's uniqueness is what `record_and_assert_new_tx` records and checks to +#! enforce replay protection; `auth_tx` itself records nothing, so a wrapper must call +#! `record_and_assert_new_tx` for replay protection to hold. #! - SIG_i is the signature from the i-th signer. #! - MSG is the transaction message being signed. #! - h(SIG_i, MSG) is the hash of the signature and message used as the advice map key. @@ -717,7 +796,6 @@ end #! - insufficient number of valid signatures (below threshold). #! #! Invocation: call -@locals(1) pub proc auth_tx(salt: word) exec.native_account::incr_nonce drop # => [SALT] 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..6bd03b70b2 100644 --- a/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm +++ b/crates/miden-standards/asm/standards/auth/multisig_smart/mod.masm @@ -41,8 +41,6 @@ const NOTE_RESTRICTION_MAX = 3 # ERRORS # ================================================================================================= -const ERR_MALFORMED_MULTISIG_CONFIG = "number of approvers must be equal to or greater than threshold" - const ERR_ZERO_IN_MULTISIG_CONFIG = "number of approvers or threshold must not be zero" const ERR_PROC_POLICY_INVALID_MODE = "called procedures do not support the selected execution mode" @@ -623,6 +621,13 @@ end #! validating smart procedure policies ([`assert_proc_policies_lte_num_approvers`]) instead #! of per-procedure threshold overrides. #! +#! Note: like [`multisig::update_signers_and_threshold`], this procedure does NOT re-scale existing +#! smart procedure policies. Their immediate/delayed thresholds are absolute signature counts, not +#! ratios, and `assert_proc_policies_lte_num_approvers` only enforces the reachability bound +#! (`threshold <= num_approvers`). Growing the signer set silently lowers the effective signing ratio +#! of every policy; operators must re-evaluate and, where appropriate, raise the affected policies in +#! the same transaction that grows the signer set. +#! #! Inputs: #! Operand stack: [MULTISIG_CONFIG_COMMITMENT, pad(12)] #! Outputs: @@ -642,6 +647,11 @@ end @locals(2) @account_procedure pub proc update_signers_and_threshold(multisig_config_commitment: word) + # The signer configuration is read from the advice provider without an inline hash check. Every + # value written below via set_item / set_map_item is folded into ACCOUNT_DELTA_COMMITMENT, which + # is part of the TX_SUMMARY_COMMITMENT the current signers sign, so a substituted configuration + # produces a different delta commitment and thus a summary the current signers never signed. The + # same applies to the per-signer loads in the loop below. adv.push_mapval # => [MULTISIG_CONFIG_COMMITMENT, pad(12)] @@ -655,8 +665,8 @@ pub proc update_signers_and_threshold(multisig_config_commitment: word) dup dup.2 # => [num_approvers, threshold, MULTISIG_CONFIG, pad(12)] - u32assert2.err=ERR_MALFORMED_MULTISIG_CONFIG - u32gt assertz.err=ERR_MALFORMED_MULTISIG_CONFIG + # make sure that the threshold is reachable with the number of approvers + exec.multisig::assert_threshold_is_reachable # => [MULTISIG_CONFIG, pad(12)] dup dup.2 diff --git a/crates/miden-standards/asm/standards/auth/signature.masm b/crates/miden-standards/asm/standards/auth/signature.masm index f6bd82636f..9b1daa5e22 100644 --- a/crates/miden-standards/asm/standards/auth/signature.masm +++ b/crates/miden-standards/asm/standards/auth/signature.masm @@ -44,7 +44,7 @@ const ERR_INVALID_SCHEME_ID_WORD = "invalid scheme ID word format expected three #! included to commit to the transaction creator's intended reference block of the transaction #! which determines the fee parameters and therefore the fee amount that is deducted. #! -#! Inputs: [PUB_KEY, scheme_id] +#! Inputs: [PK_COMM, scheme_id] #! Outputs: [] #! #! Invocation: exec @@ -55,20 +55,20 @@ pub proc authenticate_transaction exec.native_account::incr_nonce exec.tx::get_block_number push.0.0 - # => [[0, 0, ref_block_num, final_nonce], PUB_KEY, scheme_id] + # => [[0, 0, ref_block_num, final_nonce], PK_COMM, scheme_id] # Compute the message that is signed. # --------------------------------------------------------------------------------------------- exec.auth::create_tx_summary - # => [ACCOUNT_DELTA_COMMITMENT, INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, SALT, PUB_KEY, scheme_id] + # => [ACCOUNT_DELTA_COMMITMENT, INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, SALT, PK_COMM, scheme_id] # insert tx summary into advice provider for extraction by the host adv.insert_hqword - # => [ACCOUNT_DELTA_COMMITMENT, INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, SALT, PUB_KEY, scheme_id] + # => [ACCOUNT_DELTA_COMMITMENT, INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, SALT, PK_COMM, scheme_id] # The commitment to the tx summary is the message that is signed exec.auth::hash_tx_summary - # OS => [MESSAGE, PUB_KEY, scheme_id] + # OS => [MESSAGE, PK_COMM, scheme_id] # AS => [] # Fetch signature from advice provider and verify. @@ -76,15 +76,15 @@ pub proc authenticate_transaction # Emit the authentication request event that pushes a signature for the message to the advice stack emit.AUTH_REQUEST_EVENT swapw - # OS => [PUB_KEY, MESSAGE, scheme_id] + # OS => [PK_COMM, MESSAGE, scheme_id] # AS => [SIGNATURE] movup.8 - # OS => [scheme_id, PUB_KEY, MESSAGE] + # OS => [scheme_id, PK_COMM, MESSAGE] # AS => [SIGNATURE] dup.0 exec.assert_supported_scheme - # OS => [scheme_id, PUB_KEY, MESSAGE] + # OS => [scheme_id, PK_COMM, MESSAGE] # AS => [SIGNATURE] # Verify the signature against the public key and the message. The procedure gets as inputs the @@ -99,26 +99,26 @@ end # 1 => ECDSA (ecdsa_k256_keccak) # 2 => Falcon (falcon512_poseidon2) # -# Inputs: [scheme_id, PUB_KEY, MSG] +# Inputs: [scheme_id, PK_COMM, MSG] # Outputs: [] proc verify_signature_by_scheme dup eq.ECDSA_K256_KECCAK_SCHEME_ID - # => [is_one, scheme_id PUB_KEY, MESSAGE] + # => [is_one, scheme_id, PK_COMM, MESSAGE] if.true drop - # OS => [PUB_KEY, MESSAGE] + # OS => [PK_COMM, MESSAGE] exec.ecdsa_k256_keccak::verify # OS => [] # AS => [] else dup eq.FALCON_512_POSEIDON2_SCHEME_ID - # => [is_2, scheme_id, PUB_KEY, MESSAGE] + # => [is_2, scheme_id, PK_COMM, MESSAGE] if.true drop - # OS => [PUB_KEY, MESSAGE] + # OS => [PK_COMM, MESSAGE] exec.falcon512_poseidon2::verify # OS => [] @@ -222,19 +222,19 @@ pub proc verify_signatures loc_load.APPROVER_PUB_KEY_SLOT_ID_PREFIX_LOC loc_load.APPROVER_PUB_KEY_SLOT_ID_SUFFIX_LOC exec.active_account::get_initial_map_item - # => [OWNER_PUB_KEY, i-1, MSG] + # => [PK_COMM, i-1, MSG] loc_storew_le.CURRENT_PK_LOC - # => [OWNER_PUB_KEY, i-1, MSG] + # => [PK_COMM, i-1, MSG] # Check if signature exists for this signer. # ----------------------------------------------------------------------------------------- movup.4 movdn.8 - # => [OWNER_PUB_KEY, MSG, i-1] + # => [PK_COMM, MSG, i-1] dupw.1 swapw - # => [OWNER_PUB_KEY, MSG, MSG, i-1] + # => [PK_COMM, MSG, MSG, i-1] exec.poseidon2::merge # => [SIG_KEY, MSG, i-1] @@ -259,30 +259,30 @@ pub proc verify_signatures # ----------------------------------------------------------------------------------------- loc_loadw_le.CURRENT_PK_LOC - # => [PK, MSG, MSG, i-1] + # => [PK_COMM, MSG, MSG, i-1] swapw - # => [MSG, PK, MSG, i-1] + # => [MSG, PK_COMM, MSG, i-1] # Emit the authentication request event that pushes a signature for the message to the advice stack. emit.AUTH_REQUEST_EVENT swapw - # => [PUB_KEY, MSG, MSG, i-1] + # => [PK_COMM, MSG, MSG, i-1] # Build map key from the current signer index. loc_load.SIGNER_INDEX_LOC exec.create_approver_map_key - # => [APPROVER_MAP_KEY, PUB_KEY, MSG, MSG, i-1] + # => [APPROVER_MAP_KEY, PK_COMM, MSG, MSG, i-1] loc_load.APPROVER_SCHEME_ID_SLOT_ID_PREFIX_LOC loc_load.APPROVER_SCHEME_ID_SLOT_ID_SUFFIX_LOC - # => [scheme_slot_id_suffix, scheme_slot_id_prefix, APPROVER_MAP_KEY, PUB_KEY, MSG, MSG, i-1] + # => [scheme_slot_id_suffix, scheme_slot_id_prefix, APPROVER_MAP_KEY, PK_COMM, MSG, MSG, i-1] # Get scheme_id for signer index i-1 from initial storage state. exec.active_account::get_initial_map_item - # => [[scheme_id, 0, 0, 0], PUB_KEY, MSG, MSG, i-1] + # => [[scheme_id, 0, 0, 0], PK_COMM, MSG, MSG, i-1] movdn.3 drop drop drop - # OS => [scheme_id, PUB_KEY, MSG, MSG, i-1] + # OS => [scheme_id, PK_COMM, MSG, MSG, i-1] # AS => [SIGNATURE] exec.verify_signature_by_scheme diff --git a/crates/miden-standards/asm/standards/auth/tx_policy.masm b/crates/miden-standards/asm/standards/auth/tx_policy.masm index 4201e44a86..96b1c9136b 100644 --- a/crates/miden-standards/asm/standards/auth/tx_policy.masm +++ b/crates/miden-standards/asm/standards/auth/tx_policy.masm @@ -25,14 +25,13 @@ pub proc assert_only_one_non_auth_procedure_called # => [should_continue, num_procedures] while.true sub.1 dup - exec.active_account::get_procedure_root dupw - # => [PROC_ROOT, PROC_ROOT] + exec.active_account::get_procedure_root + # => [PROC_ROOT, proc_index] exec.native_account::was_procedure_called - # => [was_called, PROC_ROOT] + # => [was_called, proc_index] if.true - dropw # => [proc_index] # The auth procedure is always at procedure index 0. @@ -43,9 +42,6 @@ pub proc assert_only_one_non_auth_procedure_called loc_load.0 add.1 loc_store.0 # => [proc_index] end - else - dropw - # => [proc_index] end dup neq.0 diff --git a/crates/miden-standards/src/account/auth/multisig.rs b/crates/miden-standards/src/account/auth/multisig.rs index 914d92acec..ae4fe78972 100644 --- a/crates/miden-standards/src/account/auth/multisig.rs +++ b/crates/miden-standards/src/account/auth/multisig.rs @@ -147,6 +147,16 @@ impl AuthMultisigConfig { /// bound: on private accounts it rejects per-procedure thresholds below the default. /// /// [`create_multisig_wallet`]: crate::account::wallets::create_multisig_wallet +/// +/// # Security: growing the signer set does not re-scale overrides +/// +/// Per-procedure threshold overrides are absolute signature counts, not ratios. Updating the signer +/// set (via the `update_signers_and_threshold` account procedure) does not re-scale existing +/// overrides: the only cross-check is that each override stays `<= num_approvers`, which keeps it +/// reachable but never raises it. Growing the approver set therefore silently lowers the effective +/// signing ratio of every override (e.g. a `2`-of-`2` override becomes `2`-of-`n`). To preserve the +/// intended security level, re-evaluate the affected overrides and, where appropriate, raise them +/// via `set_procedure_threshold` in the same transaction that grows the signer set. #[derive(Debug)] pub struct AuthMultisig { config: AuthMultisigConfig, diff --git a/crates/miden-testing/tests/auth/multisig.rs b/crates/miden-testing/tests/auth/multisig.rs index 9c4a9afb18..045cf42a37 100644 --- a/crates/miden-testing/tests/auth/multisig.rs +++ b/crates/miden-testing/tests/auth/multisig.rs @@ -1561,3 +1561,173 @@ async fn test_multisig_set_procedure_threshold_uses_current_num_approvers( Ok(()) } + +/// Executes a fully-signed 2-of-4 multisig transaction whose script `call`s one of the public +/// component getters, so the getter is exercised through the 16-felt `call` ABI (a getter that +/// returns at any operand-stack depth other than 16 aborts in `restore_context` with +/// `InvalidStackDepthOnReturn`). +async fn execute_multisig_getter_call(build_script: F) -> anyhow::Result<()> +where + F: FnOnce(&[PublicKey]) -> String, +{ + let (_secret_keys, auth_schemes, public_keys, authenticators) = + setup_keys_and_authenticators_with_scheme(4, 2, AuthScheme::EcdsaK256Keccak)?; + + 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, vec![])?; + + let mock_chain = MockChainBuilder::with_accounts([multisig_account.clone()]) + .unwrap() + .build() + .unwrap(); + + let script_code = build_script(&public_keys); + let tx_script = CodeBuilder::default() + .with_dynamically_linked_library(AuthMultisig::code())? + .compile_tx_script(&script_code)?; + + let salt = Word::from([Felt::from_u8(77); 4]); + + let tx_context_builder = mock_chain + .build_tx_context(multisig_account.id(), &[], &[])? + .tx_script(tx_script) + .auth_args(salt); + + // First pass without signatures: the getter `call` must return cleanly so the transaction + // reaches authentication and only fails there for missing signatures. + 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 = SigningInputs::TransactionSummary(tx_summary); + + let sig_1 = authenticators[0] + .get_signature(public_keys[0].to_commitment(), &tx_summary) + .await?; + let sig_2 = authenticators[1] + .get_signature(public_keys[1].to_commitment(), &tx_summary) + .await?; + + // Second pass with a valid quorum: the whole transaction, getter included, executes. + 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?; + + Ok(()) +} + +/// Regression test for the `call` ABI of `get_threshold_and_num_approvers`. +/// +/// The script asserts the returned `[default_threshold, num_approvers]` matches the 2-of-4 +/// configuration, which both proves the getter returns at operand-stack depth 16 and that it +/// yields the correct values. +#[tokio::test] +async fn test_get_threshold_and_num_approvers_call_abi() -> anyhow::Result<()> { + execute_multisig_getter_call(|_| { + " + @transaction_script + pub proc main + call.::miden::standards::components::auth::multisig::get_threshold_and_num_approvers + # => [default_threshold, num_approvers, pad(14)] + push.2 eq assert + # => [num_approvers, pad(14)] + push.4 eq assert + # => [pad(14)] + exec.::miden::core::sys::truncate_stack + end + " + .to_string() + }) + .await +} + +/// Regression test for the `call` ABI of `get_signer_at`. +/// +/// The signer at index `0` is queried and the script asserts the returned scheme id matches +/// `EcdsaK256Keccak`, exercising the getter's five-felt output through the 16-felt `call` ABI. +#[tokio::test] +async fn test_get_signer_at_call_abi() -> anyhow::Result<()> { + let expected_scheme_id = AuthScheme::EcdsaK256Keccak.as_u8(); + execute_multisig_getter_call(move |_| { + format!( + r#" + @transaction_script + pub proc main + # query the signer at index 0 + push.0 + call.::miden::standards::components::auth::multisig::get_signer_at + # => [PUB_KEY, scheme_id, pad(11)] + movup.4 eq.{expected_scheme_id} assert.err="expected scheme ID {expected_scheme_id}" + # => [PUB_KEY, pad(11)] + dropw + # => [pad(12)] + exec.::miden::core::sys::truncate_stack + end + "# + ) + }) + .await +} + +/// Regression test for the `call` ABI of `is_signer` when the queried key is a signer. +/// +/// A real approver public key is pushed by the script; the getter must return `1`. +#[tokio::test] +async fn test_is_signer_true_call_abi() -> anyhow::Result<()> { + execute_multisig_getter_call(|public_keys| { + let pub_key = public_keys[0].to_commitment(); + format!( + r#" + @transaction_script + pub proc main + push.{pub_key} + call.::miden::standards::components::auth::multisig::is_signer + # => [is_signer, pad(15)] + assert + # => [pad(15)] + exec.::miden::core::sys::truncate_stack + end + "# + ) + }) + .await +} + +/// Regression test for the `call` ABI of `is_signer` when the queried key is not a signer. +/// +/// A key that is not part of the approver set is pushed by the script; the getter must return `0` +/// after iterating over every approver. +#[tokio::test] +async fn test_is_signer_false_call_abi() -> anyhow::Result<()> { + execute_multisig_getter_call(|_| { + // A word that is not any approver's public key commitment. + let non_signer = Word::from([Felt::from_u8(0xff); 4]); + format!( + r#" + @transaction_script + pub proc main + push.{non_signer} + call.::miden::standards::components::auth::multisig::is_signer + # => [is_signer, pad(15)] + assertz + # => [pad(15)] + exec.::miden::core::sys::truncate_stack + end + "# + ) + }) + .await +}