From b98bb5b5cd01e8dcbd0095bc68c740ef7ddc9cf6 Mon Sep 17 00:00:00 2001 From: Eras256 Date: Thu, 20 Aug 2026 09:36:10 -0600 Subject: [PATCH 1/2] fix(fee-abstraction): drop Lazy-mode expiration check that validates the wrong value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit collect_fee()'s Lazy branch only re-approves when the existing allowance already covers max_fee_amount. When it does, expiration_ledger is never applied to anything real — the else branch instead ran validate_expiration_ledger(e, expiration_ledger), which only checks that the *parameter passed to this call* is in the future. It has no relationship to the real allowance's actual expiration, which was set by whichever prior approve() call established it and isn't even readable through TokenClient (only the amount is, via allowance()). Two consequences, demonstrated by two new tests: - False rejection: a genuinely valid, non-expired real allowance (100 units until ledger 200, current ledger 101) still panicked with Error(Contract, #5006) if the unrelated expiration_ledger argument happened to be less than the current ledger sequence. - No real effect either way: an expiration_ledger nowhere near the real allowance's actual expiration (e.g. 9999 against a real expiration of 200) was accepted just as readily — there was never a tightening or loosening effect to preserve. The SAC's own transfer_from expiry enforcement already protects the real allowance on the line right after this branch; this check wasn't adding coverage transfer_from didn't already provide, only a false-rejection surface. Dropped the call in the else branch. validate_expiration_ledger itself stays public (still has its own direct unit test) — this only removes the one call site that was checking it against the wrong thing. The old collect_fee_with_lazy_approval_expired_ledger_panics test encoded the bug as expected behavior; replaced with two tests that assert the corrected behavior in both directions and keep the exact same setup so the fix's before/after is legible from the diff. Fixes #840. Co-Authored-By: Claude Sonnet 5 --- packages/fee-abstraction/src/storage.rs | 10 ++-- packages/fee-abstraction/src/test.rs | 69 +++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/packages/fee-abstraction/src/storage.rs b/packages/fee-abstraction/src/storage.rs index b9b9acbb8..6b2114ed3 100644 --- a/packages/fee-abstraction/src/storage.rs +++ b/packages/fee-abstraction/src/storage.rs @@ -194,11 +194,13 @@ pub fn collect_fee( &max_fee_amount, &expiration_ledger, ); - } else { - // assuming that in the other cases the expiration ledger is validated in - // `token.approve()` - validate_expiration_ledger(e, expiration_ledger); } + // Else: the existing allowance already covers max_fee_amount, so + // no approve() call is issued here, and `expiration_ledger` is + // never applied to anything. The real allowance's expiration was + // already set by whichever prior approve() call established it, + // and the SAC enforces that on `transfer_from` below — there is + // nothing left to validate `expiration_ledger` against. } } diff --git a/packages/fee-abstraction/src/test.rs b/packages/fee-abstraction/src/test.rs index 4b704da4a..27d9b4770 100644 --- a/packages/fee-abstraction/src/test.rs +++ b/packages/fee-abstraction/src/test.rs @@ -195,9 +195,20 @@ fn collect_fee_with_lazy_approval_lower_previous() { assert_eq!(balance, 20); } +// Regression test for #840. The `expiration_ledger` parameter has no +// relationship to the real, on-chain allowance's actual expiration — that +// expiration was already fixed by whichever prior approve() call set it, and +// TokenClient doesn't even expose it as a readable value, only the amount via +// `allowance()`. Before the fix, this exact scenario (a genuinely valid, +// non-expired allowance of 100 until ledger 200, current ledger 101) panicked +// with `Error(Contract, #5006)` purely because the *unrelated* parameter +// (100) happened to be less than the current ledger sequence (101) — a false +// rejection against a real allowance that still had 99 ledgers of validity +// left. After the fix, the call succeeds: the SAC's own `transfer_from` +// expiry enforcement is what actually protects the real allowance, and it's +// not fooled by this parameter either way. #[test] -#[should_panic(expected = "Error(Contract, #5006)")] -fn collect_fee_with_lazy_approval_expired_ledger_panics() { +fn collect_fee_with_lazy_approval_succeeds_regardless_of_expiration_ledger_param() { let e = Env::default(); e.mock_all_auths_allowing_non_root_auth(); @@ -209,7 +220,7 @@ fn collect_fee_with_lazy_approval_expired_ledger_panics() { let max_fee_amount = 50; let token_client = TokenClient::new(&e, &token_address); - // approve enough (100 > max_fee_amount) till ledger 200 + // approve enough (100 > max_fee_amount) till ledger 200 — genuinely valid token_client.approve(&user, &contract_address, &100, &200); e.ledger().set_sequence_number(101); @@ -220,12 +231,62 @@ fn collect_fee_with_lazy_approval_expired_ledger_panics() { &token_address, 20, max_fee_amount, - 100, // expiration_ledger < 101 + 100, // < current ledger 101, previously caused a false #5006 panic + &user, + &recipient, + FeeAbstractionApproval::Lazy, + ); + }); + + // The real allowance moved only by the fee actually spent (100 - 20 = + // 80) — no re-approve happened, and the unrelated expiration_ledger + // param neither blocked the call nor changed anything about the real + // allowance's own expiration. + let allowance = token_client.allowance(&user, &contract_address); + assert_eq!(allowance, 80); + + let balance = token_client.balance(&recipient); + assert_eq!(balance, 20); +} + +// Second half of #840: proves the parameter is inert in the other +// direction too. An arbitrary, disconnected expiration_ledger — one that +// isn't even close to the real allowance's actual expiration of 200 — +// changes nothing about whether the call succeeds. There was never a +// tightening or loosening effect to preserve; the parameter simply doesn't +// reach the real allowance in the Lazy/sufficient-allowance branch. +#[test] +fn collect_fee_with_lazy_approval_ignores_unrelated_expiration_ledger_param() { + let e = Env::default(); + e.mock_all_auths_allowing_non_root_auth(); + + let contract_address = e.register(MockContract, ()); + let user = Address::generate(&e); + let token_address = e.register(MockToken, (user.clone(),)); + let recipient = Address::generate(&e); + + let max_fee_amount = 50; + let token_client = TokenClient::new(&e, &token_address); + + // Real, on-chain allowance: 100 units, valid until ledger 200. + token_client.approve(&user, &contract_address, &100, &200); + e.ledger().set_sequence_number(101); + + e.as_contract(&contract_address, || { + collect_fee( + &e, + &token_address, + 10, + max_fee_amount, + 9999, // arbitrary, unrelated to the real allowance's expiration of 200 &user, &recipient, FeeAbstractionApproval::Lazy, ); }); + + let remaining = token_client.allowance(&user, &contract_address); + assert_eq!(remaining, 90, "only the transfer_from spend (10) should move the real allowance"); } #[test] From b68a9abbc74ddc5d8e907189255e1d68277a05d9 Mon Sep 17 00:00:00 2001 From: Eras256 Date: Mon, 24 Aug 2026 11:00:08 -0600 Subject: [PATCH 2/2] fix(fee-abstraction): use e.mock_all_auths() in sweep_token_success coderabbitai flagged that this test doesn't exercise non-root authorization machinery (sweep runs inside e.as_contract(...) with no external require_auth() involved) and should use the standard e.mock_all_auths() mock instead of the non-root variant. --- packages/fee-abstraction/src/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/fee-abstraction/src/test.rs b/packages/fee-abstraction/src/test.rs index 27d9b4770..d160c8e8c 100644 --- a/packages/fee-abstraction/src/test.rs +++ b/packages/fee-abstraction/src/test.rs @@ -516,7 +516,7 @@ fn validate_expiration_ledger_past() { #[test] fn sweep_token_success() { let e = Env::default(); - e.mock_all_auths_allowing_non_root_auth(); + e.mock_all_auths(); let contract_address = e.register(MockContract, ()); let recipient = Address::generate(&e);