From f0fb39041b687b7e8c289835ca09673196dcdee2 Mon Sep 17 00:00:00 2001 From: Brice Dobry Date: Tue, 14 Oct 2025 14:54:46 -0400 Subject: [PATCH 1/3] feat: enforce ordering of allowance check --- clarity/src/vm/functions/post_conditions.rs | 82 ++++++++++----------- clarity/src/vm/tests/post_conditions.rs | 36 +++++++++ 2 files changed, 76 insertions(+), 42 deletions(-) diff --git a/clarity/src/vm/functions/post_conditions.rs b/clarity/src/vm/functions/post_conditions.rs index f3fe9586301..1a96e5baf70 100644 --- a/clarity/src/vm/functions/post_conditions.rs +++ b/clarity/src/vm/functions/post_conditions.rs @@ -384,6 +384,13 @@ fn check_allowances( allowances: Vec, assets: &AssetMap, ) -> InterpreterResult> { + let mut earliest_violation: Option = None; + let mut record_violation = |candidate: u128| { + if earliest_violation.map_or(true, |current| candidate < current) { + earliest_violation = Some(candidate); + } + }; + // Elements are (index in allowances, amount) let mut stx_allowances: Vec<(usize, u128)> = Vec::new(); // Map assets to a vector of (index in allowances, amount) @@ -428,34 +435,30 @@ fn check_allowances( // Check STX movements if let Some(stx_moved) = assets.get_stx(owner) { - // If there are no allowances for STX, any movement is a violation if stx_allowances.is_empty() { - return Ok(Some(MAX_ALLOWANCES as u128)); - } - - // Check against the STX allowances - for (index, allowance) in &stx_allowances { - if stx_moved > *allowance { - return Ok(Some(u128::try_from(*index).map_err(|_| { - InterpreterError::Expect("failed to convert index to u128".into()) - })?)); + // If there are no allowances for STX, any movement is a violation + record_violation(MAX_ALLOWANCES as u128); + } else { + for (index, allowance) in &stx_allowances { + if stx_moved > *allowance { + record_violation(*index as u128); + break; + } } } } // Check STX burns if let Some(stx_burned) = assets.get_stx_burned(owner) { - // If there are no allowances for STX, any burn is a violation if stx_allowances.is_empty() { - return Ok(Some(MAX_ALLOWANCES as u128)); - } - - // Check against the STX allowances - for (index, allowance) in &stx_allowances { - if stx_burned > *allowance { - return Ok(Some(u128::try_from(*index).map_err(|_| { - InterpreterError::Expect("failed to convert index to u128".into()) - })?)); + // If there are no allowances for STX, any burn is a violation + record_violation(MAX_ALLOWANCES as u128); + } else { + for (index, allowance) in &stx_allowances { + if stx_burned > *allowance { + record_violation(*index as u128); + break; + } } } } @@ -479,7 +482,8 @@ fn check_allowances( if merged.is_empty() { // No allowance for this asset, any movement is a violation - return Ok(Some(MAX_ALLOWANCES as u128)); + record_violation(MAX_ALLOWANCES as u128); + continue; } // Sort by allowance index so we check allowances in order @@ -487,9 +491,8 @@ fn check_allowances( for (index, allowance) in merged { if *amount_moved > allowance { - return Ok(Some(u128::try_from(index).map_err(|_| { - InterpreterError::Expect("failed to convert index to u128".into()) - })?)); + record_violation(index as u128); + break; } } } @@ -512,20 +515,17 @@ fn check_allowances( if merged.is_empty() { // No allowance for this asset, any movement is a violation - return Ok(Some(MAX_ALLOWANCES as u128)); + record_violation(MAX_ALLOWANCES as u128); + continue; } // Sort by allowance index so we check allowances in order merged.sort_by_key(|(idx, _)| *idx); for (index, allowance_vec) in merged { - // Check against the NFT allowances - for id_moved in ids_moved { - if !allowance_vec.contains(id_moved) { - return Ok(Some(u128::try_from(index).map_err(|_| { - InterpreterError::Expect("failed to convert index to u128".into()) - })?)); - } + if ids_moved.iter().any(|id| !allowance_vec.contains(id)) { + record_violation(index as u128); + break; } } } @@ -535,20 +535,18 @@ fn check_allowances( if let Some(stx_stacked) = assets.get_stacking(owner) { // If there are no allowances for stacking, any stacking is a violation if stacking_allowances.is_empty() { - return Ok(Some(MAX_ALLOWANCES as u128)); - } - - // Check against the stacking allowances - for (index, allowance) in &stacking_allowances { - if stx_stacked > *allowance { - return Ok(Some(u128::try_from(*index).map_err(|_| { - InterpreterError::Expect("failed to convert index to u128".into()) - })?)); + record_violation(MAX_ALLOWANCES as u128); + } else { + for (index, allowance) in &stacking_allowances { + if stx_stacked > *allowance { + record_violation(*index as u128); + break; + } } } } - Ok(None) + Ok(earliest_violation) } /// Handles all allowance functions, always returning an error, since these are diff --git a/clarity/src/vm/tests/post_conditions.rs b/clarity/src/vm/tests/post_conditions.rs index 9b18f8b062c..90b30003c3f 100644 --- a/clarity/src/vm/tests/post_conditions.rs +++ b/clarity/src/vm/tests/post_conditions.rs @@ -1404,3 +1404,39 @@ fn test_restrict_assets_with_error_in_body() { ClarityError::ShortReturn(ShortReturnType::ExpectedValue(expected_err.into())); assert_eq!(short_return, execute(snippet).unwrap_err()); } + +#[test] +fn test_restrict_assets_with_multiple_violations_different_kinds() { + let snippet = r#" +(define-non-fungible-token stackaroo uint) +(nft-mint? stackaroo u122 tx-sender) +(nft-mint? stackaroo u123 tx-sender) +(let ((recipient 'SP000000000000000000002Q6VF78)) + (restrict-assets? tx-sender ((with-nft current-contract "stackaroo" (list u122)) (with-stx u10)) + (begin + (try! (stx-transfer? u50 tx-sender recipient)) + (try! (nft-transfer? stackaroo u123 tx-sender recipient)) + ) + ) +)"#; + let expected = Value::error(Value::UInt(0)).unwrap(); + assert_eq!(expected, execute(snippet).unwrap().unwrap()); +} + +#[test] +fn test_restrict_assets_with_multiple_violations_different_kinds_order_2() { + let snippet = r#" +(define-non-fungible-token stackaroo uint) +(nft-mint? stackaroo u122 tx-sender) +(nft-mint? stackaroo u123 tx-sender) +(let ((recipient 'SP000000000000000000002Q6VF78)) + (restrict-assets? tx-sender ((with-stx u10) (with-nft current-contract "stackaroo" (list u122))) + (begin + (try! (nft-transfer? stackaroo u123 tx-sender recipient)) + (try! (stx-transfer? u50 tx-sender recipient)) + ) + ) +)"#; + let expected = Value::error(Value::UInt(0)).unwrap(); + assert_eq!(expected, execute(snippet).unwrap().unwrap()); +} From b22dcac3f727fa2b3a1139076b48eb2d5de31d35 Mon Sep 17 00:00:00 2001 From: Brice Dobry Date: Tue, 14 Oct 2025 15:08:27 -0400 Subject: [PATCH 2/3] chore: clippy suggestion --- clarity/src/vm/functions/post_conditions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clarity/src/vm/functions/post_conditions.rs b/clarity/src/vm/functions/post_conditions.rs index 1a96e5baf70..f7c34cfca90 100644 --- a/clarity/src/vm/functions/post_conditions.rs +++ b/clarity/src/vm/functions/post_conditions.rs @@ -386,7 +386,7 @@ fn check_allowances( ) -> InterpreterResult> { let mut earliest_violation: Option = None; let mut record_violation = |candidate: u128| { - if earliest_violation.map_or(true, |current| candidate < current) { + if earliest_violation.is_none_or(|current| candidate < current) { earliest_violation = Some(candidate); } }; From 4fb73a0a9d80b3373f52b887be03a19436ffed51 Mon Sep 17 00:00:00 2001 From: Brice Dobry Date: Tue, 14 Oct 2025 17:29:23 -0400 Subject: [PATCH 3/3] refactor: avoid sort --- clarity/src/vm/functions/post_conditions.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/clarity/src/vm/functions/post_conditions.rs b/clarity/src/vm/functions/post_conditions.rs index f7c34cfca90..9786b653eb5 100644 --- a/clarity/src/vm/functions/post_conditions.rs +++ b/clarity/src/vm/functions/post_conditions.rs @@ -486,13 +486,9 @@ fn check_allowances( continue; } - // Sort by allowance index so we check allowances in order - merged.sort_by_key(|(idx, _)| *idx); - for (index, allowance) in merged { if *amount_moved > allowance { record_violation(index as u128); - break; } } } @@ -519,13 +515,9 @@ fn check_allowances( continue; } - // Sort by allowance index so we check allowances in order - merged.sort_by_key(|(idx, _)| *idx); - for (index, allowance_vec) in merged { if ids_moved.iter().any(|id| !allowance_vec.contains(id)) { record_violation(index as u128); - break; } } }