fix: check fee before funds

.gitignore

@@ -9,8 +9,9 @@ target/
 # MSVC Windows builds of rustc generate these, which store debugging information
 *.pdb
 
-# Ignore the dfx build artifacts
+# Ignore build artifacts
 .dfx
+**/.icp/cache/
 
 # Ignore state machine binary
 ic-test-state-machine

CHANGELOG.md

@@ -5,6 +5,7 @@
 ## [Unreleased] - ReleaseDate
 
 * Add support for canister settings `wasm_memory_limit`, `wasm_memory_threshold`, `log_visibility`, and `environment_variables`.
+* Fixed a bug where `InsufficientFunds` errors took precedence over `BadFee` errors in `icrc1_transfer`, `icrc2_approve`, and `icrc2_transfer_from`.
 
 ## [1.0.6] - 2025-09-19
 * Add support for `initial_balances` in `InitArgs`. When specifying initial balances it is up to the installer to ensure that the cycles ledger has sufficient cycles available to spend these cycles.

cycles-ledger/src/storage.rs

@@ -962,6 +962,13 @@ pub fn transfer(
         }
     }
 
+    // BadFee takes precedence over InsufficientFunds per ICRC-1 spec
+    if suggested_fee.is_some() && suggested_fee != Some(config::FEE) {
+        return Err(BadFee {
+            expected_fee: Nat::from(config::FEE),
+        });
+    }
+
     // if `amount` + `fee` overflows then the user doesn't have enough funds
     let Some(amount_with_fee) = amount.checked_add(config::FEE) else {
         return Err(InsufficientFunds {
@@ -1074,6 +1081,13 @@ pub fn approve(
         });
     }
 
+    // BadFee takes precedence over InsufficientFunds per ICRC-1 spec
+    if suggested_fee.is_some() && suggested_fee != Some(config::FEE) {
+        return Err(ApproveError::BadFee {
+            expected_fee: Nat::from(config::FEE),
+        });
+    }
+
     // check that the `from` account has enough funds to pay the fee
     let balance = balance_of(&from);
     if balance < config::FEE {
@@ -1116,9 +1130,6 @@ pub fn approve(
 
 #[derive(Debug)]
 enum ProcessTransactionError {
-    BadFee {
-        expected_fee: u128,
-    },
     Duplicate {
         duplicate_of: u64,
         canister_id: Option<Principal>,
@@ -1148,9 +1159,6 @@ impl std::error::Error for ProcessTransactionError {
 impl std::fmt::Display for ProcessTransactionError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            Self::BadFee { expected_fee } => {
-                write!(f, "Invalid fee, expected fee is {}", expected_fee)
-            }
             Self::Duplicate { duplicate_of, .. } => write!(
                 f,
                 "Input transaction is a duplicate of transaction at index {}",
@@ -1325,9 +1333,6 @@ impl From<ProcessTransactionError> for TransferFromError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { expected_fee } => Self::BadFee {
-                expected_fee: Nat::from(expected_fee),
-            },
             Duplicate { duplicate_of, .. } => Self::Duplicate {
                 duplicate_of: Nat::from(duplicate_of),
             },
@@ -1343,7 +1348,6 @@ impl From<ProcessTransactionError> for WithdrawFromError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { .. } => ic_cdk::trap("BadFee should not happen for withdraw"),
             Duplicate { duplicate_of, .. } => Self::Duplicate {
                 duplicate_of: Nat::from(duplicate_of),
             },
@@ -1366,9 +1370,6 @@ impl From<ProcessTransactionError> for ApproveError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { expected_fee } => Self::BadFee {
-                expected_fee: Nat::from(expected_fee),
-            },
             Duplicate { duplicate_of, .. } => Self::Duplicate {
                 duplicate_of: Nat::from(duplicate_of),
             },
@@ -1383,9 +1384,6 @@ impl From<ProcessTransactionError> for WithdrawError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { expected_fee } => Self::BadFee {
-                expected_fee: Nat::from(expected_fee),
-            },
             Duplicate { duplicate_of, .. } => Self::Duplicate {
                 duplicate_of: Nat::from(duplicate_of),
             },
@@ -1400,13 +1398,6 @@ impl From<ProcessTransactionError> for CreateCanisterError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { expected_fee } => Self::GenericError {
-                error_code: CreateCanisterError::BAD_FEE_ERROR.into(),
-                message: format!(
-                    "BadFee. Expected fee: {}. Should never happen.",
-                    expected_fee
-                ),
-            },
             Duplicate {
                 duplicate_of,
                 canister_id,
@@ -1425,13 +1416,6 @@ impl From<ProcessTransactionError> for CreateCanisterFromError {
         use ProcessTransactionError::*;
 
         match error {
-            BadFee { expected_fee } => Self::GenericError {
-                error_code: CreateCanisterError::BAD_FEE_ERROR.into(),
-                message: format!(
-                    "BadFee. Expected fee: {}. Should never happen.",
-                    expected_fee
-                ),
-            },
             Duplicate {
                 duplicate_of,
                 canister_id,
@@ -1514,20 +1498,15 @@ impl From<UseAllowanceError> for CreateCanisterFromError {
     }
 }
 
-// Validates the suggested fee and returns the effective fee.
-// If the validation fails then return Err with the expected fee.
-fn validate_suggested_fee(op: &Operation) -> Result<Option<u128>, u128> {
+/// Returns the effective fee for the operation.
+/// Callers must validate the suggested fee before calling this function.
+fn effective_fee(op: &Operation) -> Option<u128> {
     use Operation as Op;
 
     match op {
-        Op::Mint { .. } => Ok(Some(0)),
-        Op::Burn { .. } => Ok(Some(config::FEE)),
-        Op::Transfer { fee, .. } | Op::Approve { fee, .. } => {
-            if fee.is_some() && fee != &Some(config::FEE) {
-                return Err(config::FEE);
-            }
-            Ok(fee.is_none().then_some(config::FEE))
-        }
+        Op::Mint { .. } => Some(0),
+        Op::Burn { .. } => Some(config::FEE),
+        Op::Transfer { fee, .. } | Op::Approve { fee, .. } => fee.is_none().then_some(config::FEE),
     }
 }
 
@@ -1557,8 +1536,6 @@ fn check_duplicate(transaction: &Transaction) -> Result<(), ProcessTransactionEr
 }
 
 fn process_transaction(transaction: Transaction, now: u64) -> Result<u64, ProcessTransactionError> {
-    use ProcessTransactionError as PTErr;
-
     // The ICRC-1 and ICP Ledgers trap when the memo validation fails
     // so we do the same.
     if let Err(err) = validate_memo(&transaction.memo) {
@@ -1567,8 +1544,7 @@ fn process_transaction(transaction: Transaction, now: u64) -> Result<u64, Proces
 
     validate_created_at_time(&transaction.created_at_time, now)?;
 
-    let effective_fee = validate_suggested_fee(&transaction.operation)
-        .map_err(|expected_fee| PTErr::BadFee { expected_fee })?;
+    let effective_fee = effective_fee(&transaction.operation);
 
     let block = Block {
         transaction,

cycles-ledger/tests/tests.rs

@@ -3689,6 +3689,107 @@ fn test_icrc1_transfer_invalid_arg(env: &TestEnv) {
     // memo is tested by [test_icrc1_transfer_invalid_memo]
 }
 
+/// ICRC-1 spec: BadFee takes precedence over InsufficientFunds.
+/// ICRC-2 says nothing about order, but we keep it consistent
+#[test]
+fn test_bad_fee_takes_precedence_over_insufficient_funds() {
+    let env = TestEnv::setup();
+    let fee = env.icrc1_fee();
+
+    // --- icrc1_transfer ---
+    {
+        let account_from = account(1, None);
+        let account_to = account(2, None);
+        assert_eq!(Nat::from(0u8), env.icrc1_balance_of(account_from));
+
+        for bad_fee in [0, fee - 1, fee + 1, u128::MAX] {
+            let args = TransferArgs {
+                from_subaccount: account_from.subaccount,
+                to: account_to,
+                amount: Nat::from(fee),
+                fee: Some(Nat::from(bad_fee)),
+                created_at_time: None,
+                memo: None,
+            };
+            assert_eq!(
+                Err(TransferError::BadFee {
+                    expected_fee: Nat::from(fee)
+                }),
+                env.icrc1_transfer(account_from.owner, args),
+                "icrc1_transfer: BadFee should take precedence over InsufficientFunds (bad_fee={bad_fee})",
+            );
+        }
+    }
+
+    // --- icrc2_approve ---
+    {
+        let account_from = account(3, None);
+        let account_spender = account(4, None);
+        assert_eq!(Nat::from(0u8), env.icrc1_balance_of(account_from));
+
+        for bad_fee in [0, fee - 1, fee + 1, u128::MAX] {
+            let args = ApproveArgs {
+                from_subaccount: account_from.subaccount,
+                spender: account_spender,
+                amount: Nat::from(fee),
+                fee: Some(Nat::from(bad_fee)),
+                created_at_time: None,
+                memo: None,
+                expected_allowance: None,
+                expires_at: None,
+            };
+            assert_eq!(
+                Err(ApproveError::BadFee {
+                    expected_fee: Nat::from(fee)
+                }),
+                env.icrc2_approve(account_from.owner, args),
+                "icrc2_approve: BadFee should take precedence over InsufficientFunds (bad_fee={bad_fee})",
+            );
+        }
+    }
+
+    // --- icrc2_transfer_from ---
+    {
+        let account_from = account(5, None);
+        let account_to = account(6, None);
+        let account_spender = account(7, None);
+
+        // Deposit enough so the approval succeeds, leaving 0 balance after.
+        let _deposit_index = env.deposit(account_from, 2 * fee, None);
+        let approve_args = ApproveArgs {
+            from_subaccount: account_from.subaccount,
+            spender: account_spender,
+            amount: Nat::from(u128::MAX),
+            expected_allowance: None,
+            expires_at: None,
+            fee: None,
+            memo: None,
+            created_at_time: None,
+        };
+        let _approve_index = env.icrc2_approve_or_trap(account_from.owner, approve_args);
+        assert_eq!(Nat::from(0u8), env.icrc1_balance_of(account_from));
+
+        for bad_fee in [0, fee - 1, fee + 1, u128::MAX] {
+            let args = TransferFromArgs {
+                from: account_from,
+                to: account_to,
+                spender_subaccount: account_spender.subaccount,
+                amount: Nat::from(fee),
+                fee: Some(Nat::from(bad_fee)),
+                created_at_time: None,
+                memo: None,
+            };
+            assert_eq!(
+                Err(TransferFromError::BadFee {
+                    expected_fee: Nat::from(fee)
+                }),
+                env.icrc2_transfer_from(account_spender.owner, args),
+                "icrc2_transfer_from: BadFee should take precedence over InsufficientFunds (bad_fee={bad_fee})",
+            );
+        }
+    }
+}
+
 fn test_icrc1_transfer_insufficient_funds_with_params(
     env: &TestEnv,
     set_created_at_time: ShouldSetCreatedAtTime,