diff --git a/docs/authorization.md b/docs/authorization.md index bc87908d..b064c52c 100644 --- a/docs/authorization.md +++ b/docs/authorization.md @@ -9,6 +9,7 @@ Only the configured administrator address can perform administrative operations. * `pause` / `unpause` * `add_caller` / `remove_caller` * `transfer_admin` +* `set_limits` ## Admin Ownership Transfer (Two-Step) @@ -39,7 +40,10 @@ Recommended custody practices: * Prefer a multisig or timelock arrangement for any administrative action that would materially affect operations. * Keep the admin key materially segregated from day-to-day operational keys and rotate or recover it through a documented process. -Security note: compromise of the admin key can pause the contract and change the allowlist, but it cannot directly withdraw escrowed funds from the contract. +Security note: compromise of the admin key can pause the contract, change the +allowlist, and change operational limits, but it cannot directly withdraw +escrowed funds from the contract. A new global escrow limit cannot be set below +the amount already held in escrow. ## Privileged Callers Allowlist The contract maintains an allowlist of privileged callers who are authorized to lock funds and create new escrow transfers. diff --git a/docs/entrypoint-reference.md b/docs/entrypoint-reference.md index b20be748..e6f3d529 100644 --- a/docs/entrypoint-reference.md +++ b/docs/entrypoint-reference.md @@ -116,11 +116,24 @@ Returns a bounded page of transfer records ordered by ascending transfer id. ## Operational Configuration & Resource Queries +### `set_limits(limits: ConfiguredLimits) -> Result<(), Error>` + +Updates the operational limits enforced for new transfers and paginated queries. + +* **Authorization**: Current admin (`admin.require_auth()`) +* **Effect**: Stores the new `max_amount`, `max_expiry_window`, + `max_total_escrowed`, and `max_page_size` values in instance storage. +* **Validation**: Every value must be positive, `max_amount` cannot exceed + `max_total_escrowed`, and `max_total_escrowed` cannot be lower than the + amount currently held in escrow. +* **Events**: Emits `limits_changed` with the admin address and the complete + old and new [`ConfiguredLimits`](data-types.md#configuredlimits) values. + Supplying the current configuration is a no-op and emits no event. +* **Errors**: `NotInitialized`, `InvalidLimits`, or `CooldownNotElapsed`. + ### `get_limits() -> ConfiguredLimits` Returns the contract's configured operational limits. * **Authorization**: None (public view, callable pre/post-initialization) * **Returns**: [`ConfiguredLimits`](data-types.md#configuredlimits) containing `max_amount`, `max_expiry_window`, `max_total_escrowed`, and `max_page_size`. - - diff --git a/docs/event-reference.md b/docs/event-reference.md index 235eaf4e..f10e2d0d 100644 --- a/docs/event-reference.md +++ b/docs/event-reference.md @@ -14,6 +14,7 @@ This document describes all events emitted by the RemitFlow smart contract, deta | `created` | `("created", id: u64)` | `(from: Address, recipient: Address, amount: i128, expiry: u64)` | A new transfer is created and funds escrowed. | | `claimed` | `("claimed", id: u64)` | `(recipient: Address, amount: i128)` | Recipient claims escrowed transfer. | | `cancelled` | `("cancelled", id: u64)` | `(from: Address, amount: i128)` | Sender cancels and receives a refund for an expired transfer. | +| `limits_changed` | `("limits_changed",)` | `(admin: Address, old_limits: ConfiguredLimits, new_limits: ConfiguredLimits)` | Admin changes the contract's operational limits. | | `admin_transfer_started` | `("admin_transfer_started",)` | `(current_admin: Address, pending_admin: Address)` | Admin initiates ownership transfer. | | `admin_transfer_completed` | `("admin_transfer_completed",)` | `(old_admin: Address, new_admin: Address)` | Pending admin accepts ownership transfer. | diff --git a/docs/testing-guide.md b/docs/testing-guide.md index 39204946..e4573e5e 100644 --- a/docs/testing-guide.md +++ b/docs/testing-guide.md @@ -38,6 +38,7 @@ Admin-only guards are authorization checks that restrict certain contract operat 1. **initialize()** - Sets up the contract with an admin and token address 2. **pause()** - Blocks creation of new transfers 3. **unpause()** - Re-enables transfer creation +4. **set_limits()** - Updates operational transfer and pagination limits ### Testing Admin Authorization @@ -95,11 +96,24 @@ fn test_unpause_requires_admin_auth() { } ``` +#### Limit-Change Guards +```rust +#[test] +fn test_set_limits_requires_admin_auth() { + // Verifies set_limits() checks admin.require_auth() and does not mutate state +} + +#[test] +fn test_set_limits_by_admin_succeeds() { + // Verifies the admin can update ConfiguredLimits when authorized +} +``` + #### Operational State Guards ```rust #[test] fn test_admin_operations_require_initialization() { - // Ensures pause, unpause, and other admin ops fail when contract is not initialized + // Ensures pause, unpause, set_limits, and other admin ops fail when contract is not initialized } ``` diff --git a/docs/unit-tests.md b/docs/unit-tests.md index 0da914fa..a904a7bd 100644 --- a/docs/unit-tests.md +++ b/docs/unit-tests.md @@ -96,6 +96,11 @@ These tests specifically validate authorization mechanisms for admin-only operat - **test_unpause_requires_admin_auth**: Verifies unpause() enforces admin authentication - **test_admin_guard_on_pause_with_mock_all_auths**: Tests pause/unpause with authentication mocking +### Limit-Change Authorization +- **test_set_limits_requires_admin_auth**: Verifies `set_limits()` rejects callers that do not satisfy `admin.require_auth()`, leaving stored limits unchanged +- **test_set_limits_by_admin_succeeds**: Confirms the admin can update operational limits when authorization is present +- **test_admin_operations_require_initialization**: Also covers `set_limits()` returning `NotInitialized` before `initialize()` + ### Operational Constraints - **test_admin_operations_require_initialization**: Ensures admin operations fail if contract not initialized - **test_non_admin_cannot_pause_twice**: Tests state consistency for pause operations diff --git a/src/error.rs b/src/error.rs index e64f6219..e1b80430 100644 --- a/src/error.rs +++ b/src/error.rs @@ -52,5 +52,6 @@ pub enum Error { /// The number of operations in a atch_operations call exceeds /// MAX_BATCH_SIZE. BatchTooLarge = 21, + /// One or more configured operational limits are invalid. + InvalidLimits = 22, } - diff --git a/src/events.rs b/src/events.rs index 8adf1935..4650587a 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1,5 +1,7 @@ use soroban_sdk::{Address, Env, Symbol}; +use crate::types::ConfiguredLimits; + /// Publish an event recording contract initialization. pub fn init(env: &Env, admin: &Address, token: &Address) { let topics = (Symbol::new(env, "init"),); @@ -68,3 +70,17 @@ pub fn admin_transfer_completed(env: &Env, old_admin: &Address, new_admin: &Addr env.events() .publish(topics, (old_admin.clone(), new_admin.clone())); } + +/// Publish an event recording an administrator's operational-limit update. +pub fn limits_changed( + env: &Env, + admin: &Address, + old_limits: &ConfiguredLimits, + new_limits: &ConfiguredLimits, +) { + let topics = (Symbol::new(env, "limits_changed"),); + env.events().publish( + topics, + (admin.clone(), old_limits.clone(), new_limits.clone()), + ); +} diff --git a/src/lib.rs b/src/lib.rs index 750de6ec..1bcce32b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -53,6 +53,15 @@ pub const MAX_PAGE_SIZE: u32 = 100; /// Maximum number of operations allowed in a single batch_operations call. pub const MAX_BATCH_SIZE: u32 = 50; +fn default_limits() -> ConfiguredLimits { + ConfiguredLimits { + max_amount: MAX_AMOUNT, + max_expiry_window: MAX_EXPIRY_WINDOW, + max_total_escrowed: MAX_TOTAL_ESCROWED, + max_page_size: MAX_PAGE_SIZE, + } +} + fn require_external_address(env: &Env, address: &Address) -> Result<(), Error> { if *address == env.current_contract_address() { return Err(Error::InvalidAddress); @@ -137,6 +146,7 @@ impl RemitFlowContract { storage::set_token(&env, &token); storage::set_counter(&env, 0); storage::set_initialized_at(&env, env.ledger().timestamp()); + storage::set_limits(&env, &default_limits()); storage::extend_instance(&env); events::init(&env, &admin, &token); Ok(()) @@ -209,23 +219,24 @@ impl RemitFlowContract { if amount <= 0 { return Err(Error::InvalidAmount); } + let limits = storage::get_limits(&env).unwrap_or_else(default_limits); if storage::get_account_op_count(&env, &from) >= MAX_ACCOUNT_OPS { return Err(Error::AccountLimitReached); } - if amount > MAX_AMOUNT { + if amount > limits.max_amount { return Err(Error::AmountTooLarge); } let total_escrowed = storage::get_total_escrowed(&env); let updated_total = math::checked_add_amount(total_escrowed, amount).ok_or(Error::AmountTooLarge)?; - if updated_total > MAX_TOTAL_ESCROWED { + if updated_total > limits.max_total_escrowed { return Err(Error::EscrowCapReached); } let now = env.ledger().timestamp(); if expiry <= now { return Err(Error::InvalidExpiry); } - if expiry - now > MAX_EXPIRY_WINDOW { + if expiry - now > limits.max_expiry_window { return Err(Error::ExpiryTooFar); } if from == recipient { @@ -346,7 +357,11 @@ impl RemitFlowContract { let last = storage::get_counter(&env); let mut page = Vec::new(&env); let mut id = start_id.max(1); - let page_size = limit.min(MAX_PAGE_SIZE); + let page_size = limit.min( + storage::get_limits(&env) + .unwrap_or_else(default_limits) + .max_page_size, + ); while id <= last && page.len() < page_size { if let Some(transfer) = storage::get_transfer(&env, id) { page.push_back(transfer); @@ -485,13 +500,35 @@ impl RemitFlowContract { storage::get_pending_admin(&env) } - pub fn get_limits(_env: Env) -> ConfiguredLimits { - ConfiguredLimits { - max_amount: MAX_AMOUNT, - max_expiry_window: MAX_EXPIRY_WINDOW, - max_total_escrowed: MAX_TOTAL_ESCROWED, - max_page_size: MAX_PAGE_SIZE, + pub fn set_limits(env: Env, limits: ConfiguredLimits) -> Result<(), Error> { + let admin = storage::get_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + + let old_limits = storage::get_limits(&env).unwrap_or_else(default_limits); + if limits == old_limits { + return Ok(()); } + require_cooldown(&env)?; + + if limits.max_amount <= 0 + || limits.max_expiry_window == 0 + || limits.max_total_escrowed <= 0 + || limits.max_page_size == 0 + || limits.max_amount > limits.max_total_escrowed + || limits.max_total_escrowed < storage::get_total_escrowed(&env) + { + return Err(Error::InvalidLimits); + } + + storage::set_limits(&env, &limits); + record_privileged_call(&env); + storage::extend_instance(&env); + events::limits_changed(&env, &admin, &old_limits, &limits); + Ok(()) + } + + pub fn get_limits(env: Env) -> ConfiguredLimits { + storage::get_limits(&env).unwrap_or_else(default_limits) } /// Sweeps an expired transfer, returning the escrowed funds to the original sender. @@ -533,4 +570,3 @@ impl RemitFlowContract { Ok(()) } } - diff --git a/src/storage.rs b/src/storage.rs index 588c5082..824835da 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -1,6 +1,6 @@ use soroban_sdk::{contracttype, Address, Env}; -use crate::types::Transfer; +use crate::types::{ConfiguredLimits, Transfer}; /// Number of ledgers used as the threshold before bumping instance TTL. pub const INSTANCE_BUMP_THRESHOLD: u32 = 518_400; @@ -20,8 +20,8 @@ pub const PERSISTENT_BUMP_AMOUNT: u32 = 535_680; /// # Collision safety /// Soroban serialises #[contracttype] enum keys as an XDR ScVec whose /// first element is the variant name as a Symbol. Because the name string is -/// part of the on-chain key, no two distinct variants — even with identical -/// payloads — can ever collide. Separating instance and persistent keys into +/// part of the on-chain key, no two distinct variants - even with identical +/// payloads - can ever collide. Separating instance and persistent keys into /// two enums makes a mis-routed write (e.g. passing an [InstanceKey] to the /// persistent store) a compile error rather than a silent bug. #[contracttype] @@ -48,6 +48,8 @@ pub enum InstanceKey { InitializedAt, /// Timestamp of the most recent privileged administrative call. LastPrivilegedCall, + /// Mutable operational limits enforced by the contract. + Limits, } /// Keys for values held in **persistent** storage. @@ -193,6 +195,16 @@ pub fn set_last_privileged_call(env: &Env, timestamp: u64) { .set(&InstanceKey::LastPrivilegedCall, ×tamp); } +/// Read the configured operational limits, if they have been stored. +pub fn get_limits(env: &Env) -> Option { + env.storage().instance().get(&InstanceKey::Limits) +} + +/// Persist the operational limits in instance storage. +pub fn set_limits(env: &Env, limits: &ConfiguredLimits) { + env.storage().instance().set(&InstanceKey::Limits, limits); +} + // --------------------------------------------------------------------------- // Persistent storage helpers // --------------------------------------------------------------------------- diff --git a/src/test.rs b/src/test.rs index 187a0a30..040681ef 100644 --- a/src/test.rs +++ b/src/test.rs @@ -9,7 +9,8 @@ use crate::test_utils::{ TestFixture, DEFAULT_EXPIRY_OFFSET, DEFAULT_SENDER_BALANCE, DEFAULT_TRANSFER_AMOUNT, }; use crate::types::{ - BatchOperation, BatchOperationResult, ClaimTransferOperation, CreateTransferOperation, Status, + BatchOperation, BatchOperationResult, ClaimTransferOperation, ConfiguredLimits, + CreateTransferOperation, Status, }; use crate::{RemitFlowContract, RemitFlowContractClient}; @@ -60,6 +61,84 @@ fn setup<'a>() -> TestFixture<'a> { TestFixture::new() } +#[test] +fn test_get_limits_returns_defaults() { + let s = setup(); + + assert_eq!( + s.client.get_limits(), + ConfiguredLimits { + max_amount: crate::MAX_AMOUNT, + max_expiry_window: crate::MAX_EXPIRY_WINDOW, + max_total_escrowed: crate::MAX_TOTAL_ESCROWED, + max_page_size: crate::MAX_PAGE_SIZE, + } + ); +} + +#[test] +fn test_set_limits_emits_change_event() { + let s = setup(); + let old_limits = s.client.get_limits(); + let new_limits = ConfiguredLimits { + max_amount: 500, + max_expiry_window: 2_000, + max_total_escrowed: 2_000, + max_page_size: 25, + }; + + s.client.set_limits(&new_limits); + + assert_eq!(s.client.get_limits(), new_limits); + let events = s.env.events().all(); + let event = events.last().unwrap(); + let topic: soroban_sdk::Symbol = event.1.get(0).unwrap().into_val(&s.env); + assert_eq!(topic, soroban_sdk::Symbol::new(&s.env, "limits_changed")); + assert_eq!(event.1.len(), 1); + + let (admin, emitted_old, emitted_new): (Address, ConfiguredLimits, ConfiguredLimits) = + event.2.into_val(&s.env); + assert_eq!(admin, s.admin); + assert_eq!(emitted_old, old_limits); + assert_eq!(emitted_new, new_limits); +} + +#[test] +fn test_updated_limits_are_enforced() { + let s = setup(); + let limits = ConfiguredLimits { + max_amount: 250, + max_expiry_window: crate::MAX_EXPIRY_WINDOW, + max_total_escrowed: crate::MAX_TOTAL_ESCROWED, + max_page_size: crate::MAX_PAGE_SIZE, + }; + s.client.set_limits(&limits); + + let result = s + .client + .try_create_transfer(&s.from, &s.recipient, &251, &s.future_expiry()); + + assert_eq!(result, Err(Ok(crate::error::Error::AmountTooLarge))); +} + +#[test] +fn test_set_limits_rejects_invalid_configuration() { + let s = setup(); + let original = s.client.get_limits(); + let invalid = ConfiguredLimits { + max_amount: 0, + max_expiry_window: original.max_expiry_window, + max_total_escrowed: original.max_total_escrowed, + max_page_size: original.max_page_size, + }; + + assert_eq!( + s.client.try_set_limits(&invalid), + Err(Ok(crate::error::Error::InvalidLimits)) + ); + assert_eq!(s.client.get_limits(), original); +} + #[test] fn test_batch_operations_executes_successful_batch_in_order() { let s = setup(); @@ -824,6 +903,44 @@ fn test_add_caller_requires_admin_auth() { assert!(res.is_err()); } +#[test] +fn test_set_limits_requires_admin_auth() { + let env = Env::default(); + let admin = Address::generate(&env); + let (token, _, _) = create_token(&env, &admin); + + let contract_id = env.register_contract(None, RemitFlowContract); + let client = RemitFlowContractClient::new(&env, &contract_id); + env.mock_all_auths(); + client.initialize(&admin, &token); + let original = client.get_limits(); + env.set_auths(&[]); + + let new_limits = ConfiguredLimits { + max_amount: 500, + max_expiry_window: 2_000, + max_total_escrowed: 2_000, + max_page_size: 25, + }; + let res = client.try_set_limits(&new_limits); + assert!(res.is_err()); + assert_eq!(client.get_limits(), original); +} + +#[test] +fn test_set_limits_by_admin_succeeds() { + let s = setup(); + let new_limits = ConfiguredLimits { + max_amount: 500, + max_expiry_window: 2_000, + max_total_escrowed: 2_000, + max_page_size: 25, + }; + + s.client.set_limits(&new_limits); + assert_eq!(s.client.get_limits(), new_limits); +} + #[test] fn test_pause_requires_admin_auth() { let s = setup(); @@ -995,6 +1112,15 @@ fn test_admin_operations_require_initialization() { let res = client.try_unpause(); assert_eq!(res, Err(Ok(crate::error::Error::NotInitialized))); + + let limits = ConfiguredLimits { + max_amount: 500, + max_expiry_window: 2_000, + max_total_escrowed: 2_000, + max_page_size: 25, + }; + let res = client.try_set_limits(&limits); + assert_eq!(res, Err(Ok(crate::error::Error::NotInitialized))); } // --- Arithmetic boundary tests --- @@ -2237,7 +2363,10 @@ fn test_sweep_expired_success() { let transfer = s.client.get_transfer(&id); assert_eq!(transfer.status, Status::Cancelled); - assert_eq!(s.token_client().balance(&s.from), initial_balance + DEFAULT_TRANSFER_AMOUNT); + assert_eq!( + s.token_client().balance(&s.from), + initial_balance + DEFAULT_TRANSFER_AMOUNT + ); } #[test]