Expired Timestamp Returned Without Expiration Check

[caocongyu]

Users will experience confusion and poor UX when querying asset expiration affecting frontend applications as the expires() function returns expired timestamps without indicating expiration status.

Summary

The missing expiration validation in expires() will cause a poor user experience for applications querying asset expiration as the function returns raw timestamps without checking if they represent past dates, requiring callers to perform additional timestamp comparisons.

Root Cause

In oracle/src/assets.rs:82, the expires() function returns stored timestamps directly without validating if they are in the past:

pub fn expires(e: &Env, asset: Asset) -> Option<u64> {
    let asset_index = resolve_asset_index(e, &asset);
    if asset_index.is_none() {
        e.panic_with_error(Error::AssetMissing);
    }
    let expirations = load_expiration_records(e);
    expirations.get(asset_index.unwrap())  // ⚠️ No expiration check
}

This forces every caller to implement their own expiration logic by comparing the returned timestamp with the current time, creating inconsistent implementations across different applications.

Internal Pre-conditions

1. Admin needs to call set_expiration() to set an asset expiration timestamp to any value
2. Time needs to advance beyond the expiration timestamp (ledger timestamp > expiration timestamp)

External Pre-conditions

1. Frontend application needs to call expires() to display asset status
2. Current ledger time needs to be greater than the stored expiration timestamp

Attack Path

1. Admin calls set_expiration(asset_A, 1730419200000) on 2025-11-01 to set 30-day expiration (expires 2025-12-01)
2. Time advances to 2025-12-15 (14 days past expiration)
3. User calls expires(asset_A) to check if asset is still active
4. Function returns Some(1730419200000) (the expired timestamp from 2025-12-01)
5. User receives a timestamp without context about whether it means "expires on" or "expired on"
6. User must manually implement: if timestamp < current_time { /* expired */ } else { /* active */ }
7. Different applications implement this check inconsistently (some use <, some use <=, some check seconds vs milliseconds)
8. Frontend displays confusing information: "Expires: 2025-12-01" when it should show "Expired on: 2025-12-01" or "Status: Expired"

Impact

Users experience poor UX and frontend applications must implement additional logic to interpret expiration status. The impact is limited to usability concerns:

• No funds are at risk
• Business logic in extend_ttl() correctly handles expired timestamps
• Contract state remains valid
• Only affects query interface convenience

Why Low Severity:

• Expiration checking logic in extend_ttl() already exists and works correctly
• No economic loss even if users misinterpret expiration status
• Common design pattern (lazy deletion) used for storage optimization
• Similar to other protocols (Uniswap V3 positions, Compound liquidated accounts) that don't automatically clear expired records

PoC

Add this test to oracle/src/tests/assets_test.rs:

#[cfg(test)]
mod tests {
    use super::*;
    use soroban_sdk::Env;

    #[test]
    fn test_expires_returns_past_timestamp() {
        let env = Env::default();
        let contract = setup_contract(&env);
        
        // Set current time: 2025-11-10
        env.ledger().set_timestamp(1731196800000);
        
        // Set asset expiration: 2025-11-01 (9 days ago, already expired)
        let expired_time = 1730419200000u64;
        contract.set_expiration(&Asset::BTC, expired_time);
        
        // Query expiration
        let result = contract.expires(&env, &Asset::BTC);
        
        // Current behavior: Returns expired timestamp without indication
        assert_eq!(result, Some(expired_time));  // ❌ Confusing for users
        
        // Expected behavior for better UX: Return None for expired assets
        // assert_eq!(result, None);  // ✅ Clear signal of expiration
    }
    
    #[test]
    fn test_user_must_implement_own_check() {
        let env = Env::default();
        let contract = setup_contract(&env);
        
        env.ledger().set_timestamp(1731196800000);
        let expired_time = 1730419200000u64;
        contract.set_expiration(&Asset::BTC, expired_time);
        
        // User code must do this check manually
        let expiration = contract.expires(&env, &Asset::BTC);
        let is_expired = match expiration {
            Some(timestamp) => timestamp < env.ledger().timestamp(),
            None => true,
        };
        
        assert!(is_expired);  // User burden to implement this logic
    }
    
    #[test]
    fn test_business_logic_handles_correctly() {
        let env = Env::default();
        let contract = setup_contract(&env);
        
        // Set expired asset
        env.ledger().set_timestamp(1731196800000);
        contract.set_expiration(&Asset::BTC, 1730419200000);
        
        // extend_ttl() correctly resets expired timestamp
        contract.extend_ttl(&user, &Asset::BTC, 50_0000000);
        
        // Verify: New expiration = current_time + 50 days (correct)
        let new_exp = contract.expires(&env, &Asset::BTC).unwrap();
        let expected = 1731196800000 + (50 * 86400000);
        assert_eq!(new_exp, expected);  // ✅ Business logic correct
    }
}

Run with: cargo test test_expires_returns_past_timestamp

Mitigation

Option 1 (Recommended): Add expiration validation to return None for expired assets:

pub fn expires(e: &Env, asset: Asset) -> Option<u64> {
    let asset_index = resolve_asset_index(e, &asset);
    if asset_index.is_none() {
        e.panic_with_error(Error::AssetMissing);
    }
    
    let expirations = load_expiration_records(e);
    let expiration = expirations.get(asset_index.unwrap())?;
    
+   // Return None if expired for better UX
+   let now = timestamps::ledger_timestamp(&e);
+   if expiration < now {
+       return None;
+   }
    
    Some(expiration)
}

Benefits:

• Clear signal: None = expired or not set
• No manual timestamp comparison needed
• ~1 additional comparison operation (~100 gas)
• Backward compatible (return type unchanged)

Option 2: Add helper functions while keeping expires() unchanged:

/// Returns true if asset has expired or has no expiration set
pub fn is_expired(e: &Env, asset: Asset) -> bool {
    let expiration = self.expires(e, asset.clone());
    match expiration {
        None => true,
        Some(exp) => exp < timestamps::ledger_timestamp(&e)
    }
}

/// Returns remaining days until expiration (0 if expired)
pub fn remaining_days(e: &Env, asset: Asset) -> u32 {
    let expiration = self.expires(e, asset.clone());
    match expiration {
        None => 0,
        Some(exp) => {
            let now = timestamps::ledger_timestamp(&e);
            if exp <= now {
                0
            } else {
                ((exp - now) / 86400000) as u32
            }
        }
    }
}

Benefits:

• Provides multiple query options for different use cases
• Maintains backward compatibility completely
• More flexible but increases API surface

Option 3: Document existing behavior (minimum effort):

Add documentation to clarify that callers must check expiration themselves:

pub fn expires(e: &Env, asset: Asset) -> Option<u64> { ... }