diff --git a/contracts/ccip/ccip/sources/offramp_state_helper.move b/contracts/ccip/ccip/sources/offramp_state_helper.move index cfb6dd218..e9a28a0bb 100644 --- a/contracts/ccip/ccip/sources/offramp_state_helper.move +++ b/contracts/ccip/ccip/sources/offramp_state_helper.move @@ -1,7 +1,6 @@ module ccip::offramp_state_helper; use ccip::client::{Self, Any2SuiMessage, Any2SuiTokenAmount}; -use ccip::eth_abi; use ccip::ownable::OwnerCap; use ccip::receiver_registry; use ccip::state_object::{Self, CCIPObjectRef}; @@ -21,13 +20,6 @@ const ETokenTransferAlreadyCompleted: u64 = 8; const EMessageAlreadyExists: u64 = 9; const EInvalidOwnerCap: u64 = 10; const ETokenTransferNotCompleted: u64 = 11; -const EInvalidRemoteChainDecimals: u64 = 12; -const EDecimalOverflow: u64 = 13; -const EInvalidEncodedAmount: u64 = 14; - -const MAX_U256: u256 = - 115792089237316195423570985008687907853269984665640564039457584007913129639935; -const MAX_U64: u256 = 18446744073709551615; public struct OFFRAMP_STATE_HELPER has drop {} @@ -175,61 +167,24 @@ public fun get_token_param_data( ) } -fun parse_remote_decimals(source_pool_data: vector, local_decimals: u8): u8 { - let data_len = source_pool_data.length(); - if (data_len == 0) { - return local_decimals - }; - assert!(data_len == 32, EInvalidRemoteChainDecimals); - let remote_decimals = eth_abi::decode_u256_value(source_pool_data); - assert!(remote_decimals <= 255, EInvalidRemoteChainDecimals); - remote_decimals as u8 -} - -fun calculate_local_amount( - remote_amount: u256, - remote_decimals: u8, - local_decimals: u8, -): u64 { - let local_amount = if (remote_decimals == local_decimals) { - remote_amount - } else if (remote_decimals > local_decimals) { - let decimals_diff = remote_decimals - local_decimals; - let mut current_amount = remote_amount; - let mut i: u8 = 0; - while (i < decimals_diff) { - current_amount = current_amount / 10; - i = i + 1; - }; - current_amount - } else { - let decimals_diff = local_decimals - remote_decimals; - assert!(decimals_diff <= 77, EDecimalOverflow); - let mut multiplier: u256 = 1; - let mut i: u8 = 0; - while (i < decimals_diff) { - multiplier = multiplier * 10; - i = i + 1; - }; - assert!(remote_amount <= (MAX_U256 / multiplier), EDecimalOverflow); - remote_amount * multiplier - }; - assert!(local_amount <= MAX_U64, EInvalidEncodedAmount); - local_amount as u64 -} - /// only the token pool with a proper type proof can call this function to /// add a receipt to the receiver params. +/// +/// `local_amount` is the amount of tokens the pool actually minted/released on Sui. It is +/// the single source of truth for the destination amount and, when the message carries token +/// transfers, the value surfaced to the receiver. The pool computes it once (and uses the same +/// value to mint/release) then passes it in; we must NOT re-derive it here from +/// `source_pool_data`, whose encoding is pool-specific — e.g. the USDC/CCTP pool packs +/// (nonce, domain) into 64 bytes, not the remote decimals (see F-01). public fun complete_token_transfer( ref: &CCIPObjectRef, receiver_params: &mut ReceiverParams, + local_amount: u64, _: TypeProof, ) { let dest_token_transfer = receiver_params.token_transfer.borrow(); let token_receiver = dest_token_transfer.token_receiver; let dest_token_address = dest_token_transfer.dest_token_address; - let source_amount = dest_token_transfer.source_amount; - let source_pool_data = dest_token_transfer.source_pool_data; let (_, _, _, _, _, type_proof, _, _) = registry::get_token_config_data( ref, dest_token_address, @@ -240,9 +195,6 @@ public fun complete_token_transfer( assert!(type_proof == proof_tn_str, ETypeProofMismatch); if (receiver_params.message.is_some()) { - let local_decimals = registry::get_local_decimals_for_token(ref, dest_token_address); - let remote_decimals = parse_remote_decimals(source_pool_data, local_decimals); - let local_amount = calculate_local_amount(source_amount, remote_decimals, local_decimals); client::set_dest_token_amount( receiver_params.message.borrow_mut(), 0, @@ -352,20 +304,6 @@ public fun test_init(ctx: &mut TxContext) { init(OFFRAMP_STATE_HELPER {}, ctx); } -#[test_only] -public fun test_calculate_local_amount( - remote_amount: u256, - remote_decimals: u8, - local_decimals: u8, -): u64 { - calculate_local_amount(remote_amount, remote_decimals, local_decimals) -} - -#[test_only] -public fun test_parse_remote_decimals(source_pool_data: vector, local_decimals: u8): u8 { - parse_remote_decimals(source_pool_data, local_decimals) -} - #[test_only] public fun deconstruct_receiver_params_with_message_for_test( _: &DestTransferCap, diff --git a/contracts/ccip/ccip/tests/decimal_parity_test.move b/contracts/ccip/ccip/tests/decimal_parity_test.move deleted file mode 100644 index 881c5f12a..000000000 --- a/contracts/ccip/ccip/tests/decimal_parity_test.move +++ /dev/null @@ -1,133 +0,0 @@ -#[test_only] -module ccip::decimal_parity_test; - -use ccip::offramp_state_helper; - -#[test] -public fun test_parity_18_to_9() { - let result = offramp_state_helper::test_calculate_local_amount( - 1_000_000_000_000_000_000, // 1e18 - 18, - 9, - ); - assert!(result == 1_000_000_000); // 1e9 -} - -#[test] -public fun test_parity_18_to_6() { - let result = offramp_state_helper::test_calculate_local_amount( - 1_000_000_000_000_000_000, // 1e18 - 18, - 6, - ); - assert!(result == 1_000_000); // 1e6 -} - -#[test] -public fun test_parity_6_to_9() { - let result = offramp_state_helper::test_calculate_local_amount( - 1_000_000, // 1e6 - 6, - 9, - ); - assert!(result == 1_000_000_000); // 1e9 -} - -#[test] -public fun test_parity_same_decimals() { - let result = offramp_state_helper::test_calculate_local_amount( - 123_456_789, - 8, - 8, - ); - assert!(result == 123_456_789); -} - -#[test] -public fun test_parity_18_to_8() { - let result = offramp_state_helper::test_calculate_local_amount( - 5_000_000_000_000_000_000, // 5e18 - 18, - 8, - ); - assert!(result == 500_000_000); // 5e8 -} - -#[test] -public fun test_parity_8_to_18() { - let result = offramp_state_helper::test_calculate_local_amount( - 500_000_000, // 5e8 - 8, - 18, - ); - assert!(result == 5_000_000_000_000_000_000); // 5e18 -} - -#[test] -public fun test_parity_truncation_rounds_down() { - // 1_999_999_999 with 18->9 should truncate to 1 (not round up) - let result = offramp_state_helper::test_calculate_local_amount( - 1_999_999_999, // less than 1e10 - 18, - 9, - ); - assert!(result == 1); // floor(1_999_999_999 / 1e9) = 1 -} - -#[test] -public fun test_parity_zero_amount() { - let result = offramp_state_helper::test_calculate_local_amount(0, 18, 9); - assert!(result == 0); -} - -#[test] -public fun test_parse_remote_decimals_valid() { - // ABI-encoded u256(18) = 0x12 - let data = x"0000000000000000000000000000000000000000000000000000000000000012"; - let result = offramp_state_helper::test_parse_remote_decimals(data, 9); - assert!(result == 18); -} - -#[test] -public fun test_parse_remote_decimals_empty_falls_back_to_local() { - let result = offramp_state_helper::test_parse_remote_decimals(vector[], 9); - assert!(result == 9); -} - -#[test] -#[expected_failure] -public fun test_parse_remote_decimals_invalid_length() { - let data = x"001122"; // 3 bytes, not 0 or 32 - offramp_state_helper::test_parse_remote_decimals(data, 9); -} - -#[test] -public fun test_parity_matrix() { - // Comprehensive matrix: (source_amount, remote_decimals, local_decimals, expected) - // These values match what burn_mint_token_pool::token_pool::calculate_local_amount produces - let source_amounts: vector = vector[ - 1_000_000_000_000_000_000, // 1e18 - 500_000_000_000_000_000, // 0.5e18 - 1, // minimum - 999_999_999_999_999_999, // just under 1e18 - ]; - let remote_decimals: vector = vector[18, 18, 18, 18]; - let local_decimals: vector = vector[9, 9, 9, 9]; - let expected: vector = vector[ - 1_000_000_000, // 1e18 / 1e9 - 500_000_000, // 0.5e18 / 1e9 - 0, // 1 / 1e9 rounds to 0 - 999_999_999, // floor(999_999_999_999_999_999 / 1e9) - ]; - - let mut i: u64 = 0; - while (i < source_amounts.length()) { - let result = offramp_state_helper::test_calculate_local_amount( - source_amounts[i], - remote_decimals[i], - local_decimals[i], - ); - assert!(result == expected[i]); - i = i + 1; - }; -} diff --git a/contracts/ccip/ccip/tests/dest_token_amount_regression_test.move b/contracts/ccip/ccip/tests/dest_token_amount_regression_test.move index 80c3218d7..48ba185b2 100644 --- a/contracts/ccip/ccip/tests/dest_token_amount_regression_test.move +++ b/contracts/ccip/ccip/tests/dest_token_amount_regression_test.move @@ -116,6 +116,7 @@ public fun test_token_message_18_to_9_receiver_amount_equals_local() { offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + (EXPECTED_LOCAL_AMOUNT_9_DEC as u64), TestTypeProof {}, ); @@ -171,6 +172,7 @@ public fun test_token_only_path_unchanged() { offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + (SOURCE_AMOUNT_18_DEC as u64), TestTypeProof {}, ); @@ -265,6 +267,7 @@ public fun test_same_decimals_no_conversion() { offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + (source_amount as u64), TestTypeProof {}, ); diff --git a/contracts/ccip/ccip/tests/f01_usdc_message_regression_test.move b/contracts/ccip/ccip/tests/f01_usdc_message_regression_test.move new file mode 100644 index 000000000..05169671c --- /dev/null +++ b/contracts/ccip/ccip/tests/f01_usdc_message_regression_test.move @@ -0,0 +1,216 @@ +// Regression test for audit finding F-01: +// "USDC + programmatic message inbound transfer permanently aborts (stranded burned funds)" +// +// Before the fix, offramp_state_helper::complete_token_transfer re-derived the destination +// amount by parsing the token's source_pool_data as an ABI-encoded decimals word +// (assert!(data_len == 32, EInvalidRemoteChainDecimals)). The USDC/CCTP pool encodes its +// source_pool_data as encode_u64(nonce) ++ encode_u32(domain) = 64 bytes, so every USDC +// transfer that also carried a programmatic message aborted on that assert — stranding the +// already-CCTP-burned source funds. +// +// The fix makes the pool the single source of truth: it passes the local_amount it actually +// minted/released into complete_token_transfer, which surfaces that value to the receiver +// verbatim and never re-parses source_pool_data. This test proves the 64-byte USDC + message +// path now COMPLETES and surfaces the pool's amount. +// +// It uses only the audited public API and the genuine owner-minted DestTransferCap (the same +// object the offramp consumes at initialize); nothing is forged. The 64-byte blob is exactly +// what usdc_token_pool::encode_source_pool_data produces. + +#[test_only] +module ccip::f01_usdc_message_regression_test; + +use ccip::client; +use ccip::eth_abi; +use ccip::offramp_state_helper::{Self, DestTransferCap}; +use ccip::ownable::OwnerCap; +use ccip::state_object::{Self, CCIPObjectRef}; +use ccip::token_admin_registry as registry; +use ccip::upgrade_registry; +use std::ascii; +use std::string; +use std::type_name; +use sui::test_scenario::{Self as ts, Scenario}; + +public struct TestTypeProof has drop {} + +const OWNER: address = @0x1000; +const RECEIVER_ADDRESS: address = @0x2000; +const TOKEN_ADDRESS: address = + @0x1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b; +const TOKEN_POOL_ADDRESS: address = + @0xdeeb7a4662eec9f2f3def03fb937a663dddaa2e215b8078a284d026b7946c270; +const SOURCE_CHAIN_SELECTOR: u64 = 1000; +const SOURCE_AMOUNT: u256 = 1_000_000; // 1 USDC at 6 decimals (source-chain denominated) +// The amount the USDC pool actually mints on Sui (CCTP-attested), passed into +// complete_token_transfer as the single source of truth. +const LOCAL_MINTED_AMOUNT: u64 = 1_000_000; +const LOCAL_DECIMALS: u8 = 6; // USDC is 6 decimals on Sui +const USDC_DOMAIN: u32 = 0; // Ethereum CCTP domain +const USDC_NONCE: u64 = 12345; + +fun setup_test(): (Scenario, OwnerCap, CCIPObjectRef, DestTransferCap) { + let mut scenario = ts::begin(OWNER); + let ctx = scenario.ctx(); + + state_object::test_init(ctx); + scenario.next_tx(OWNER); + + let owner_cap = scenario.take_from_sender(); + let mut ref = scenario.take_shared(); + + upgrade_registry::initialize(&mut ref, &owner_cap, scenario.ctx()); + registry::initialize(&mut ref, &owner_cap, scenario.ctx()); + registry::initialize_local_decimals(&mut ref, &owner_cap, scenario.ctx()); + + offramp_state_helper::test_init(scenario.ctx()); + scenario.next_tx(OWNER); + let dest_cap = scenario.take_from_sender(); + + (scenario, owner_cap, ref, dest_cap) +} + +fun cleanup_test( + scenario: Scenario, + owner_cap: OwnerCap, + ref: CCIPObjectRef, + dest_cap: DestTransferCap, +) { + ts::return_to_sender(&scenario, owner_cap); + ts::return_shared(ref); + transfer::public_transfer(dest_cap, @0x0); + ts::end(scenario); +} + +fun register_usdc_pool(owner_cap: &OwnerCap, ref: &mut CCIPObjectRef, scenario: &mut Scenario) { + registry::register_pool_as_owner_v2( + owner_cap, + ref, + TOKEN_ADDRESS, + TOKEN_POOL_ADDRESS, + string::utf8(b"usdc_token_pool"), + ascii::string(b"TestType"), + OWNER, + type_name::into_string(type_name::with_defining_ids()), + vector
[], + vector
[], + LOCAL_DECIMALS, + scenario.ctx(), + ); +} + +/// Byte-exact mirror of usdc_token_pool::encode_source_pool_data: +/// nonce (u64) then domain (u32), each ABI-encoded into a 32-byte big-endian word => 64 bytes. +fun usdc_source_pool_data(domain: u32, nonce: u64): vector { + let mut spd = vector[]; + eth_abi::encode_u64(&mut spd, nonce); // 32 bytes + eth_abi::encode_u32(&mut spd, domain); // 32 bytes + spd +} + +/// Precondition: the bytes the USDC pool emits are 64 long (not 0 and not 32). This is the +/// blob that the pre-fix `assert!(data_len == 32)` rejected. +#[test] +fun test_usdc_source_pool_data_is_64_bytes() { + let spd = usdc_source_pool_data(USDC_DOMAIN, USDC_NONCE); + assert!(spd.length() == 64, 9999); +} + +/// THE F-01 REGRESSION: a USDC transfer carrying a programmatic message and a 64-byte +/// source_pool_data now COMPLETES (no abort) and surfaces the pool's local minted amount to +/// the receiver. This is the 64-byte case the prior test suite never covered. +#[test] +fun test_usdc_plus_message_completes_and_surfaces_local_amount() { + let (mut scenario, owner_cap, mut ref, dest_cap) = setup_test(); + register_usdc_pool(&owner_cap, &mut ref, &mut scenario); + + let mut receiver_params = offramp_state_helper::create_receiver_params( + &dest_cap, + SOURCE_CHAIN_SELECTOR, + ); + + // The 64-byte USDC blob (domain ++ nonce) — exactly what the offramp copies into + // receiver_params from the source pool. With the fix it is no longer parsed as decimals. + offramp_state_helper::add_dest_token_transfer( + &dest_cap, + &mut receiver_params, + RECEIVER_ADDRESS, + SOURCE_CHAIN_SELECTOR, + SOURCE_AMOUNT, + TOKEN_ADDRESS, + TOKEN_POOL_ADDRESS, + b"source_pool_address", + usdc_source_pool_data(USDC_DOMAIN, USDC_NONCE), + b"offchain_data", + ); + + // The programmatic-message leg: a populated message makes message.is_some(), the branch + // that pre-fix triggered the decimals re-parse and aborted. + let dest_token_amounts = client::new_dest_token_amounts( + vector[TOKEN_ADDRESS], + vector[SOURCE_AMOUNT], + ); + let test_message = client::new_any2sui_message( + b"message_id_32_bytes_long_test_msg", + SOURCE_CHAIN_SELECTOR, + b"sender_address", + b"call_my_receiver", + @0x5432, + @0x12345, + dest_token_amounts, + ); + offramp_state_helper::populate_message(&dest_cap, &mut receiver_params, test_message); + + // Pre-fix this aborted with EInvalidRemoteChainDecimals; post-fix it completes and writes + // the pool's already-minted local amount into the message. + offramp_state_helper::complete_token_transfer( + &ref, + &mut receiver_params, + LOCAL_MINTED_AMOUNT, + TestTypeProof {}, + ); + + let message = offramp_state_helper::extract_any2sui_message(&mut receiver_params); + let (_, _, _, _, _, _, extracted) = client::consume_any2sui_message(message, @0x5432); + // The receiver sees exactly the amount the pool minted on Sui — the single source of truth. + assert!(client::get_amount(&extracted[0]) == (LOCAL_MINTED_AMOUNT as u256)); + + offramp_state_helper::deconstruct_receiver_params(&dest_cap, receiver_params); + cleanup_test(scenario, owner_cap, ref, dest_cap); +} + +/// Control: the same 64-byte USDC transfer with NO message also completes (it always did — +/// the pure token path never re-parsed source_pool_data). +#[test] +fun test_usdc_token_only_succeeds() { + let (mut scenario, owner_cap, mut ref, dest_cap) = setup_test(); + register_usdc_pool(&owner_cap, &mut ref, &mut scenario); + + let mut receiver_params = offramp_state_helper::create_receiver_params( + &dest_cap, + SOURCE_CHAIN_SELECTOR, + ); + + offramp_state_helper::add_dest_token_transfer( + &dest_cap, + &mut receiver_params, + RECEIVER_ADDRESS, + SOURCE_CHAIN_SELECTOR, + SOURCE_AMOUNT, + TOKEN_ADDRESS, + TOKEN_POOL_ADDRESS, + b"source_pool_address", + usdc_source_pool_data(USDC_DOMAIN, USDC_NONCE), + b"offchain_data", + ); + + offramp_state_helper::complete_token_transfer( + &ref, + &mut receiver_params, + LOCAL_MINTED_AMOUNT, + TestTypeProof {}, + ); + + offramp_state_helper::deconstruct_receiver_params(&dest_cap, receiver_params); + cleanup_test(scenario, owner_cap, ref, dest_cap); +} diff --git a/contracts/ccip/ccip/tests/offramp_state_helper_tests.move b/contracts/ccip/ccip/tests/offramp_state_helper_tests.move index c92bc621b..0b98c8443 100644 --- a/contracts/ccip/ccip/tests/offramp_state_helper_tests.move +++ b/contracts/ccip/ccip/tests/offramp_state_helper_tests.move @@ -234,10 +234,11 @@ public fun test_complete_token_transfer() { // Create a test coin to transfer let test_coin = coin::mint_for_testing(500, scenario.ctx()); - // Complete the token transfer + // Complete the token transfer (no message: local_amount is unused on this path) offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + 1000, TestTypeProof {}, ); @@ -426,6 +427,7 @@ public fun test_complete_token_transfer_twice_should_fail() { offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + 1000, TestTypeProof {}, ); @@ -433,6 +435,7 @@ public fun test_complete_token_transfer_twice_should_fail() { offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + 1000, TestTypeProof {}, ); @@ -548,15 +551,17 @@ public fun test_extract_token_message_after_complete_succeeds() { ); populate_token_message_receiver_params(&dest_cap, &mut receiver_params); + // The pool passes in the local amount it minted/released (here 1e9). That value is + // surfaced to the receiver verbatim — no re-derivation from source_pool_data (F-01). offramp_state_helper::complete_token_transfer( &ref, &mut receiver_params, + 1_000_000_000, TestTypeProof {}, ); let message = offramp_state_helper::extract_any2sui_message(&mut receiver_params); let (_, _, _, _, _, _, dest_token_amounts) = client::consume_any2sui_message(message, @0x5432); - // After fix: amount is recomputed from 18-dec to 9-dec assert!(client::get_amount(&dest_token_amounts[0]) == 1_000_000_000); offramp_state_helper::deconstruct_receiver_params(&dest_cap, receiver_params); diff --git a/contracts/ccip/ccip_token_pools/burn_mint_token_pool/sources/burn_mint_token_pool.move b/contracts/ccip/ccip_token_pools/burn_mint_token_pool/sources/burn_mint_token_pool.move index a30a4d6cb..fa4ecf746 100644 --- a/contracts/ccip/ccip_token_pools/burn_mint_token_pool/sources/burn_mint_token_pool.move +++ b/contracts/ccip/ccip_token_pools/burn_mint_token_pool/sources/burn_mint_token_pool.move @@ -360,6 +360,7 @@ public fun release_or_mint( offramp_sh::complete_token_transfer( ref, receiver_params, + local_amount, TypeProof {}, ); } diff --git a/contracts/ccip/ccip_token_pools/lock_release_token_pool/sources/lock_release_token_pool.move b/contracts/ccip/ccip_token_pools/lock_release_token_pool/sources/lock_release_token_pool.move index c91f4ad8a..9cfbb0aae 100644 --- a/contracts/ccip/ccip_token_pools/lock_release_token_pool/sources/lock_release_token_pool.move +++ b/contracts/ccip/ccip_token_pools/lock_release_token_pool/sources/lock_release_token_pool.move @@ -383,6 +383,7 @@ public fun release_or_mint( offramp_sh::complete_token_transfer( ref, receiver_params, + local_amount, TypeProof {}, ); } diff --git a/contracts/ccip/ccip_token_pools/managed_token_pool/sources/managed_token_pool.move b/contracts/ccip/ccip_token_pools/managed_token_pool/sources/managed_token_pool.move index d995eee36..d28708d75 100644 --- a/contracts/ccip/ccip_token_pools/managed_token_pool/sources/managed_token_pool.move +++ b/contracts/ccip/ccip_token_pools/managed_token_pool/sources/managed_token_pool.move @@ -407,6 +407,7 @@ public fun release_or_mint( offramp_sh::complete_token_transfer( ref, receiver_params, + local_amount, TypeProof {}, ); } diff --git a/contracts/ccip/ccip_token_pools/usdc_token_pool/sources/usdc_token_pool.move b/contracts/ccip/ccip_token_pools/usdc_token_pool/sources/usdc_token_pool.move index 8e3bc76d2..048670bae 100644 --- a/contracts/ccip/ccip_token_pools/usdc_token_pool/sources/usdc_token_pool.move +++ b/contracts/ccip/ccip_token_pools/usdc_token_pool/sources/usdc_token_pool.move @@ -449,6 +449,7 @@ public fun release_or_mint( offramp_sh::complete_token_transfer( ref, receiver_params, + amount, TypeProof {}, ); }