diff --git a/Cargo.lock b/Cargo.lock index a5559f584c16..86e8a5afab8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3101,6 +3101,7 @@ dependencies = [ "prost 0.13.4", "rand 0.8.5", "serde", + "serde_bytes", "serde_cbor", "sha2 0.10.8", "yansi 0.5.1", diff --git a/rs/nns/cmc/BUILD.bazel b/rs/nns/cmc/BUILD.bazel index 56ac2907ce2a..270763dd66ea 100644 --- a/rs/nns/cmc/BUILD.bazel +++ b/rs/nns/cmc/BUILD.bazel @@ -50,6 +50,14 @@ BUILD_DEPENDENCIES = [ "@crate_index//:build-info-build", ] +DEV_DEPENDENCIES = [ + # Keep sorted. + "//rs/types/types_test_utils", + "@crate_index//:candid_parser", + "@crate_index//:futures", + "@crate_index//:serde_bytes", +] + ALIASES = {} cargo_build_script( @@ -86,7 +94,7 @@ rust_canister( rust_test( name = "cmc_test", crate = ":cmc", - deps = DEPENDENCIES + ["@crate_index//:futures"], + deps = DEPENDENCIES + DEV_DEPENDENCIES, ) rust_test( @@ -96,10 +104,5 @@ rust_test( env = { "CARGO_MANIFEST_DIR": "rs/nns/cmc", }, - deps = [ - # Keep sorted. - "//rs/types/types_test_utils", - "@crate_index//:candid_parser", - "@crate_index//:futures", - ], + deps = DEPENDENCIES + DEV_DEPENDENCIES, ) diff --git a/rs/nns/cmc/Cargo.toml b/rs/nns/cmc/Cargo.toml index c2597568c0d4..a6d775f9d4ed 100644 --- a/rs/nns/cmc/Cargo.toml +++ b/rs/nns/cmc/Cargo.toml @@ -47,6 +47,7 @@ yansi = "0.5.0" candid_parser = { workspace = true } futures = { workspace = true } ic-types-test-utils = { path = "../../types/types_test_utils" } +serde_bytes = { workspace = true } [[bin]] name = "cycles-minting-canister" diff --git a/rs/nns/cmc/src/main.rs b/rs/nns/cmc/src/main.rs index 51fe6e728e88..4d11dfce0232 100644 --- a/rs/nns/cmc/src/main.rs +++ b/rs/nns/cmc/src/main.rs @@ -33,7 +33,7 @@ use ic_nns_constants::{ use ic_types::{CanisterId, Cycles, PrincipalId, SubnetId}; use icp_ledger::{ AccountIdentifier, Block, BlockIndex, BlockRes, CyclesResponse, Memo, Operation, SendArgs, - Subaccount, Tokens, TransactionNotification, DEFAULT_TRANSFER_FEE, + Subaccount, Tokens, Transaction, TransactionNotification, DEFAULT_TRANSFER_FEE, }; use icrc_ledger_types::icrc1::account::Account; use lazy_static::lazy_static; @@ -1569,6 +1569,62 @@ fn memo_to_intent_str(memo: Memo) -> String { } } +/// Returns Ok if transaction matches expected_memo. +/// +/// First, looks at the memo field. If that does not match, falls back to the +/// icrc1_field. If that is of length 8, converts assuming little endian, and if +/// that matches, returns Ok. +fn transaction_has_expected_memo( + transaction: &Transaction, + expected_memo: Memo, +) -> Result<(), NotifyError> { + fn stringify_memo(memo: Memo) -> String { + format!("{} ({})", memo_to_intent_str(memo), memo.0) + } + + if transaction.memo == expected_memo { + return Ok(()); + } + + // Fall back to icrc1_memo. + + // Read the field. + let Some(icrc1_memo) = &transaction.icrc1_memo else { + return Err(NotifyError::InvalidTransaction(format!( + "The transaction's memo ({}) does not have the required value ({}).", + stringify_memo(transaction.memo), + stringify_memo(expected_memo), + ))); + }; + + // Convert it to Memo. + type U64Array = [u8; std::mem::size_of::()]; + let observed_icrc1_memo = U64Array::try_from(icrc1_memo.as_ref()).map_err(|_err| { + NotifyError::InvalidTransaction(format!( + "The transaction's memo ({}) does not have the required value ({}).", + stringify_memo(transaction.memo), + stringify_memo(expected_memo), + )) + })?; + let observed_icrc1_memo = Memo(u64::from_le_bytes(observed_icrc1_memo)); + + // Compare to the required value. + if observed_icrc1_memo == expected_memo { + return Ok(()); + } + + Err(NotifyError::InvalidTransaction(format!( + "Neither the memo ({}) nor the icrc1_memo ({}) of the transaction \ + has the required value ({}).", + stringify_memo(transaction.memo), + stringify_memo(observed_icrc1_memo), + stringify_memo(expected_memo), + ))) +} + +/// Returns amount, and source of the transfer in (ICP) ledger. +/// +/// Returns Ok if the arguments are matched. (Otherwise, returns Err). async fn fetch_transaction( block_index: BlockIndex, expected_destination_account: AccountIdentifier, @@ -1588,22 +1644,15 @@ async fn fetch_transaction( )) } }; + if to != expected_destination_account { return Err(NotifyError::InvalidTransaction(format!( "Destination account in the block ({}) different than in the notification ({})", to, expected_destination_account ))); } - let memo = block.transaction().memo; - if memo != expected_memo { - return Err(NotifyError::InvalidTransaction(format!( - "Intent in the block ({} == {}) different than in the notification ({} == {})", - memo.0, - memo_to_intent_str(memo), - expected_memo.0, - memo_to_intent_str(expected_memo), - ))); - } + + transaction_has_expected_memo(block.transaction().as_ref(), expected_memo)?; Ok((amount, from)) } @@ -2443,6 +2492,7 @@ mod tests { use super::*; use ic_types_test_utils::ids::{subnet_test_id, user_test_id}; use rand::Rng; + use serde_bytes::ByteBuf; use std::str::FromStr; pub(crate) fn init_test_state() { @@ -3158,4 +3208,172 @@ mod tests { ) .expect("The CMC canister interface is not compatible with the cmc.did file"); } + + #[test] + fn test_transaction_has_expected_memo_happy() { + // Not relevant to this test. + let operation = Operation::Mint { + to: AccountIdentifier::new(PrincipalId::new_user_test_id(668_857_347), None), + amount: Tokens::from_e8s(123_456), + }; + + // Case A: Legacy memo is used. + let transaction_with_legacy_memo = Transaction { + memo: Memo(42), + icrc1_memo: None, + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + + assert_eq!( + transaction_has_expected_memo(&transaction_with_legacy_memo, Memo(42),), + Ok(()), + ); + + // Case B: When the user uses icrc1's memo to indicate the purpose of + // the transfer, and as a result the legacy memo field is implicitly set + // to 0. + let transaction_with_icrc1_memo = Transaction { + memo: Memo(0), + icrc1_memo: Some(ByteBuf::from(43_u64.to_le_bytes().to_vec())), + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + + assert_eq!( + transaction_has_expected_memo(&transaction_with_icrc1_memo, Memo(43),), + Ok(()), + ); + } + + #[test] + fn test_transaction_has_expected_memo_sad() { + // Not relevant to this test. + let operation = Operation::Mint { + to: AccountIdentifier::new(PrincipalId::new_user_test_id(668_857_347), None), + amount: Tokens::from_e8s(123_456), + }; + + // Case A: Legacy memo is used. + { + let transaction = Transaction { + memo: Memo(77), + icrc1_memo: None, + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + let result = transaction_has_expected_memo(&transaction, Memo(42)); + + let original_err = match result { + Err(NotifyError::InvalidTransaction(err)) => err, + wrong => panic!("{:?}", wrong), + }; + + let lower_err = original_err.to_lowercase(); + for key_word in ["memo", "77", "42"] { + assert!( + lower_err.contains(key_word), + "{} not in {:?}", + key_word, + original_err + ); + } + } + + // Case B: When the user uses icrc1's memo to indicate the purpose of + // the transfer, and as a result the legacy memo field is implicitly set + // to 0. + { + let transaction = Transaction { + memo: Memo(0), + icrc1_memo: Some(ByteBuf::from(78_u64.to_le_bytes().to_vec())), + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + + let result = transaction_has_expected_memo(&transaction, Memo(42)); + + let original_err = match result { + Err(NotifyError::InvalidTransaction(err)) => err, + wrong => panic!("{:?}", wrong), + }; + + let lower_err = original_err.to_lowercase(); + for key_word in ["memo", "78", "42"] { + assert!( + lower_err.contains(key_word), + "{} not in {:?}", + key_word, + original_err + ); + } + } + + // Case C: icrc1's memo is used, but is not of length 8, and we + // therefore do not consider it to contain a (little endian) u64. + { + let transaction = Transaction { + memo: Memo(0), + icrc1_memo: Some(ByteBuf::from(vec![1, 2, 3])), + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + + let result = transaction_has_expected_memo(&transaction, Memo(42)); + + let original_err = match result { + Err(NotifyError::InvalidTransaction(err)) => err, + wrong => panic!("{:?}", wrong), + }; + + let lower_err = original_err.to_lowercase(); + for key_word in ["memo", "0", "42"] { + assert!( + lower_err.contains(key_word), + "{} not in {:?}", + key_word, + original_err + ); + } + } + + // Case D: legacy memo is 0, and ircr1_memo is None. + { + let transaction = Transaction { + memo: Memo(0), + icrc1_memo: None, + + // Irrelevant to this test. + operation: operation.clone(), + created_at_time: None, + }; + + let result = transaction_has_expected_memo(&transaction, Memo(42)); + + let original_err = match result { + Err(NotifyError::InvalidTransaction(err)) => err, + wrong => panic!("{:?}", wrong), + }; + + let lower_err = original_err.to_lowercase(); + for key_word in ["memo", "0", "42"] { + assert!( + lower_err.contains(key_word), + "{} not in {:?}", + key_word, + original_err + ); + } + } + } }