From 63a7f5e481330c759719d1816fd6d2495802f56e Mon Sep 17 00:00:00 2001 From: Bellabuks Date: Mon, 22 Jun 2026 07:07:08 +0100 Subject: [PATCH 1/2] fix(CO-07): enforce minimum approval validity window in loyalty_token approve() --- contracts/loyalty_token/src/lib.rs | 127 +++++++++++++++++++---------- 1 file changed, 85 insertions(+), 42 deletions(-) diff --git a/contracts/loyalty_token/src/lib.rs b/contracts/loyalty_token/src/lib.rs index 9418054..4d62356 100644 --- a/contracts/loyalty_token/src/lib.rs +++ b/contracts/loyalty_token/src/lib.rs @@ -4,14 +4,14 @@ //! customer loyalty programme. //! //! ## Token details -//! | Field | Value | -//! |---------|---------------| -//! | Name | Bite Rewards | -//! | Symbol | BITE | -//! | Decimals| 7 | +//! | Field | Value | +//! |---------|----------------| +//! | Name | Bite Rewards | +//! | Symbol | BITE | +//! | Decimals| 7 | //! //! ## Earning BITE -//! The admin (or an authorised minter – typically the Order contract) calls +//! The admin (or an authorised minter - typically the Order contract) calls //! `mint` after an order is marked *Delivered*. A suggested policy is: //! **1 BITE per 10 000 stroops (0.001 XLM) spent**. //! @@ -25,12 +25,14 @@ #![no_std] +/// Minimum ledger window for a non-zero allowance (~24 h at 5-second close time). +const MIN_APPROVAL_VALIDITY_LEDGERS: u32 = 17_280; +/// Maximum ledger window for an allowance (~1 year). +const MAX_APPROVAL_VALIDITY_LEDGERS: u32 = 6_307_200; + use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, String}; -// --------------------------------------------------------------------------- // Storage keys -// --------------------------------------------------------------------------- - #[contracttype] pub enum DataKey { /// The platform admin who controls minting. @@ -41,14 +43,11 @@ pub enum DataKey { TotalSupply, /// Per-account balances. Balance(Address), - /// Allowances: (owner, spender) → (amount, expiration_ledger). + /// Allowances: (owner, spender) ? (amount, expiration_ledger). Allowance(Address, Address), } -// --------------------------------------------------------------------------- // Token metadata (stored once at init) -// --------------------------------------------------------------------------- - #[contracttype] #[derive(Clone)] pub struct TokenMeta { @@ -62,10 +61,7 @@ pub enum MetaKey { Meta, } -// --------------------------------------------------------------------------- // Allowance helper struct -// --------------------------------------------------------------------------- - #[contracttype] #[derive(Clone)] pub struct AllowanceData { @@ -73,24 +69,19 @@ pub struct AllowanceData { pub expiration_ledger: u32, } -// --------------------------------------------------------------------------- // Contract -// --------------------------------------------------------------------------- - #[contract] pub struct LoyaltyToken; #[contractimpl] impl LoyaltyToken { - // ----------------------------------------------------------------------- // Initialisation - // ----------------------------------------------------------------------- - + /// Deploy the BITE token. /// /// # Arguments - /// - `admin` – address with mint authority. - /// - `minter` – optional secondary minter (pass `admin` to disable). + /// - `admin` - address with mint authority. + /// - `minter` - optional secondary minter (pass `admin` to disable). pub fn initialize(env: Env, admin: Address, minter: Address) { if env.storage().instance().has(&DataKey::Admin) { panic!("already initialized"); @@ -109,9 +100,7 @@ impl LoyaltyToken { env.storage().instance().extend_ttl(17_280, 17_280); } - // ----------------------------------------------------------------------- // Admin / Minter actions - // ----------------------------------------------------------------------- /// Mint `amount` BITE to `to`. Only callable by admin or minter. pub fn mint(env: Env, caller: Address, to: Address, amount: i128) { @@ -155,9 +144,7 @@ impl LoyaltyToken { env.storage().instance().extend_ttl(17_280, 17_280); } - // ----------------------------------------------------------------------- // SEP-41 token interface - // ----------------------------------------------------------------------- /// Return the token balance of `account`. pub fn balance(env: Env, account: Address) -> i128 { @@ -177,8 +164,12 @@ impl LoyaltyToken { /// Approve `spender` to transfer up to `amount` on behalf of `from`. /// - /// `expiration_ledger` is the last ledger at which the approval is valid. - /// Pass `0` to revoke. + /// `expiration_ledger` is the **last ledger** at which the approval is valid + /// (inclusive). The allowance returns 0 on `expiration_ledger + 1`. + /// Pass `0` for both `amount` and `expiration_ledger` to revoke. + /// + /// Non-zero approvals must satisfy: + /// `current + MIN_APPROVAL_VALIDITY_LEDGERS <= expiration_ledger <= current + MAX_APPROVAL_VALIDITY_LEDGERS` pub fn approve( env: Env, from: Address, @@ -190,8 +181,14 @@ impl LoyaltyToken { if amount < 0 { panic!("allowance amount cannot be negative"); } - if amount > 0 && expiration_ledger < env.ledger().sequence() { - panic!("expiration_ledger is in the past"); + if amount > 0 { + let current = env.ledger().sequence(); + if expiration_ledger < current.saturating_add(MIN_APPROVAL_VALIDITY_LEDGERS) { + panic!("expiration too soon"); + } + if expiration_ledger > current.saturating_add(MAX_APPROVAL_VALIDITY_LEDGERS) { + panic!("expiration too far in the future"); + } } let data = AllowanceData { amount, @@ -269,9 +266,7 @@ impl LoyaltyToken { Self::do_burn(&env, &from, amount); } - // ----------------------------------------------------------------------- // Token metadata (SEP-41) - // ----------------------------------------------------------------------- pub fn name(env: Env) -> String { let meta: TokenMeta = env.storage().instance().get(&MetaKey::Meta).unwrap(); @@ -295,9 +290,7 @@ impl LoyaltyToken { .unwrap_or(0) } - // ----------------------------------------------------------------------- // Private helpers - // ----------------------------------------------------------------------- fn balance_of(env: &Env, account: &Address) -> i128 { env.storage() @@ -368,6 +361,8 @@ impl LoyaltyToken { match data { None => 0, Some(d) => { + // expiration_ledger is inclusive: the allowance is valid through + // and including that ledger; it returns 0 on expiration_ledger + 1. if env.ledger().sequence() > d.expiration_ledger { 0 } else { @@ -393,10 +388,7 @@ impl LoyaltyToken { } } -// --------------------------------------------------------------------------- // Tests -// --------------------------------------------------------------------------- - #[cfg(test)] mod test { use super::*; @@ -452,8 +444,8 @@ mod test { client.mint(&admin, &alice, &1_000_000); - // Alice approves bob to spend 300_000 for 1000 ledgers. - let expiry = env.ledger().sequence() + 1_000; + // Use MIN_APPROVAL_VALIDITY_LEDGERS to satisfy the minimum window. + let expiry = env.ledger().sequence() + 17_280; client.approve(&alice, &bob, &300_000, &expiry); assert_eq!(client.allowance(&alice, &bob), 300_000); @@ -492,4 +484,55 @@ mod test { let rando = Address::generate(&env); client.mint(&rando, &rando, &1_000_000); } -} + + #[test] + #[should_panic(expected = "expiration too soon")] + fn test_approve_expiry_too_soon_panics() { + let (env, client, admin) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.mint(&admin, &alice, &1_000_000); + // expiry is only 1 ledger ahead - below MIN_APPROVAL_VALIDITY_LEDGERS + let bad_expiry = env.ledger().sequence() + 1; + client.approve(&alice, &bob, &100_000, &bad_expiry); + } + + #[test] + fn test_approve_minimum_valid_expiry() { + let (env, client, admin) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.mint(&admin, &alice, &1_000_000); + let min_expiry = env.ledger().sequence() + 17_280; + client.approve(&alice, &bob, &100_000, &min_expiry); + assert_eq!(client.allowance(&alice, &bob), 100_000); + } + + #[test] + fn test_allowance_valid_at_expiry_ledger() { + let (env, client, admin) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.mint(&admin, &alice, &1_000_000); + let current = env.ledger().sequence(); + let expiry = current + 17_280; + client.approve(&alice, &bob, &100_000, &expiry); + // Advance to the exact expiration ledger - allowance must still be valid (inclusive). + env.ledger().with_mut(|li| li.sequence_number = expiry); + assert_eq!(client.allowance(&alice, &bob), 100_000); + } + + #[test] + fn test_allowance_zero_after_expiry_ledger() { + let (env, client, admin) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.mint(&admin, &alice, &1_000_000); + let current = env.ledger().sequence(); + let expiry = current + 17_280; + client.approve(&alice, &bob, &100_000, &expiry); + // Advance one ledger past expiry - allowance must return 0. + env.ledger().with_mut(|li| li.sequence_number = expiry + 1); + assert_eq!(client.allowance(&alice, &bob), 0); + } +} \ No newline at end of file From 3853e9403710d4ce68fd1055d7be1cdd50c9121f Mon Sep 17 00:00:00 2001 From: Bellabuks Date: Thu, 25 Jun 2026 11:12:58 +0100 Subject: [PATCH 2/2] fix(co-07): resolve merge conflicts - integrate two-step admin transfer with expiry fix --- contracts/loyalty_token/src/lib.rs | 134 +++++++++++++++++++++++++++-- 1 file changed, 129 insertions(+), 5 deletions(-) diff --git a/contracts/loyalty_token/src/lib.rs b/contracts/loyalty_token/src/lib.rs index 4d62356..8476394 100644 --- a/contracts/loyalty_token/src/lib.rs +++ b/contracts/loyalty_token/src/lib.rs @@ -37,13 +37,15 @@ use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, E pub enum DataKey { /// The platform admin who controls minting. Admin, + /// Proposed but not yet confirmed new admin (two-step transfer). + PendingAdmin, /// Optional secondary minter (e.g. the Order contract address). Minter, /// Total tokens in circulation. TotalSupply, /// Per-account balances. Balance(Address), - /// Allowances: (owner, spender) ? (amount, expiration_ledger). + /// Allowances: (owner, spender) -> (amount, expiration_ledger). Allowance(Address, Address), } @@ -76,7 +78,7 @@ pub struct LoyaltyToken; #[contractimpl] impl LoyaltyToken { // Initialisation - + /// Deploy the BITE token. /// /// # Arguments @@ -136,11 +138,51 @@ impl LoyaltyToken { env.storage().instance().extend_ttl(17_280, 17_280); } - /// Transfer the admin role. + /// Step 1: propose a new admin. Does NOT change the active admin. + /// + /// Emits `(symbol_short!("admin"), symbol_short!("proposed"))`. + /// The new admin must call `accept_admin` to take effect. pub fn transfer_admin(env: Env, caller: Address, new_admin: Address) { caller.require_auth(); Self::assert_admin_or_panic(&env, &caller); - env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().instance().set(&DataKey::PendingAdmin, &new_admin); + env.storage().instance().extend_ttl(17_280, 17_280); + env.events().publish( + (symbol_short!("admin"), symbol_short!("proposed")), + (caller, new_admin), + ); + } + + /// Step 2: accept a pending admin transfer. Caller must be `PendingAdmin`. + /// + /// Promotes caller to `Admin`, clears `PendingAdmin`, + /// and emits `(symbol_short!("admin"), symbol_short!("accepted"))`. + pub fn accept_admin(env: Env, caller: Address) { + caller.require_auth(); + let pending: Address = env + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .expect("not pending admin"); + if caller != pending { + panic!("not pending admin"); + } + env.storage().instance().set(&DataKey::Admin, &caller); + env.storage().instance().remove(&DataKey::PendingAdmin); + env.storage().instance().extend_ttl(17_280, 17_280); + env.events().publish( + (symbol_short!("admin"), symbol_short!("accepted")), + caller, + ); + } + + /// Cancel a pending admin transfer. Only the current admin may call this. + /// + /// Clears `PendingAdmin` without changing the active admin. + pub fn cancel_transfer(env: Env, caller: Address) { + caller.require_auth(); + Self::assert_admin_or_panic(&env, &caller); + env.storage().instance().remove(&DataKey::PendingAdmin); env.storage().instance().extend_ttl(17_280, 17_280); } @@ -485,6 +527,88 @@ mod test { client.mint(&rando, &rando, &1_000_000); } + // Two-step admin transfer tests + + #[test] + fn test_transfer_admin_does_not_change_active_admin() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + // propose but do not accept + client.transfer_admin(&admin, &new_admin); + // old admin can still mint + let user = Address::generate(&env); + client.mint(&admin, &user, &100); + assert_eq!(client.balance(&user), 100); + } + + #[test] + fn test_happy_path_propose_accept_old_admin_locked_out() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + + // Step 1: propose + client.transfer_admin(&admin, &new_admin); + + // Step 2: accept + client.accept_admin(&new_admin); + + // new admin can mint + let user = Address::generate(&env); + client.mint(&new_admin, &user, &500); + assert_eq!(client.balance(&user), 500); + } + + #[test] + #[should_panic(expected = "unauthorized: admin only")] + fn test_old_admin_cannot_mint_after_accept() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + client.transfer_admin(&admin, &new_admin); + client.accept_admin(&new_admin); + + // old admin should fail + let user = Address::generate(&env); + client.mint(&admin, &user, &100); + } + + #[test] + #[should_panic(expected = "not pending admin")] + fn test_accept_admin_wrong_address_panics() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + let rando = Address::generate(&env); + + client.transfer_admin(&admin, &new_admin); + // rando tries to accept + client.accept_admin(&rando); + } + + #[test] + fn test_cancel_transfer_clears_pending_admin() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + + // propose then cancel + client.transfer_admin(&admin, &new_admin); + client.cancel_transfer(&admin); + + // old admin still active + let user = Address::generate(&env); + client.mint(&admin, &user, &100); + assert_eq!(client.balance(&user), 100); + } + + #[test] + #[should_panic(expected = "not pending admin")] + fn test_accept_after_cancel_panics() { + let (env, client, admin) = setup(); + let new_admin = Address::generate(&env); + client.transfer_admin(&admin, &new_admin); + client.cancel_transfer(&admin); + // accept should now panic since PendingAdmin is cleared + client.accept_admin(&new_admin); + } + #[test] #[should_panic(expected = "expiration too soon")] fn test_approve_expiry_too_soon_panics() { @@ -535,4 +659,4 @@ mod test { env.ledger().with_mut(|li| li.sequence_number = expiry + 1); assert_eq!(client.allowance(&alice, &bob), 0); } -} \ No newline at end of file +}