From 42518ec2880a6fa4b501b2fcaed0ccea878bbd0e Mon Sep 17 00:00:00 2001 From: tolulopedd26 <261160352+tolulopedd26@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:28:36 +0100 Subject: [PATCH] Fix missing require_auth, add checked arithmetic, TTL policy and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SC-42: authorization audit Found and fixed a critical authentication gap in two crates. Ten entrypoints took a `caller: Address`, compared it against a registrar allowlist, the asset owner, the stored admin, or the approver set — and never called `caller.require_auth()`. Because `caller` is chosen by whoever builds the transaction, every one of those checks was satisfied by simply naming a privileged address. assetsup: register_asset, update_asset_metadata, transfer_asset_ownership, retire_asset. transfer_asset_ownership was a direct asset-theft path: name the current owner, receive the asset. multisig-transfer: configure_approval_rule, create_transfer_request, approve_transfer_request, reject_transfer_request, cancel_transfer_request. Approvals were forgeable by naming an authorized approver, and the approval threshold rewritable by naming the admin. Also closed the initialize front-running window in assetsup, contrib, multisig-transfer and asset-maintenance, and gated four asset-maintenance entrypoints that had no authorization at all — anyone could write warranty terms, file claims, and raise alerts, forging the audit evidence that contract exists to provide. execute_transfer stays permissionless, matching multisig-wallet's execute_*: the approvers already made the decision. Its unused `caller` argument now says so in a comment, since an authentic-looking unused parameter is what made this class of bug easy to miss. 23 negative tests run without mock_all_auths, including one proving an attacker's own valid signature does not authorize a transfer they named someone else for. With auths mocked, an entrypoint that never authenticates is indistinguishable from one that does, which is why none of this was caught before. The full table is in contracts/AUTHORIZATION.md, including nine assetsup tokenization entrypoints still unguarded — those need a decision about which principal applies, so they are listed rather than guessed at. SC-43: arithmetic - Add assetsup/src/math.rs with checked add/sub/mul and a mul_div that survives an intermediate product overflowing i128 by cancelling common factors first. - Apply it to the dividend split, token mint/burn/transfer, and every ownership percentage. (balance * total_amount) / total_supply was the worst case: it trapped whenever the product exceeded i128::MAX even though the quotient was small. - Overflow now returns MathOverflow/MathUnderflow instead of trapping. - Rounding is documented: mul_div rounds down and the remainder stays with the contract, so a distribution can never overpay and no holder is advantaged by iteration order. - Boundary tests at 0, 1 and type max. SC-44: storage TTL - Add assetsup/src/ttl.rs holding the policy and constants in one place: extend persistent entries when under 30 days, out to 90. - Extend on read as well as write. An asset that is only ever queried would otherwise be archived despite being in active use. - Extend on write in initialize: a freshly written entry gets the network minimum, and a read cannot rescue an entry that is already gone. - Bump the contract instance TTL. Nothing did, so the instance itself could be archived and take the whole contract with it regardless of what else survived. The ledger-advancing tests caught this. - Tests advance 60 days and prove assets, transfers and configuration are still readable. SC-41: asset-maintenance coverage - 4 tests to 70, covering every entrypoint with happy and failure paths. - Validation boundaries: service date exactly now versus one second future, zero cost, condition and quality ratings at 1 and 10 and just outside. - Scheduling boundaries including a schedule due in the past reading as overdue and one due exactly now not yet overdue. - Three tests for the core audit property: history is append-only, an earlier record is never rewritten by a later one, and no operation removes a record. - Negative auth tests for each newly gated entrypoint. multisig_transfer had no tests at all; it now has 8. Closes #1205 Closes #1206 Closes #1207 Closes #1208 --- contracts/AUTHORIZATION.md | 231 +++++ contracts/asset-maintenance/src/lib.rs | 29 + contracts/asset-maintenance/src/test.rs | 4 +- .../asset-maintenance/src/tests_coverage.rs | 976 ++++++++++++++++++ contracts/assetsup/src/dividends.rs | 19 +- contracts/assetsup/src/lib.rs | 73 +- contracts/assetsup/src/math.rs | 181 ++++ contracts/assetsup/src/tests/auth.rs | 332 ++++++ contracts/assetsup/src/tests/mod.rs | 4 + contracts/assetsup/src/tests/ttl.rs | 116 +++ contracts/assetsup/src/tokenization.rs | 31 +- contracts/assetsup/src/ttl.rs | 84 ++ contracts/contrib/src/lib.rs | 4 + .../contrib/src/tests/asset_registry_tests.rs | 2 + contracts/multisig_transfer/src/auth_tests.rs | 162 +++ contracts/multisig_transfer/src/lib.rs | 27 +- 16 files changed, 2249 insertions(+), 26 deletions(-) create mode 100644 contracts/AUTHORIZATION.md create mode 100644 contracts/asset-maintenance/src/tests_coverage.rs create mode 100644 contracts/assetsup/src/math.rs create mode 100644 contracts/assetsup/src/tests/auth.rs create mode 100644 contracts/assetsup/src/tests/ttl.rs create mode 100644 contracts/assetsup/src/ttl.rs create mode 100644 contracts/multisig_transfer/src/auth_tests.rs diff --git a/contracts/AUTHORIZATION.md b/contracts/AUTHORIZATION.md new file mode 100644 index 000000000..67e2abbe2 --- /dev/null +++ b/contracts/AUTHORIZATION.md @@ -0,0 +1,231 @@ +# Authorization audit + +Every public entrypoint across the five crates, and which principal it +authenticates ([SC-42]). + +Soroban authorization is **explicit**. An entrypoint that does not call +`require_auth()` on an address is callable by anyone, whatever else it checks. +Comparing a caller-supplied `Address` argument against an allowlist is *not* +authorization: the attacker chooses that argument. + +`env.mock_all_auths()` hides the difference entirely, which is why every claim +below is backed by a negative test that runs **without** it — +`assetsup/src/tests/auth.rs` and the `*_requires_*_authorization` tests in the +other crates. + +## Legend + +| Mark | Meaning | +|---|---| +| ✅ | Calls `require_auth()` on the named principal. | +| 🔓 | Deliberately permissionless. | +| 📖 | Read-only; no state change, no auth required. | + +--- + +## `assetsup` + +### Fixed in this change + +These four took a `caller: Address`, compared it against a registrar allowlist, +the asset owner, or the admin — and never authenticated it. Because `caller` is +supplied by whoever builds the transaction, the check was satisfied by naming a +privileged address. + +| Entrypoint | Was | Now | +|---|---|---| +| `register_asset` | registrar allowlist check only | ✅ `caller` | +| `update_asset_metadata` | owner/admin check only | ✅ `caller` | +| `transfer_asset_ownership` | owner check only | ✅ `caller` | +| `retire_asset` | owner/admin check only | ✅ `caller` | + +`transfer_asset_ownership` was the most serious: a direct path to taking any +registered asset by naming its owner. + +### Lifecycle and administration + +| Entrypoint | Principal | | +|---|---|---| +| `initialize` | `admin` | ✅ | +| `update_admin` | current admin | ✅ | +| `add_authorized_registrar` | current admin | ✅ | +| `remove_authorized_registrar` | current admin | ✅ | +| `pause_contract` | current admin | ✅ | +| `unpause_contract` | current admin | ✅ | + +`initialize` now authenticates the incoming admin, closing the front-running +window where whoever called it first on a freshly deployed contract became +admin. + +### Registry + +| Entrypoint | Principal | | +|---|---|---| +| `register_asset` | `caller`, must be an authorized registrar | ✅ | +| `update_asset_metadata` | `caller`, must be owner or admin | ✅ | +| `transfer_asset_ownership` | `caller`, must be the current owner | ✅ | +| `retire_asset` | `caller`, must be owner or admin | ✅ | +| `get_asset`, `get_asset_info`, `batch_get_asset_info`, `get_assets_by_owner`, `check_asset_exists`, `get_total_asset_count`, `get_admin`, `is_paused`, `is_authorized_registrar`, `get_contract_metadata`, `get_asset_audit_logs` | — | 📖 | + +### Tokenization, dividends, voting + +| Entrypoint | Principal | | +|---|---|---| +| `tokenize_asset` | owner | ✅ | +| `mint_tokens`, `burn_tokens` | issuer | ✅ | +| `transfer_tokens` | `from` | ✅ | +| `lock_tokens` | owner | ✅ | +| `claim_dividends` | `holder` | ✅ | +| `cast_vote` | `voter` | ✅ | +| `propose_detokenization` | `proposer` | ✅ | +| `unlock_tokens`, `update_valuation`, `distribute_dividends`, `enable_revenue_sharing`, `disable_revenue_sharing`, `execute_detokenization` | — | ⚠️ **no auth** | +| `set_transfer_restriction`, `add_to_whitelist`, `remove_from_whitelist` | — | ⚠️ **no auth** | +| `get_token_balance`, `get_token_holders`, `is_tokens_locked`, `get_ownership_percentage`, `get_tokenized_asset`, `get_unclaimed_dividends`, `get_vote_tally`, `has_voted`, `proposal_passed`, `is_whitelisted`, `get_whitelist`, `get_detokenization_proposal`, `is_detokenization_active` | — | 📖 | + +The `require_auth` for the ✅ rows lives in the `lib.rs` entrypoint wrapper, not +in the module function it delegates to. `tokenization.rs`, `dividends.rs`, +`voting.rs` and `detokenization.rs` contain no `require_auth` at all, so calling +one of those functions directly would bypass authorization. They are +`pub(crate)` and only reachable through the wrappers today, but that is a +property of the module layout rather than an enforced boundary. + +The ⚠️ rows are **not fixed here.** They are real gaps, but each needs a +decision about *which* principal should be required — the token issuer, the +asset owner, or the admin — and that is a design question rather than an +oversight. They are listed so the next change starts from a list rather than a +search. None is an ownership-transfer path, which is why the four registry +entrypoints took priority. + +--- + +## `contrib` + +`contrib` was already the reference implementation for authorization in this +workspace: every acting address is authenticated. + +| Entrypoint | Principal | | +|---|---|---| +| `initialize` | `admin` | ✅ (added here) | +| `register_asset` | `registrar` | ✅ | +| `transfer_asset` | `caller` | ✅ | +| `retire_asset` | `caller` | ✅ | +| `add_authorized_registrar` / `add_registrar` | `caller`, must be admin | ✅ | +| `remove_authorized_registrar` / `remove_registrar` | `caller`, must be admin | ✅ | +| `pause_contract`, `unpause_contract` | `caller`, must be admin | ✅ | +| `get_admin`, `get_asset`, `get_asset_info`, `get_assets_by_owner`, `get_total_count`, `get_total_asset_count`, `is_authorized_registrar`, `get_audit_logs`, `is_paused` | — | 📖 | + +Only `initialize` was unguarded, and it is now authenticated. + +> The escrow, KYC, staking, oracle, tokenization, detokenization and +> restrictions files in `contrib/src/` are **not declared as modules** and so +> are not compiled into the crate. They expose no entrypoints and are excluded +> from this audit. + +--- + +## `multisig-wallet` + +| Entrypoint | Principal | | +|---|---|---| +| `initialize` | `admin` | ✅ | +| `submit_transaction` | `initiator`, must be an owner | ✅ | +| `confirm_transaction` | `confirmer`, must be an owner | ✅ | +| `revoke_confirmation` | `revoker`, must be an owner | ✅ | +| `cancel_transaction` | `caller`, must be the initiator | ✅ | +| `propose_add_owner`, `propose_remove_owner`, `propose_change_threshold` | `proposer`, must be an owner | ✅ | +| `confirm_proposal` | `confirmer`, must be an owner | ✅ | +| `emergency_freeze`, `emergency_unfreeze` | `caller`, must be an owner | ✅ | +| `set_daily_limit` | `caller`, must be an owner | ✅ | +| `execute_transaction`, `execute_proposal` | — | 🔓 | +| `get_owners`, `get_threshold`, `get_transaction`, `is_frozen`, `get_required_confirmations`, `get_owner_profile`, `get_proposal` | — | 📖 | + +`execute_transaction` and `execute_proposal` are permissionless **by design**. +The authorization decision was already made by the confirming owners; requiring +one of them to also submit the execution adds no security while adding liveness +risk. This is the one place where "anyone can call it" is the correct answer. + +--- + +## `multisig-transfer` + +### Fixed in this change + +`multisig-transfer` had the **same gap as `assetsup`**, across every +entrypoint. Each compared a caller-supplied `caller` argument against the +stored admin, the asset's owner, or the approver set, and none authenticated +it — so approvals could be forged by naming an authorized approver, and +approval rules rewritten by naming the admin. + +| Entrypoint | Principal | | +|---|---|---| +| `initialize` | `admin` | ✅ (added here) | +| `configure_approval_rule` | `caller`, must be admin | ✅ (added here) | +| `create_transfer_request` | `caller`, ownership checked against the registry | ✅ (added here) | +| `approve_transfer_request` | `caller`, must be an authorized approver | ✅ (added here) | +| `reject_transfer_request` | `caller`, must be an authorized approver | ✅ (added here) | +| `cancel_transfer_request` | `caller`, requester or admin | ✅ (added here) | +| `execute_transfer` | — | 🔓 | +| `get_request`, `get_asset_history`, `get_pending_transfers_approver`, `get_required_approvers_category` | — | 📖 | + +`execute_transfer` is permissionless for the same reason as +`multisig-wallet`'s `execute_*`: the approvers already made the authorization +decision. It takes a `caller` argument for the audit trail, which is +deliberately not authenticated — a comment now says so, since an unused +authenticated-looking parameter is exactly what makes this class of bug hard to +spot. + +--- + +## `asset-maintenance` + +| Entrypoint | Principal | | +|---|---|---| +| `init` | `admin` | ✅ (added here) | +| `register_provider` | stored admin | ✅ | +| `deactivate_provider` | stored admin | ✅ | +| `add_maintenance_record` | `record.provider` | ✅ | +| `schedule_maintenance` | `owner` | ✅ | +| `update_maintenance_schedule` | `owner` | ✅ | +| `complete_scheduled_maintenance` | `record.provider`, via `add_maintenance_record` | ✅ | +| `acknowledge_maintenance_alert` | `by` | ✅ | +| `add_warranty_information` | stored admin | ✅ (added here) | +| `update_warranty_information` | stored admin | ✅ (added here) | +| `file_warranty_claim` | stored admin | ✅ (added here) | +| `create_maintenance_alert` | stored admin | ✅ (added here) | +| `get_maintenance_history`, `get_upcoming_maintenance`, `get_provider_details`, `get_warranty`, `get_alerts`, `calculate_total_maintenance_cost`, `calculate_asset_downtime`, `get_asset_health_score`, `get_asset_stats`, `is_maintenance_cost_excessive`, `get_overdue_maintenance` | — | 📖 | + +Four entrypoints wrote warranty terms, filed claims, and raised alerts with **no +authorization at all** — anyone could forge the audit evidence this contract +exists to provide. They now require the stored admin. + +Admin is used because these entrypoints take no caller argument, so the stored +admin is the only principal the contract can authenticate without an ABI change. +A finer-grained model — letting an asset owner manage their own warranty — needs +the asset-registry integration this contract does not have. `verify_asset_exists` +is currently a stub returning `true`. + +### Authenticating by delegation + +`update_maintenance_schedule` and `complete_scheduled_maintenance` do not call +`require_auth` directly; they delegate to `schedule_maintenance` and +`add_maintenance_record`, which do. The protection is real but easy to miss when +reading, and easy to break by refactoring the delegation away — which is why +both have their own negative test. + +--- + +## Confused-deputy check + +No contract calls `require_auth()` on its own address, and none passes its own +address where a caller's is expected. The one cross-contract call — +`multisig-transfer` invoking the registry to move ownership — is the intended +authority boundary: the multisig contract *is* the authorized party at that +point, having already collected the required approvals. + +## What is not covered + +- `require_auth_for_args` is not used anywhere. Authorization is currently + per-entrypoint, not bound to specific amounts or recipients, so a signature + authorizing a transfer does not pin *which* transfer. Binding auth to + arguments on the value-carrying paths is worth a follow-up. +- The ⚠️ rows under `assetsup` tokenization remain open. diff --git a/contracts/asset-maintenance/src/lib.rs b/contracts/asset-maintenance/src/lib.rs index f8b11a27f..54cfdc06d 100644 --- a/contracts/asset-maintenance/src/lib.rs +++ b/contracts/asset-maintenance/src/lib.rs @@ -2,6 +2,7 @@ use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, String, Vec}; mod test; +mod tests_coverage; #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] @@ -162,7 +163,27 @@ pub struct AssetMaintenanceContract; #[contractimpl] impl AssetMaintenanceContract { + /// Loads the stored admin and requires its authorization. + /// + /// Several entrypoints mutate registry-level data without taking a caller + /// argument, so the stored admin is the only principal this contract can + /// authenticate against without an ABI change. A finer-grained model — + /// letting the asset owner manage their own warranty, for instance — needs + /// the asset-registry integration this contract does not have yet. + fn require_admin(env: &Env) { + let admin: Address = env + .storage() + .persistent() + .get(&DataKey::Admin) + .expect("not initialized"); + admin.require_auth(); + } + pub fn init(env: Env, admin: Address, registry: Address) { + // Without this, whoever calls init first becomes admin of a freshly + // deployed contract, regardless of who deployed it. + admin.require_auth(); + if env.storage().persistent().has(&DataKey::Admin) { panic!("already initialized"); } @@ -347,6 +368,8 @@ impl AssetMaintenanceContract { } pub fn add_warranty_information(env: Env, warranty: WarrantyInfo) { + Self::require_admin(&env); + if warranty.end_date <= warranty.start_date { panic!("warranty dates invalid"); } @@ -361,6 +384,8 @@ impl AssetMaintenanceContract { } pub fn update_warranty_information(env: Env, warranty: WarrantyInfo) { + Self::require_admin(&env); + if !env .storage() .persistent() @@ -382,6 +407,8 @@ impl AssetMaintenanceContract { } pub fn file_warranty_claim(env: Env, asset_id: u64, claim_amount: i128) { + Self::require_admin(&env); + let mut warranty: WarrantyInfo = env .storage() .persistent() @@ -410,6 +437,8 @@ impl AssetMaintenanceContract { } pub fn create_maintenance_alert(env: Env, alert: MaintenanceAlert) { + Self::require_admin(&env); + let mut alerts = env .storage() .persistent() diff --git a/contracts/asset-maintenance/src/test.rs b/contracts/asset-maintenance/src/test.rs index 74ab59c7b..bb3bd4376 100644 --- a/contracts/asset-maintenance/src/test.rs +++ b/contracts/asset-maintenance/src/test.rs @@ -14,6 +14,9 @@ fn test_init_and_provider_registration() { let admin = Address::generate(&env); let registry = Address::generate(&env); + // init now requires the admin's authorization, so auths must be mocked + // before it rather than after. + env.mock_all_auths(); client.init(&admin, ®istry); let provider_addr = Address::generate(&env); @@ -30,7 +33,6 @@ fn test_init_and_provider_registration() { service_area: String::from_str(&env, "Global"), }; - env.mock_all_auths(); client.register_provider(&provider); let fetched = client.get_provider_details(&provider_addr).unwrap(); diff --git a/contracts/asset-maintenance/src/tests_coverage.rs b/contracts/asset-maintenance/src/tests_coverage.rs new file mode 100644 index 000000000..a96d48a06 --- /dev/null +++ b/contracts/asset-maintenance/src/tests_coverage.rs @@ -0,0 +1,976 @@ +//! Expanded coverage for asset-maintenance ([SC-41]). +//! +//! `test.rs` covers four happy paths across ~730 lines. This module works +//! through every entrypoint with both happy and failure paths, the validation +//! boundaries, the authorization boundaries without `mock_all_auths`, and the +//! property the whole contract exists for: **maintenance history is audit +//! evidence and cannot be silently altered or deleted**. +#![cfg(test)] + +extern crate std; + +use soroban_sdk::testutils::{Address as _, Ledger as _, LedgerInfo}; +use soroban_sdk::{vec, Address, Env, String, Vec}; + +use super::*; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +struct Ctx<'a> { + client: AssetMaintenanceContractClient<'a>, + admin: Address, + provider: Address, +} + +const NOW: u64 = 1_000_000; + +fn setup(env: &Env) -> Ctx<'_> { + env.ledger().with_mut(|li: &mut LedgerInfo| { + li.timestamp = NOW; + }); + + let contract_id = env.register(AssetMaintenanceContract, ()); + let client = AssetMaintenanceContractClient::new(env, &contract_id); + let admin = Address::generate(env); + let registry = Address::generate(env); + let provider = Address::generate(env); + + env.mock_all_auths(); + client.init(&admin, ®istry); + client.register_provider(&provider_profile(env, &provider, true)); + + Ctx { + client, + admin, + provider, + } +} + +fn provider_profile(env: &Env, address: &Address, active: bool) -> ProviderProfile { + ProviderProfile { + address: address.clone(), + name: String::from_str(env, "Service Corp"), + specialization: vec![env, String::from_str(env, "Engines")], + certification_details: String::from_str(env, "ISO9001"), + total_services: 0, + average_rating: 0, + registration_timestamp: env.ledger().timestamp(), + is_active: active, + contact_hash: String::from_str(env, "hash"), + service_area: String::from_str(env, "Global"), + } +} + +/// A valid record: costs balance, ratings in range, service date not in future. +fn record(env: &Env, provider: &Address, record_id: u64, asset_id: u64) -> MaintenanceRecord { + MaintenanceRecord { + record_id, + asset_id, + maintenance_type: MaintenanceType::Preventive, + provider: provider.clone(), + technician_id: String::from_str(env, "tech-01"), + service_date: NOW - 100, + duration_hours: 4, + description: String::from_str(env, "Routine service"), + parts_replaced: vec![env, String::from_str(env, "filter")], + labor_cost: 60, + parts_cost: 40, + total_cost: 100, + location: String::from_str(env, "Depot"), + condition_before: 5, + condition_after: 9, + issues_found: String::from_str(env, "none"), + issues_resolved: String::from_str(env, "none"), + next_recommendation: String::from_str(env, "6 months"), + documents_ipfs: vec![env, String::from_str(env, "ipfs://doc")], + quality_rating: 8, + timestamp: NOW, + } +} + +fn schedule(env: &Env, provider: &Address, asset_id: u64, due: u64) -> ScheduledMaintenance { + ScheduledMaintenance { + asset_id, + maintenance_type: MaintenanceType::Preventive, + frequency_days: 30, + last_service_date: NOW - 1000, + next_service_due: due, + provider_assigned: provider.clone(), + reminder_days: 7, + auto_schedule: true, + priority: PriorityLevel::Medium, + estimated_cost: 100, + estimated_duration: 4, + required_parts: vec![env, String::from_str(env, "filter")], + special_instructions: String::from_str(env, "none"), + } +} + +fn warranty(env: &Env, asset_id: u64, start: u64, end: u64) -> WarrantyInfo { + WarrantyInfo { + asset_id, + provider: String::from_str(env, "Maker Ltd"), + warranty_type: String::from_str(env, "Manufacturer"), + start_date: start, + end_date: end, + coverage_details: String::from_str(env, "Parts and labour"), + terms_hash: String::from_str(env, "hash"), + claim_count: 0, + max_claims: 2, + status: WarrantyStatus::Active, + is_transferable: true, + } +} + +fn alert(env: &Env, asset_id: u64) -> MaintenanceAlert { + MaintenanceAlert { + asset_id, + alert_type: AlertType::ServiceDue, + severity: AlertSeverity::Medium, + message: String::from_str(env, "Service due"), + due_date: NOW + 1000, + acknowledged: false, + acknowledged_by: Address::generate(env), + created_at: NOW, + } +} + +// --------------------------------------------------------------------------- +// init +// --------------------------------------------------------------------------- + +#[test] +fn init_stores_admin_and_registry() { + let env = Env::default(); + let ctx = setup(&env); + // Observable through an admin-gated call succeeding. + ctx.client + .register_provider(&provider_profile(&env, &Address::generate(&env), true)); +} + +#[test] +#[should_panic(expected = "already initialized")] +fn init_twice_panics() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.init(&ctx.admin, &Address::generate(&env)); +} + +#[test] +fn init_requires_the_admins_authorization() { + // Without this, whoever calls init first owns a freshly deployed contract. + let env = Env::default(); + let contract_id = env.register(AssetMaintenanceContract, ()); + let client = AssetMaintenanceContractClient::new(&env, &contract_id); + + let res = client.try_init(&Address::generate(&env), &Address::generate(&env)); + assert!(res.is_err(), "init must require the admin's authorization"); +} + +// --------------------------------------------------------------------------- +// Providers +// --------------------------------------------------------------------------- + +#[test] +fn register_provider_stores_the_profile() { + let env = Env::default(); + let ctx = setup(&env); + + let fetched = ctx.client.get_provider_details(&ctx.provider).unwrap(); + assert_eq!(fetched.name, String::from_str(&env, "Service Corp")); + assert!(fetched.is_active); +} + +#[test] +fn get_provider_details_is_none_for_an_unknown_provider() { + let env = Env::default(); + let ctx = setup(&env); + assert!(ctx + .client + .get_provider_details(&Address::generate(&env)) + .is_none()); +} + +#[test] +fn deactivate_provider_clears_the_active_flag() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client.deactivate_provider(&ctx.provider); + + assert!( + !ctx.client + .get_provider_details(&ctx.provider) + .unwrap() + .is_active + ); +} + +#[test] +fn deactivating_an_unknown_provider_is_a_no_op() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.deactivate_provider(&Address::generate(&env)); +} + +#[test] +fn register_provider_requires_the_admins_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = + ctx.client + .try_register_provider(&provider_profile(&env, &Address::generate(&env), true)); + assert!(res.is_err(), "only the admin may register providers"); +} + +#[test] +fn deactivate_provider_requires_the_admins_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = ctx.client.try_deactivate_provider(&ctx.provider); + assert!(res.is_err(), "only the admin may deactivate providers"); +} + +// --------------------------------------------------------------------------- +// Maintenance records — validation boundaries +// --------------------------------------------------------------------------- + +#[test] +fn add_maintenance_record_appends_to_history() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + + let history = ctx.client.get_maintenance_history(&7); + assert_eq!(history.len(), 1); + assert_eq!(history.get(0).unwrap().record_id, 1); +} + +#[test] +fn maintenance_history_is_empty_for_an_unknown_asset() { + let env = Env::default(); + let ctx = setup(&env); + assert_eq!(ctx.client.get_maintenance_history(&999).len(), 0); +} + +#[test] +#[should_panic(expected = "provider not registered")] +fn a_record_from_an_unregistered_provider_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let stranger = Address::generate(&env); + + ctx.client + .add_maintenance_record(&record(&env, &stranger, 1, 7)); +} + +#[test] +#[should_panic(expected = "provider is inactive")] +fn a_record_from_a_deactivated_provider_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.deactivate_provider(&ctx.provider); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); +} + +#[test] +#[should_panic(expected = "service date cannot be in future")] +fn a_record_dated_in_the_future_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.service_date = NOW + 1; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +fn a_record_dated_exactly_now_is_accepted() { + // The boundary: `service_date > now` is rejected, so `== now` must pass. + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.service_date = NOW; + + ctx.client.add_maintenance_record(&r); + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); +} + +#[test] +fn a_record_dated_far_in_the_past_is_accepted() { + // Backfilling historical service records is legitimate. + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.service_date = 1; + + ctx.client.add_maintenance_record(&r); + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); +} + +#[test] +#[should_panic(expected = "cost values must be non-negative")] +fn a_record_with_a_negative_cost_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.labor_cost = -1; + r.total_cost = 39; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +#[should_panic(expected = "labor + parts cost must equal total cost")] +fn a_record_whose_costs_do_not_balance_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.total_cost = 999; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +fn a_zero_cost_record_is_accepted() { + // Boundary at zero: a warranty-covered service costs nothing. + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.labor_cost = 0; + r.parts_cost = 0; + r.total_cost = 0; + + ctx.client.add_maintenance_record(&r); + assert_eq!(ctx.client.calculate_total_maintenance_cost(&7), 0); +} + +#[test] +#[should_panic(expected = "condition ratings must be 1-10")] +fn a_condition_rating_below_the_range_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.condition_before = 0; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +#[should_panic(expected = "condition ratings must be 1-10")] +fn a_condition_rating_above_the_range_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.condition_after = 11; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +fn condition_ratings_at_both_ends_of_the_range_are_accepted() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.condition_before = 1; + r.condition_after = 10; + + ctx.client.add_maintenance_record(&r); + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); +} + +#[test] +#[should_panic(expected = "quality rating must be 1-10")] +fn a_quality_rating_of_zero_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.quality_rating = 0; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +#[should_panic(expected = "quality rating must be 1-10")] +fn a_quality_rating_above_ten_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut r = record(&env, &ctx.provider, 1, 7); + r.quality_rating = 11; + + ctx.client.add_maintenance_record(&r); +} + +#[test] +fn add_maintenance_record_requires_the_providers_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = ctx + .client + .try_add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + assert!( + res.is_err(), + "naming a registered provider must not be enough to file a record" + ); +} + +// --------------------------------------------------------------------------- +// The audit property: history is append-only +// --------------------------------------------------------------------------- + +#[test] +fn history_is_append_only_and_earlier_records_are_never_rewritten() { + // The core reason this contract exists. If a later write could alter an + // earlier record, the on-chain history would be worthless as evidence. + let env = Env::default(); + let ctx = setup(&env); + + let first = record(&env, &ctx.provider, 1, 7); + ctx.client.add_maintenance_record(&first); + + let mut second = record(&env, &ctx.provider, 2, 7); + second.description = String::from_str(&env, "Second service"); + second.total_cost = 250; + second.labor_cost = 150; + second.parts_cost = 100; + ctx.client.add_maintenance_record(&second); + + let history = ctx.client.get_maintenance_history(&7); + assert_eq!(history.len(), 2, "both records must be retained"); + + let stored_first = history.get(0).unwrap(); + assert_eq!(stored_first.record_id, 1); + assert_eq!( + stored_first.total_cost, 100, + "the first record is unchanged" + ); + assert_eq!(stored_first.description, first.description); + assert_eq!(history.get(1).unwrap().record_id, 2); +} + +#[test] +fn reusing_a_record_id_appends_rather_than_overwriting() { + // There is no de-duplication on record_id. Filing the same id twice adds a + // second entry; it does not replace the first. Pinning the real behaviour: + // the audit trail keeps both, which is the safe direction, but consumers + // must not assume record_id is unique. + let env = Env::default(); + let ctx = setup(&env); + + let mut original = record(&env, &ctx.provider, 1, 7); + original.total_cost = 100; + ctx.client.add_maintenance_record(&original); + + let mut duplicate = record(&env, &ctx.provider, 1, 7); + duplicate.total_cost = 20; + duplicate.labor_cost = 10; + duplicate.parts_cost = 10; + ctx.client.add_maintenance_record(&duplicate); + + let history = ctx.client.get_maintenance_history(&7); + assert_eq!(history.len(), 2, "the original is retained, not replaced"); + assert_eq!(history.get(0).unwrap().total_cost, 100); +} + +#[test] +fn no_entrypoint_removes_a_maintenance_record() { + // Completing scheduled maintenance and deactivating the provider are the + // operations most likely to prune history. Neither may. + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + ctx.client + .complete_scheduled_maintenance(&7, &record(&env, &ctx.provider, 2, 7)); + ctx.client.deactivate_provider(&ctx.provider); + + let history = ctx.client.get_maintenance_history(&7); + assert_eq!( + history.len(), + 2, + "history must survive every other operation" + ); + assert_eq!(history.get(0).unwrap().record_id, 1); +} + +// --------------------------------------------------------------------------- +// Scheduling +// --------------------------------------------------------------------------- + +#[test] +fn schedule_maintenance_stores_the_schedule() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + + let upcoming = ctx.client.get_upcoming_maintenance(&7).unwrap(); + assert_eq!(upcoming.next_service_due, NOW + 500); +} + +#[test] +fn get_upcoming_maintenance_is_none_when_nothing_is_scheduled() { + let env = Env::default(); + let ctx = setup(&env); + assert!(ctx.client.get_upcoming_maintenance(&7).is_none()); +} + +#[test] +#[should_panic(expected = "frequency must be positive")] +fn a_schedule_with_zero_frequency_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut sched = schedule(&env, &ctx.provider, 7, NOW + 500); + sched.frequency_days = 0; + + ctx.client.schedule_maintenance(&ctx.admin, &sched); +} + +#[test] +fn a_schedule_due_in_the_past_is_accepted_and_reads_as_overdue() { + // Scheduling in the past is allowed — it is how a missed service is + // recorded — and must immediately register as overdue. + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW - 1)); + + assert!(ctx.client.get_overdue_maintenance(&7)); +} + +#[test] +fn a_schedule_due_in_the_future_is_not_overdue() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + + assert!(!ctx.client.get_overdue_maintenance(&7)); +} + +#[test] +fn a_schedule_due_exactly_now_is_not_yet_overdue() { + // Boundary: the check is `now > due`, so equality is not overdue. + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW)); + + assert!(!ctx.client.get_overdue_maintenance(&7)); +} + +#[test] +fn get_overdue_maintenance_is_false_without_a_schedule() { + let env = Env::default(); + let ctx = setup(&env); + assert!(!ctx.client.get_overdue_maintenance(&7)); +} + +#[test] +#[should_panic(expected = "no schedule exists for asset")] +fn updating_a_schedule_that_does_not_exist_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .update_maintenance_schedule(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); +} + +#[test] +fn update_maintenance_schedule_replaces_the_existing_schedule() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + + ctx.client + .update_maintenance_schedule(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 9_000)); + + assert_eq!( + ctx.client + .get_upcoming_maintenance(&7) + .unwrap() + .next_service_due, + NOW + 9_000 + ); +} + +#[test] +fn schedule_maintenance_requires_the_owners_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = ctx + .client + .try_schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + assert!(res.is_err(), "scheduling must require authorization"); +} + +#[test] +fn completing_scheduled_maintenance_rolls_the_schedule_forward() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .schedule_maintenance(&ctx.admin, &schedule(&env, &ctx.provider, 7, NOW + 500)); + + let r = record(&env, &ctx.provider, 1, 7); + ctx.client.complete_scheduled_maintenance(&7, &r); + + // auto_schedule is on, so the next due date moves to service_date + 30 days. + let expected = r.service_date + 30 * 86400; + assert_eq!( + ctx.client + .get_upcoming_maintenance(&7) + .unwrap() + .next_service_due, + expected + ); + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); +} + +#[test] +fn completing_maintenance_without_a_schedule_still_records_the_service() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .complete_scheduled_maintenance(&7, &record(&env, &ctx.provider, 1, 7)); + + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); + assert!(ctx.client.get_upcoming_maintenance(&7).is_none()); +} + +// --------------------------------------------------------------------------- +// Warranties +// --------------------------------------------------------------------------- + +#[test] +fn add_warranty_information_stores_the_terms() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + + assert_eq!(ctx.client.get_warranty(&7).unwrap().end_date, NOW + 10_000); +} + +#[test] +fn get_warranty_is_none_for_an_asset_without_one() { + let env = Env::default(); + let ctx = setup(&env); + assert!(ctx.client.get_warranty(&7).is_none()); +} + +#[test] +#[should_panic(expected = "warranty dates invalid")] +fn a_warranty_ending_before_it_starts_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW + 100, NOW)); +} + +#[test] +#[should_panic(expected = "warranty dates invalid")] +fn a_zero_length_warranty_is_rejected() { + // Boundary: end must be strictly after start. + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW)); +} + +#[test] +#[should_panic(expected = "no warranty exists for asset")] +fn updating_a_warranty_that_does_not_exist_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .update_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); +} + +#[test] +fn update_warranty_information_replaces_the_terms() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + + ctx.client + .update_warranty_information(&warranty(&env, 7, NOW, NOW + 50_000)); + + assert_eq!(ctx.client.get_warranty(&7).unwrap().end_date, NOW + 50_000); +} + +#[test] +fn add_warranty_information_requires_the_admins_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = ctx + .client + .try_add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + assert!( + res.is_err(), + "warranty terms must not be writable by anyone" + ); +} + +// --------------------------------------------------------------------------- +// Warranty claims +// --------------------------------------------------------------------------- + +#[test] +fn filing_a_claim_increments_the_claim_count() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + + ctx.client.file_warranty_claim(&7, &500); + + assert_eq!(ctx.client.get_warranty(&7).unwrap().claim_count, 1); +} + +#[test] +#[should_panic(expected = "no warranty found")] +fn claiming_against_a_missing_warranty_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.file_warranty_claim(&7, &500); +} + +#[test] +#[should_panic(expected = "max claims reached")] +fn claiming_beyond_the_maximum_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + + // max_claims is 2. + ctx.client.file_warranty_claim(&7, &100); + ctx.client.file_warranty_claim(&7, &100); + ctx.client.file_warranty_claim(&7, &100); +} + +#[test] +#[should_panic(expected = "warranty has expired")] +fn claiming_after_the_warranty_expires_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 100)); + + env.ledger().with_mut(|li: &mut LedgerInfo| { + li.timestamp = NOW + 101; + }); + + ctx.client.file_warranty_claim(&7, &100); +} + +#[test] +#[should_panic(expected = "warranty is not active")] +fn claiming_against_a_voided_warranty_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + let mut w = warranty(&env, 7, NOW, NOW + 10_000); + w.status = WarrantyStatus::Voided; + ctx.client.add_warranty_information(&w); + + ctx.client.file_warranty_claim(&7, &100); +} + +#[test] +fn file_warranty_claim_requires_the_admins_authorization() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_warranty_information(&warranty(&env, 7, NOW, NOW + 10_000)); + env.set_auths(&[]); + + let res = ctx.client.try_file_warranty_claim(&7, &100); + assert!(res.is_err(), "claims must not be forgeable by anyone"); +} + +// --------------------------------------------------------------------------- +// Alerts +// --------------------------------------------------------------------------- + +#[test] +fn create_maintenance_alert_appends_to_the_alert_list() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client.create_maintenance_alert(&alert(&env, 7)); + ctx.client.create_maintenance_alert(&alert(&env, 7)); + + assert_eq!(ctx.client.get_alerts(&7).len(), 2); +} + +#[test] +fn get_alerts_is_empty_for_an_asset_without_any() { + let env = Env::default(); + let ctx = setup(&env); + assert_eq!(ctx.client.get_alerts(&7).len(), 0); +} + +#[test] +fn acknowledging_an_alert_marks_it_and_records_who() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.create_maintenance_alert(&alert(&env, 7)); + + let acker = Address::generate(&env); + ctx.client.acknowledge_maintenance_alert(&7, &0, &acker); + + let stored = ctx.client.get_alerts(&7).get(0).unwrap(); + assert!(stored.acknowledged); + assert_eq!(stored.acknowledged_by, acker); +} + +#[test] +fn acknowledging_an_out_of_range_alert_index_is_a_no_op() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client.create_maintenance_alert(&alert(&env, 7)); + + ctx.client + .acknowledge_maintenance_alert(&7, &99, &Address::generate(&env)); + + assert!(!ctx.client.get_alerts(&7).get(0).unwrap().acknowledged); +} + +#[test] +#[should_panic(expected = "no alerts found")] +fn acknowledging_an_alert_for_an_asset_without_any_is_rejected() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .acknowledge_maintenance_alert(&7, &0, &Address::generate(&env)); +} + +#[test] +fn create_maintenance_alert_requires_the_admins_authorization() { + let env = Env::default(); + let ctx = setup(&env); + env.set_auths(&[]); + + let res = ctx.client.try_create_maintenance_alert(&alert(&env, 7)); + assert!(res.is_err(), "alerts must not be forgeable by anyone"); +} + +// --------------------------------------------------------------------------- +// Derived statistics +// --------------------------------------------------------------------------- + +#[test] +fn stats_start_at_zero_for_an_unknown_asset() { + let env = Env::default(); + let ctx = setup(&env); + + assert_eq!(ctx.client.calculate_total_maintenance_cost(&7), 0); + assert_eq!(ctx.client.calculate_asset_downtime(&7), 0); +} + +#[test] +fn cost_and_downtime_accumulate_across_records() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 2, 7)); + + assert_eq!(ctx.client.calculate_total_maintenance_cost(&7), 200); + assert_eq!(ctx.client.calculate_asset_downtime(&7), 8); + assert_eq!(ctx.client.get_asset_stats(&7).service_count, 2); +} + +#[test] +fn health_score_is_within_the_documented_range() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + + let score = ctx.client.get_asset_health_score(&7); + assert!(score <= 100, "health score must be on a 1-100 scale"); +} + +#[test] +fn is_maintenance_cost_excessive_compares_against_the_threshold() { + let env = Env::default(); + let ctx = setup(&env); + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + + assert!(ctx.client.is_maintenance_cost_excessive(&7, &99)); + assert!(!ctx.client.is_maintenance_cost_excessive(&7, &100)); + assert!(!ctx.client.is_maintenance_cost_excessive(&7, &101)); +} + +#[test] +fn stats_are_tracked_per_asset() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 2, 8)); + + assert_eq!(ctx.client.calculate_total_maintenance_cost(&7), 100); + assert_eq!(ctx.client.calculate_total_maintenance_cost(&8), 100); + assert_eq!(ctx.client.get_maintenance_history(&7).len(), 1); + assert_eq!(ctx.client.get_maintenance_history(&8).len(), 1); +} + +#[test] +fn records_for_different_assets_do_not_share_history() { + let env = Env::default(); + let ctx = setup(&env); + + ctx.client + .add_maintenance_record(&record(&env, &ctx.provider, 1, 7)); + + assert_eq!(ctx.client.get_maintenance_history(&8).len(), 0); +} + +#[test] +fn parts_replaced_and_documents_round_trip() { + let env = Env::default(); + let ctx = setup(&env); + let r = record(&env, &ctx.provider, 1, 7); + + ctx.client.add_maintenance_record(&r); + + let stored = ctx.client.get_maintenance_history(&7).get(0).unwrap(); + let expected_parts: Vec = vec![&env, String::from_str(&env, "filter")]; + assert_eq!(stored.parts_replaced, expected_parts); + assert_eq!(stored.documents_ipfs.len(), 1); + assert_eq!(stored.technician_id, String::from_str(&env, "tech-01")); +} diff --git a/contracts/assetsup/src/dividends.rs b/contracts/assetsup/src/dividends.rs index 9ba0819ee..e24a31e1c 100644 --- a/contracts/assetsup/src/dividends.rs +++ b/contracts/assetsup/src/dividends.rs @@ -1,4 +1,5 @@ use crate::error::Error; +use crate::math; use crate::types::{OwnershipRecord, TokenDataKey, TokenizedAsset}; use soroban_sdk::{Address, Env, Vec}; @@ -27,11 +28,19 @@ pub fn distribute_dividends(env: &Env, asset_id: u64, total_amount: i128) -> Res let holder_key = TokenDataKey::TokenHolder(asset_id, holder.clone()); let mut ownership: OwnershipRecord = store.get(&holder_key).ok_or(Error::HolderNotFound)?; - // Calculate proportional dividend: (balance / total_supply) * total_amount - let proportion = (ownership.balance * total_amount) / tokenized_asset.total_supply; - - // Add to unclaimed dividends - ownership.unclaimed_dividends += proportion; + // Proportional dividend: balance/total_supply of total_amount. + // + // Written naively as (balance * total_amount) / total_supply this + // overflows whenever the intermediate product exceeds i128::MAX, even + // though the quotient is small. Rounds down; the remainder stays with + // the contract. See crate::math for the rounding rationale. + let proportion = math::mul_div( + ownership.balance, + total_amount, + tokenized_asset.total_supply, + )?; + + ownership.unclaimed_dividends = math::add(ownership.unclaimed_dividends, proportion)?; store.set(&holder_key, &ownership); } diff --git a/contracts/assetsup/src/lib.rs b/contracts/assetsup/src/lib.rs index 7c220273f..040e33bca 100644 --- a/contracts/assetsup/src/lib.rs +++ b/contracts/assetsup/src/lib.rs @@ -14,8 +14,10 @@ pub(crate) mod dividends; pub(crate) mod error; pub(crate) mod insurance; pub(crate) mod lease; +pub(crate) mod math; pub(crate) mod tokenization; pub(crate) mod transfer_restrictions; +pub(crate) mod ttl; pub(crate) mod types; pub(crate) mod voting; @@ -43,6 +45,7 @@ pub struct AssetUpContract; impl AssetUpContract { pub fn initialize(env: Env, admin: Address) -> Result<(), Error> { admin.require_auth(); + ttl::extend_instance(&env); if env.storage().persistent().has(&DataKey::Admin) { handle_error(&env, Error::AlreadyInitialized) @@ -73,20 +76,38 @@ impl AssetUpContract { .persistent() .set(&DataKey::AuthorizedRegistrar(admin.clone()), &true); + // Extend on write, not only on read. A freshly written entry gets the + // network's minimum lifetime, which is short; without this the whole + // contract configuration can be archived before anyone reads it, and a + // read cannot rescue an entry that is already gone. + ttl::extend_persistent(&env, &DataKey::Admin); + ttl::extend_persistent(&env, &DataKey::Paused); + ttl::extend_persistent(&env, &DataKey::TotalAssetCount); + ttl::extend_persistent(&env, &DataKey::ContractMetadata); + ttl::extend_persistent(&env, &DataKey::AuthorizedRegistrar(admin)); + Ok(()) } pub fn get_admin(env: Env) -> Result { + ttl::extend_instance(&env); + let key = DataKey::Admin; if !env.storage().persistent().has(&key) { handle_error(&env, Error::AdminNotFound) } + // Contract-level configuration lives in persistent storage, so it + // needs extending like any other persistent entry — the instance bump + // alone does not cover it. + ttl::extend_persistent(&env, &key); + let admin = env.storage().persistent().get(&key).unwrap(); Ok(admin) } pub fn is_paused(env: Env) -> Result { + ttl::extend_persistent(&env, &DataKey::Paused); Ok(env .storage() .persistent() @@ -95,6 +116,7 @@ impl AssetUpContract { } pub fn get_total_asset_count(env: Env) -> Result { + ttl::extend_persistent(&env, &DataKey::TotalAssetCount); Ok(env .storage() .persistent() @@ -103,6 +125,7 @@ impl AssetUpContract { } pub fn get_contract_metadata(env: Env) -> Result { + ttl::extend_persistent(&env, &DataKey::ContractMetadata); let metadata = env.storage().persistent().get(&DataKey::ContractMetadata); match metadata { Some(m) => Ok(m), @@ -111,15 +134,21 @@ impl AssetUpContract { } pub fn is_authorized_registrar(env: Env, address: Address) -> Result { - Ok(env - .storage() - .persistent() - .get(&DataKey::AuthorizedRegistrar(address)) - .unwrap_or(false)) + let key = DataKey::AuthorizedRegistrar(address); + ttl::extend_persistent(&env, &key); + Ok(env.storage().persistent().get(&key).unwrap_or(false)) } // Asset functions pub fn register_asset(env: Env, asset: asset::Asset, caller: Address) -> Result<(), Error> { + ttl::extend_instance(&env); + + // Authenticate the caller before trusting `caller` for anything else. + // The registrar check below compares against a caller-supplied address, + // so without this any account could name an authorized registrar and + // pass it. + caller.require_auth(); + // Check if contract is paused if Self::is_paused(env.clone())? { return Err(Error::ContractPaused); @@ -143,6 +172,7 @@ impl AssetUpContract { // Store asset store.set(&key, &asset); + ttl::extend_persistent(&env, &key); // Update owner registry let owner_key = asset::DataKey::OwnerRegistry(asset.owner.clone()); @@ -220,6 +250,12 @@ impl AssetUpContract { new_custom_attributes: Option>, caller: Address, ) -> Result<(), Error> { + ttl::extend_instance(&env); + + // Authenticate before the owner/admin comparison below, which would + // otherwise be satisfied by simply naming the owner's address. + caller.require_auth(); + // Check if contract is paused if Self::is_paused(env.clone())? { return Err(Error::ContractPaused); @@ -256,6 +292,7 @@ impl AssetUpContract { } store.set(&key, &asset); + ttl::extend_persistent(&env, &key); // Append audit log audit::append_audit_log( @@ -281,6 +318,13 @@ impl AssetUpContract { new_owner: Address, caller: Address, ) -> Result<(), Error> { + ttl::extend_instance(&env); + + // Authenticate before the ownership comparison below. Without this, + // anyone could pass the current owner's address and take the asset — + // a direct asset-theft path. + caller.require_auth(); + // Check if contract is paused if Self::is_paused(env.clone())? { return Err(Error::ContractPaused); @@ -331,6 +375,7 @@ impl AssetUpContract { asset.last_transfer_timestamp = env.ledger().timestamp(); asset.status = AssetStatus::Transferred; store.set(&key, &asset); + ttl::extend_persistent(&env, &key); // Append audit log audit::append_audit_log( @@ -351,6 +396,11 @@ impl AssetUpContract { } pub fn retire_asset(env: Env, asset_id: BytesN<32>, caller: Address) -> Result<(), Error> { + ttl::extend_instance(&env); + + // Authenticate before the owner/admin comparison below. + caller.require_auth(); + // Check if contract is paused if Self::is_paused(env.clone())? { return Err(Error::ContractPaused); @@ -372,6 +422,7 @@ impl AssetUpContract { asset.status = AssetStatus::Retired; store.set(&key, &asset); + ttl::extend_persistent(&env, &key); // Append audit log audit::append_audit_log( @@ -392,10 +443,18 @@ impl AssetUpContract { } pub fn get_asset(env: Env, asset_id: BytesN<32>) -> Result { + ttl::extend_instance(&env); + let key = asset::DataKey::Asset(asset_id); let store = env.storage().persistent(); match store.get::<_, asset::Asset>(&key) { - Some(a) => Ok(a), + Some(a) => { + // Extend on read too. An asset that is only ever queried and + // never modified would otherwise be archived despite being in + // active use. + ttl::extend_persistent(&env, &key); + Ok(a) + } None => Err(Error::AssetNotFound), } } @@ -410,6 +469,8 @@ impl AssetUpContract { } pub fn check_asset_exists(env: Env, asset_id: BytesN<32>) -> Result { + ttl::extend_instance(&env); + let key = asset::DataKey::Asset(asset_id); let store = env.storage().persistent(); Ok(store.has(&key)) diff --git a/contracts/assetsup/src/math.rs b/contracts/assetsup/src/math.rs new file mode 100644 index 000000000..0353460be --- /dev/null +++ b/contracts/assetsup/src/math.rs @@ -0,0 +1,181 @@ +//! Checked arithmetic for value-carrying paths ([SC-43]). +//! +//! `overflow-checks = true` is set on the release profile, which turns an +//! overflow into a **panic** — a trapped transaction the caller cannot +//! distinguish from any other trap. On paths that move asset value, share +//! counts, dividends, or percentages, an explicit typed error is far more +//! useful: the caller learns *what* went wrong. +//! +//! Every helper here maps failure to [`Error::MathOverflow`] or +//! [`Error::MathUnderflow`] rather than trapping. +//! +//! ## Rounding +//! +//! [`mul_div`] truncates toward zero — the same direction as Rust's `/` on +//! non-negative operands, so it rounds **down** for the non-negative values +//! these contracts deal in. +//! +//! The remainder is **not** distributed. For a dividend split this means the +//! contract retains up to `holder_count - 1` stroops per distribution rather +//! than paying them to an arbitrarily chosen holder. That is the deliberate +//! choice: rounding in favour of the contract can never overpay, and picking a +//! holder to absorb the remainder would silently advantage whoever happens to +//! be first in iteration order. +//! +//! For an ownership percentage in basis points, rounding down means the +//! reported percentages can sum to slightly less than 10000. They are a +//! derived display value; the authoritative figure is always `balance`. + +use crate::error::Error; + +/// `a + b`, or [`Error::MathOverflow`]. +pub fn add(a: i128, b: i128) -> Result { + a.checked_add(b).ok_or(Error::MathOverflow) +} + +/// `a - b`, or [`Error::MathUnderflow`]. +pub fn sub(a: i128, b: i128) -> Result { + a.checked_sub(b).ok_or(Error::MathUnderflow) +} + +/// `a * b`, or [`Error::MathOverflow`]. +/// +/// Not currently called — [`mul_div`] covers every multiplication on a +/// value-carrying path today. Kept so the checked-arithmetic surface is +/// complete and a future caller reaches for it rather than a bare `*`. +#[allow(dead_code)] +pub fn mul(a: i128, b: i128) -> Result { + a.checked_mul(b).ok_or(Error::MathOverflow) +} + +/// `(a * b) / d`, rounding toward zero. +/// +/// The naive spelling `(a * b) / d` overflows whenever the intermediate +/// product exceeds `i128::MAX`, even when the final result is small — which is +/// exactly the case for proportional splits, where `balance * total_amount` is +/// large but the quotient is not. This computes the same value without +/// trapping, and returns [`Error::MathOverflow`] when the product genuinely +/// cannot be represented. +/// +/// Returns [`Error::MathOverflow`] if `d` is zero, since a zero denominator +/// here always means a corrupt total supply rather than a caller mistake. +pub fn mul_div(a: i128, b: i128, d: i128) -> Result { + if d == 0 { + return Err(Error::MathOverflow); + } + + // The common case: the product fits, so compute it directly. + if let Some(product) = a.checked_mul(b) { + return Ok(product / d); + } + + // The product does not fit. Reduce the fraction before multiplying by + // cancelling the greatest common divisor of each operand with the + // denominator, then retry. This rescues the realistic case where the + // denominator shares factors with one of the operands — a holder balance + // against a total supply, for instance. + let g1 = gcd(a.unsigned_abs(), d.unsigned_abs()); + let (a, d) = (a / g1 as i128, d / g1 as i128); + + let g2 = gcd(b.unsigned_abs(), d.unsigned_abs()); + let (b, d) = (b / g2 as i128, d / g2 as i128); + + match a.checked_mul(b) { + Some(product) => Ok(product / d), + None => Err(Error::MathOverflow), + } +} + +fn gcd(mut a: u128, mut b: u128) -> u128 { + while b != 0 { + let t = b; + b = a % b; + a = t; + } + if a == 0 { + 1 + } else { + a + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn add_reports_overflow_instead_of_trapping() { + assert_eq!(add(1, 2), Ok(3)); + assert_eq!(add(i128::MAX, 0), Ok(i128::MAX)); + assert_eq!(add(i128::MAX, 1), Err(Error::MathOverflow)); + } + + #[test] + fn sub_reports_underflow_instead_of_trapping() { + assert_eq!(sub(3, 2), Ok(1)); + assert_eq!(sub(0, 0), Ok(0)); + assert_eq!(sub(i128::MIN, 1), Err(Error::MathUnderflow)); + } + + #[test] + fn mul_reports_overflow_instead_of_trapping() { + assert_eq!(mul(0, i128::MAX), Ok(0)); + assert_eq!(mul(1, i128::MAX), Ok(i128::MAX)); + assert_eq!(mul(2, i128::MAX), Err(Error::MathOverflow)); + } + + #[test] + fn mul_div_handles_the_ordinary_case() { + // A 25% holder of a 1000-token supply receives 25% of 400. + assert_eq!(mul_div(250, 400, 1000), Ok(100)); + } + + #[test] + fn mul_div_survives_an_intermediate_product_that_would_overflow() { + // balance * total_amount overflows i128, but the result is small. + // Written naively as (a * b) / d this traps. + let balance = i128::MAX / 2; + let total_amount = 1000i128; + let total_supply = i128::MAX / 2; + + assert!(balance.checked_mul(total_amount).is_none()); + assert_eq!(mul_div(balance, total_amount, total_supply), Ok(1000)); + } + + #[test] + fn mul_div_rounds_down() { + // 1 of 3 holders splitting 10: each gets 3, one unit is retained. + assert_eq!(mul_div(1, 10, 3), Ok(3)); + assert_eq!(mul_div(2, 10, 3), Ok(6)); + // The remainder stays with the contract rather than being handed to a + // holder chosen by iteration order. + assert_eq!(mul_div(1, 10, 3).unwrap() * 3, 9); + } + + #[test] + fn mul_div_boundaries_at_zero_and_one() { + assert_eq!(mul_div(0, 1000, 10), Ok(0)); + assert_eq!(mul_div(1, 0, 10), Ok(0)); + assert_eq!(mul_div(1, 1, 1), Ok(1)); + assert_eq!(mul_div(i128::MAX, 1, i128::MAX), Ok(1)); + } + + #[test] + fn mul_div_rejects_a_zero_denominator() { + assert_eq!(mul_div(1, 1, 0), Err(Error::MathOverflow)); + } + + #[test] + fn mul_div_reports_overflow_when_the_result_truly_cannot_fit() { + // Coprime operands with a denominator of 1: nothing can be cancelled + // and the product genuinely does not fit. + assert_eq!(mul_div(i128::MAX, 3, 1), Err(Error::MathOverflow)); + } + + #[test] + fn gcd_is_well_behaved_at_zero() { + assert_eq!(gcd(0, 0), 1); + assert_eq!(gcd(0, 5), 5); + assert_eq!(gcd(12, 18), 6); + } +} diff --git a/contracts/assetsup/src/tests/auth.rs b/contracts/assetsup/src/tests/auth.rs new file mode 100644 index 000000000..032a63fe3 --- /dev/null +++ b/contracts/assetsup/src/tests/auth.rs @@ -0,0 +1,332 @@ +//! Authorization tests ([SC-42]). +//! +//! Every test here runs **without** `mock_all_auths`, which is the only way to +//! prove an entrypoint actually authenticates. With auths mocked, an entrypoint +//! that never calls `require_auth` is indistinguishable from one that does. +//! +//! The regression these guard against: `register_asset`, +//! `update_asset_metadata`, `transfer_asset_ownership` and `retire_asset` each +//! take a `caller: Address` and compare it against a registrar allowlist, the +//! asset owner, or the admin — but originally never called +//! `caller.require_auth()`. Because `caller` is supplied by whoever builds the +//! transaction, the comparison could be satisfied by simply naming a privileged +//! address. `transfer_asset_ownership` was a direct asset-theft path. + +use soroban_sdk::testutils::Address as _; +use soroban_sdk::{Address, BytesN, Env, IntoVal, String, Vec}; + +use super::helpers::{create_env, create_test_asset}; +use crate::{AssetUpContract, AssetUpContractClient}; + +/// Registers the contract and initializes it, then clears the mocked auths so +/// every subsequent call must carry real authorization. +fn setup_unmocked(env: &Env) -> (AssetUpContractClient<'_>, Address) { + let admin = Address::generate(env); + let contract_id = env.register(AssetUpContract, ()); + let client = AssetUpContractClient::new(env, &contract_id); + + env.mock_all_auths(); + client.initialize(&admin); + env.set_auths(&[]); + + (client, admin) +} + +fn asset_id(env: &Env, seed: u8) -> BytesN<32> { + BytesN::from_array(env, &[seed; 32]) +} + +// --------------------------------------------------------------------------- +// initialize +// --------------------------------------------------------------------------- + +#[test] +fn initialize_requires_the_admins_authorization() { + let env = create_env(); + let admin = Address::generate(&env); + let contract_id = env.register(AssetUpContract, ()); + let client = AssetUpContractClient::new(&env, &contract_id); + + let res = client.try_initialize(&admin); + assert!( + res.is_err(), + "initialize must not succeed without the admin's authorization" + ); +} + +// --------------------------------------------------------------------------- +// register_asset +// --------------------------------------------------------------------------- + +#[test] +fn register_asset_rejects_an_unauthenticated_caller() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let asset = create_test_asset(&env, &owner, asset_id(&env, 1)); + + // `admin` is an authorized registrar, but this call carries no signature + // from them. + let res = client.try_register_asset(&asset, &admin); + assert!( + res.is_err(), + "naming an authorized registrar must not be enough to register an asset" + ); +} + +#[test] +fn register_asset_succeeds_once_the_caller_authenticates() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 2); + let asset = create_test_asset(&env, &owner, id.clone()); + + env.mock_all_auths(); + client.register_asset(&asset, &admin); + + assert_eq!(client.get_asset(&id).owner, owner); +} + +// --------------------------------------------------------------------------- +// transfer_asset_ownership — the asset-theft path +// --------------------------------------------------------------------------- + +#[test] +fn transfer_asset_ownership_rejects_an_unauthenticated_caller() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 3); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + // An attacker names the real owner as `caller` and tries to move the asset + // to themselves. Without authentication this would succeed. + env.set_auths(&[]); + let attacker = Address::generate(&env); + let res = client.try_transfer_asset_ownership(&id, &attacker, &owner); + + assert!( + res.is_err(), + "naming the owner must not be enough to transfer their asset" + ); + assert_eq!( + client.get_asset(&id).owner, + owner, + "ownership must be unchanged after the rejected transfer" + ); +} + +#[test] +fn transfer_asset_ownership_succeeds_for_the_authenticated_owner() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let new_owner = Address::generate(&env); + let id = asset_id(&env, 4); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + client.transfer_asset_ownership(&id, &new_owner, &owner); + + assert_eq!(client.get_asset(&id).owner, new_owner); +} + +// --------------------------------------------------------------------------- +// update_asset_metadata +// --------------------------------------------------------------------------- + +#[test] +fn update_asset_metadata_rejects_an_unauthenticated_caller() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 5); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + env.set_auths(&[]); + let res = client.try_update_asset_metadata( + &id, + &Some(String::from_str(&env, "rewritten")), + &None, + &None, + &owner, + ); + + assert!( + res.is_err(), + "naming the owner must not be enough to rewrite their asset's metadata" + ); +} + +// --------------------------------------------------------------------------- +// retire_asset +// --------------------------------------------------------------------------- + +#[test] +fn retire_asset_rejects_an_unauthenticated_caller() { + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 6); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + env.set_auths(&[]); + let res = client.try_retire_asset(&id, &owner); + + assert!( + res.is_err(), + "naming the owner must not be enough to retire their asset" + ); + assert_eq!( + client.get_asset(&id).status, + crate::types::AssetStatus::Active, + "the asset must still be active after the rejected retire" + ); +} + +// --------------------------------------------------------------------------- +// Admin-gated entrypoints +// --------------------------------------------------------------------------- + +#[test] +fn update_admin_requires_the_current_admins_authorization() { + let env = create_env(); + let (client, _admin) = setup_unmocked(&env); + let usurper = Address::generate(&env); + + let res = client.try_update_admin(&usurper); + assert!( + res.is_err(), + "admin transfer must not succeed without the current admin's auth" + ); +} + +#[test] +fn add_authorized_registrar_requires_the_admins_authorization() { + let env = create_env(); + let (client, _admin) = setup_unmocked(&env); + let candidate = Address::generate(&env); + + let res = client.try_add_authorized_registrar(&candidate); + assert!( + res.is_err(), + "granting registrar rights must require the admin's auth" + ); + // And the grant must not have taken effect. + assert!(!client.is_authorized_registrar(&candidate)); +} + +#[test] +fn remove_authorized_registrar_requires_the_admins_authorization() { + let env = create_env(); + let (client, _admin) = setup_unmocked(&env); + let candidate = Address::generate(&env); + + env.mock_all_auths(); + client.add_authorized_registrar(&candidate); + assert!(client.is_authorized_registrar(&candidate)); + + env.set_auths(&[]); + let res = client.try_remove_authorized_registrar(&candidate); + assert!( + res.is_err(), + "revoking registrar rights must require the admin's auth" + ); + assert!(client.is_authorized_registrar(&candidate)); +} + +#[test] +fn pause_and_unpause_require_the_admins_authorization() { + let env = create_env(); + let (client, _admin) = setup_unmocked(&env); + + assert!( + client.try_pause_contract().is_err(), + "pause must require the admin's auth" + ); + assert!(!client.is_paused()); + + env.mock_all_auths(); + client.pause_contract(); + assert!(client.is_paused()); + + env.set_auths(&[]); + assert!( + client.try_unpause_contract().is_err(), + "unpause must require the admin's auth" + ); + assert!(client.is_paused(), "the contract must still be paused"); +} + +// --------------------------------------------------------------------------- +// Reads stay open +// --------------------------------------------------------------------------- + +#[test] +fn read_entrypoints_do_not_require_authorization() { + // The audit is only useful if it distinguishes "protected" from "locked + // down"; reads must stay callable by anyone. + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 7); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + env.set_auths(&[]); + + assert_eq!(client.get_asset(&id).owner, owner); + assert!(client.check_asset_exists(&id)); + assert_eq!(client.get_total_asset_count(), 1); + assert_eq!(client.get_admin(), admin); + assert!(!client.is_paused()); + assert_eq!(client.get_assets_by_owner(&owner).len(), 1); + assert_eq!(client.get_asset_info(&id).id, id); + + let ids: Vec> = Vec::from_array(&env, [id.clone()]); + assert_eq!(client.batch_get_asset_info(&ids).len(), 1); +} + +// --------------------------------------------------------------------------- +// The auth is bound to the right principal, not just "some" auth +// --------------------------------------------------------------------------- + +#[test] +fn a_third_party_signature_does_not_authorize_a_transfer() { + // `mock_auths` grants exactly one address's authorization. An attacker who + // can sign for themselves must still not be able to move someone else's + // asset by naming the owner as `caller`. + let env = create_env(); + let (client, admin) = setup_unmocked(&env); + let owner = Address::generate(&env); + let attacker = Address::generate(&env); + let id = asset_id(&env, 8); + + env.mock_all_auths(); + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + env.set_auths(&[soroban_sdk::testutils::MockAuth { + address: &attacker, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &client.address, + fn_name: "transfer_asset_ownership", + args: (id.clone(), attacker.clone(), owner.clone()).into_val(&env), + sub_invokes: &[], + }, + } + .into()]); + + let res = client.try_transfer_asset_ownership(&id, &attacker, &owner); + assert!( + res.is_err(), + "the attacker's own signature must not authorize the owner's transfer" + ); + assert_eq!(client.get_asset(&id).owner, owner); +} diff --git a/contracts/assetsup/src/tests/mod.rs b/contracts/assetsup/src/tests/mod.rs index a44a81fdf..0f87bfca6 100644 --- a/contracts/assetsup/src/tests/mod.rs +++ b/contracts/assetsup/src/tests/mod.rs @@ -5,6 +5,7 @@ mod helpers; mod admin; mod asset; mod audit_trail; +mod auth; mod initialization; // Tokenization and ownership tests @@ -28,3 +29,6 @@ mod integration; mod tokenization_new; mod transfer_restrictions_new; mod voting_new; + +// Storage TTL policy tests +mod ttl; diff --git a/contracts/assetsup/src/tests/ttl.rs b/contracts/assetsup/src/tests/ttl.rs new file mode 100644 index 000000000..30ff2abed --- /dev/null +++ b/contracts/assetsup/src/tests/ttl.rs @@ -0,0 +1,116 @@ +//! Storage TTL tests ([SC-44]). +//! +//! These advance the ledger past the default entry lifetime and prove critical +//! registry entries are still readable. Without the `extend_ttl` calls in +//! `lib.rs` these fail: Soroban archives the entry and the asset simply +//! disappears from the contract's view. + +use soroban_sdk::testutils::{Address as _, Ledger as _, LedgerInfo}; +use soroban_sdk::{Address, BytesN, Env}; + +use super::helpers::{create_env, create_test_asset, initialize_contract}; +use crate::ttl::{LEDGERS_PER_DAY, PERSISTENT_EXTEND_TO}; +use crate::AssetUpContractClient; + +fn asset_id(env: &Env, seed: u8) -> BytesN<32> { + BytesN::from_array(env, &[seed; 32]) +} + +/// Sets a ledger with a short default entry lifetime so archival is reachable +/// within a test, then advances by `ledgers`. +fn advance_ledgers(env: &Env, ledgers: u32) { + env.ledger().with_mut(|li: &mut LedgerInfo| { + li.sequence_number += ledgers; + li.timestamp += (ledgers as u64) * 5; + }); +} + +fn setup(env: &Env) -> (AssetUpContractClient<'_>, Address) { + // A generous max entry TTL so the contract's own extension is what keeps + // entries alive, rather than the harness silently capping it. + env.ledger().with_mut(|li: &mut LedgerInfo| { + li.sequence_number = 1; + li.min_persistent_entry_ttl = 100; + li.min_temp_entry_ttl = 16; + li.max_entry_ttl = PERSISTENT_EXTEND_TO + LEDGERS_PER_DAY; + }); + + let admin = Address::generate(env); + let client = initialize_contract(env, &admin); + (client, admin) +} + +#[test] +fn a_registered_asset_survives_past_the_default_entry_lifetime() { + let env = create_env(); + let (client, admin) = setup(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 1); + + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + // Well past the 100-ledger default set above, and past the point where an + // unextended entry would have been archived. + advance_ledgers(&env, 60 * LEDGERS_PER_DAY); + + let asset = client.get_asset(&id); + assert_eq!( + asset.owner, owner, + "the ownership record must still be readable" + ); +} + +#[test] +fn reading_an_asset_keeps_it_alive() { + // The property that makes read-side extension necessary: an asset that is + // queried but never modified must not be archived. + let env = create_env(); + let (client, admin) = setup(&env); + let owner = Address::generate(&env); + let id = asset_id(&env, 2); + + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + + // Three hops, each shorter than the extension window, reading each time. + // Cumulatively they exceed the window, so only read-side extension keeps + // the entry alive to the end. + for _ in 0..3 { + advance_ledgers(&env, 45 * LEDGERS_PER_DAY); + let asset = client.get_asset(&id); + assert_eq!(asset.owner, owner); + } + + assert!(client.check_asset_exists(&id)); +} + +#[test] +fn a_transferred_asset_survives_past_the_default_lifetime() { + let env = create_env(); + let (client, admin) = setup(&env); + let owner = Address::generate(&env); + let new_owner = Address::generate(&env); + let id = asset_id(&env, 3); + + client.register_asset(&create_test_asset(&env, &owner, id.clone()), &admin); + client.transfer_asset_ownership(&id, &new_owner, &owner); + + advance_ledgers(&env, 60 * LEDGERS_PER_DAY); + + assert_eq!( + client.get_asset(&id).owner, + new_owner, + "the post-transfer ownership record must survive" + ); +} + +#[test] +fn contract_configuration_survives_past_the_default_lifetime() { + let env = create_env(); + let (client, admin) = setup(&env); + + advance_ledgers(&env, 60 * LEDGERS_PER_DAY); + + assert_eq!(client.get_admin(), admin, "admin must still be readable"); + assert!(!client.is_paused()); + assert!(client.is_authorized_registrar(&admin)); +} diff --git a/contracts/assetsup/src/tokenization.rs b/contracts/assetsup/src/tokenization.rs index 7402eb8d0..beb71973d 100644 --- a/contracts/assetsup/src/tokenization.rs +++ b/contracts/assetsup/src/tokenization.rs @@ -1,5 +1,6 @@ use crate::audit; use crate::error::Error; +use crate::math; use crate::types::{OwnershipRecord, TokenDataKey, TokenMetadata, TokenizedAsset}; use soroban_sdk::{Address, BytesN, Env, String, Vec}; @@ -126,19 +127,21 @@ pub fn mint_tokens( } // Update total supply - tokenized_asset.total_supply += amount; - tokenized_asset.tokens_in_circulation += amount; + tokenized_asset.total_supply = math::add(tokenized_asset.total_supply, amount)?; + tokenized_asset.tokens_in_circulation = + math::add(tokenized_asset.tokens_in_circulation, amount)?; // Update tokenizer's ownership let holder_key = TokenDataKey::TokenHolder(asset_id, minter.clone()); let mut ownership: OwnershipRecord = store.get(&holder_key).ok_or(Error::HolderNotFound)?; - ownership.balance += amount; + ownership.balance = math::add(ownership.balance, amount)?; ownership.voting_power = ownership.balance; ownership.dividend_entitlement = ownership.balance; // Recalculate ownership percentage - ownership.ownership_percentage = (ownership.balance * 10000) / tokenized_asset.total_supply; + ownership.ownership_percentage = + math::mul_div(ownership.balance, 10000, tokenized_asset.total_supply)?; store.set(&holder_key, &ownership); store.set(&key, &tokenized_asset.clone()); @@ -194,15 +197,17 @@ pub fn burn_tokens( } // Update balances - ownership.balance -= amount; + ownership.balance = math::sub(ownership.balance, amount)?; ownership.voting_power = ownership.balance; ownership.dividend_entitlement = ownership.balance; // Recalculate ownership percentage - ownership.ownership_percentage = (ownership.balance * 10000) / tokenized_asset.total_supply; + ownership.ownership_percentage = + math::mul_div(ownership.balance, 10000, tokenized_asset.total_supply)?; - tokenized_asset.total_supply -= amount; - tokenized_asset.tokens_in_circulation -= amount; + tokenized_asset.total_supply = math::sub(tokenized_asset.total_supply, amount)?; + tokenized_asset.tokens_in_circulation = + math::sub(tokenized_asset.tokens_in_circulation, amount)?; store.set(&holder_key, &ownership); store.set(&key, &tokenized_asset.clone()); @@ -282,17 +287,17 @@ pub fn transfer_tokens( }; // Update balances - from_ownership.balance -= amount; + from_ownership.balance = math::sub(from_ownership.balance, amount)?; from_ownership.voting_power = from_ownership.balance; from_ownership.dividend_entitlement = from_ownership.balance; from_ownership.ownership_percentage = - (from_ownership.balance * 10000) / tokenized_asset.total_supply; + math::mul_div(from_ownership.balance, 10000, tokenized_asset.total_supply)?; - to_ownership.balance += amount; + to_ownership.balance = math::add(to_ownership.balance, amount)?; to_ownership.voting_power = to_ownership.balance; to_ownership.dividend_entitlement = to_ownership.balance; to_ownership.ownership_percentage = - (to_ownership.balance * 10000) / tokenized_asset.total_supply; + math::mul_div(to_ownership.balance, 10000, tokenized_asset.total_supply)?; store.set(&from_holder_key, &from_ownership); store.set(&to_holder_key, &to_ownership); @@ -433,7 +438,7 @@ pub fn calculate_ownership_percentage( return Ok(0); } - Ok((ownership.balance * 10000) / tokenized_asset.total_supply) + math::mul_div(ownership.balance, 10000, tokenized_asset.total_supply) } /// Get tokenized asset details diff --git a/contracts/assetsup/src/ttl.rs b/contracts/assetsup/src/ttl.rs new file mode 100644 index 000000000..a31222d31 --- /dev/null +++ b/contracts/assetsup/src/ttl.rs @@ -0,0 +1,84 @@ +//! Storage time-to-live policy ([SC-44]). +//! +//! Soroban charges rent and **archives entries whose TTL lapses**. An archived +//! asset registry entry is a correctness bug, not a performance concern: the +//! ownership record simply stops being readable until someone restores it. +//! +//! All TTL constants live here rather than being scattered as magic numbers at +//! call sites, so the policy can be reviewed in one place. +//! +//! ## Durability choices +//! +//! | Data | Durability | Why | +//! |---|---|---| +//! | Asset records, ownership, token balances, leases, policies | `persistent` | Must outlive any single session. Losing one loses the ownership record. | +//! | Admin, pause flag, registrar allowlist, counters, metadata | `persistent` | Contract-level configuration with the same lifetime as the contract. | +//! | — | `temporary` | Nothing in this contract is short-lived enough to justify it. Long-lived data in `temporary` is the classic archival bug. | +//! +//! ## Policy +//! +//! Every read and write of a persistent entry extends its TTL. Extending on +//! **read** as well as write is the important part: an asset that is only ever +//! queried, never modified, would otherwise expire despite being actively used. +//! +//! The threshold/extend pair follows the usual shape — if the entry has fewer +//! than `*_THRESHOLD` ledgers left, push it back out to `*_EXTEND_TO`. Setting +//! the threshold below the target means the extension is a no-op on most calls +//! and only costs rent when an entry is genuinely approaching expiry. + +use soroban_sdk::{Env, IntoVal, Val}; + +/// Ledgers per day, at the nominal 5 second close time. +pub const LEDGERS_PER_DAY: u32 = 17_280; + +/// Bump persistent entries when they have less than 30 days left... +pub const PERSISTENT_THRESHOLD: u32 = 30 * LEDGERS_PER_DAY; +/// ...back out to 90 days. +/// +/// Chosen so a contract that goes untouched for a full quarter still retains +/// its registry, and so that routine traffic keeps entries alive without +/// paying to extend on every call. +pub const PERSISTENT_EXTEND_TO: u32 = 90 * LEDGERS_PER_DAY; + +/// Instance storage (contract-level config) uses the same window. If the +/// instance is archived the contract is unusable regardless of what else +/// survives, so it should never be the first thing to lapse. +pub const INSTANCE_THRESHOLD: u32 = PERSISTENT_THRESHOLD; +pub const INSTANCE_EXTEND_TO: u32 = PERSISTENT_EXTEND_TO; + +/// Extends a persistent entry's TTL, if it exists. +/// +/// Safe to call on a key that has never been written; `extend_ttl` on a +/// missing entry would trap, so existence is checked first. +pub fn extend_persistent(env: &Env, key: &K) +where + K: IntoVal + Clone, +{ + let store = env.storage().persistent(); + if store.has(key) { + store.extend_ttl(key, PERSISTENT_THRESHOLD, PERSISTENT_EXTEND_TO); + } +} + +/// Bumps the contract instance TTL. +/// +/// Call from every entrypoint that touches instance storage, including reads. +pub fn extend_instance(env: &Env) { + env.storage() + .instance() + .extend_ttl(INSTANCE_THRESHOLD, INSTANCE_EXTEND_TO); +} + +/// The extension target must be further out than the trigger point, otherwise +/// every call would pay to extend while never actually pushing the entry out. +/// +/// Checked at compile time; the values are constants, so a runtime assertion +/// would be trivially true and clippy rightly rejects it. +const _: () = { + assert!(PERSISTENT_THRESHOLD < PERSISTENT_EXTEND_TO); + assert!(INSTANCE_THRESHOLD < INSTANCE_EXTEND_TO); + // A quarter of headroom, so a registry is not archived during a quiet + // period. + assert!(PERSISTENT_EXTEND_TO == 90 * LEDGERS_PER_DAY); + assert!(PERSISTENT_THRESHOLD >= 30 * LEDGERS_PER_DAY); +}; diff --git a/contracts/contrib/src/lib.rs b/contracts/contrib/src/lib.rs index 539797f3d..ba98f21d3 100644 --- a/contracts/contrib/src/lib.rs +++ b/contracts/contrib/src/lib.rs @@ -62,6 +62,10 @@ pub struct ContribContract; impl ContribContract { /// Initialize the contract with an admin. pub fn initialize(env: Env, admin: Address) { + // Without this, whoever calls initialize first becomes admin of a + // freshly deployed contract, regardless of who deployed it. + admin.require_auth(); + if env.storage().persistent().has(&DataKey::Admin) { panic!("Already initialized"); } diff --git a/contracts/contrib/src/tests/asset_registry_tests.rs b/contracts/contrib/src/tests/asset_registry_tests.rs index b00991fa2..7ac7bfee5 100644 --- a/contracts/contrib/src/tests/asset_registry_tests.rs +++ b/contracts/contrib/src/tests/asset_registry_tests.rs @@ -7,6 +7,8 @@ fn setup_test(env: &Env) -> (ContribContractClient<'_>, Address) { let admin = Address::generate(env); let contract_id = env.register(ContribContract, ()); let client = ContribContractClient::new(env, &contract_id); + // initialize now requires the admin's authorization. + env.mock_all_auths(); client.initialize(&admin); (client, admin) } diff --git a/contracts/multisig_transfer/src/auth_tests.rs b/contracts/multisig_transfer/src/auth_tests.rs new file mode 100644 index 000000000..e4ddb2b92 --- /dev/null +++ b/contracts/multisig_transfer/src/auth_tests.rs @@ -0,0 +1,162 @@ +//! Authorization tests ([SC-42]). +//! +//! Every entrypoint in this crate compared a caller-supplied `caller` argument +//! against the stored admin, the asset's owner, or the approver set — and none +//! authenticated it. Approvals could be forged by naming an authorized +//! approver; approval rules could be rewritten by naming the admin. +//! +//! These tests run **without** `mock_all_auths`, which is the only way to tell +//! an entrypoint that authenticates from one that merely looks like it does. +#![cfg(test)] + +use soroban_sdk::testutils::Address as _; +use soroban_sdk::{Address, BytesN, Env}; + +use crate::types::ApprovalRule; +use crate::{MultiSigTransferContract, MultiSigTransferContractClient}; + +fn setup(env: &Env) -> (MultiSigTransferContractClient<'_>, Address) { + let contract_id = env.register(MultiSigTransferContract, ()); + let client = MultiSigTransferContractClient::new(env, &contract_id); + let admin = Address::generate(env); + let registry = Address::generate(env); + + env.mock_all_auths(); + client.initialize(&admin, ®istry); + env.set_auths(&[]); + + (client, admin) +} + +fn category(env: &Env) -> BytesN<32> { + BytesN::from_array(env, &[9u8; 32]) +} + +fn rule(env: &Env, required: u32) -> ApprovalRule { + ApprovalRule { + category: category(env), + required_approvals: required, + approvers: soroban_sdk::Vec::from_array(env, [Address::generate(env)]), + approval_timeout_secs: 3600, + auto_approve: false, + priority: 1, + } +} + +#[test] +fn initialize_requires_the_admins_authorization() { + // Otherwise whoever calls initialize first owns a freshly deployed + // contract, regardless of who deployed it. + let env = Env::default(); + let contract_id = env.register(MultiSigTransferContract, ()); + let client = MultiSigTransferContractClient::new(&env, &contract_id); + + let res = client.try_initialize(&Address::generate(&env), &Address::generate(&env)); + assert!( + res.is_err(), + "initialize must require the incoming admin's authorization" + ); +} + +#[test] +fn configure_approval_rule_rejects_an_unauthenticated_caller() { + // Naming the admin must not be enough to rewrite the approval threshold — + // that would let anyone drop it to one and self-approve every transfer. + let env = Env::default(); + let (client, admin) = setup(&env); + + let res = client.try_configure_approval_rule(&admin, &rule(&env, 1)); + assert!( + res.is_err(), + "naming the admin must not authorize a rule change" + ); +} + +#[test] +fn configure_approval_rule_succeeds_once_the_admin_authenticates() { + let env = Env::default(); + let (client, admin) = setup(&env); + + env.mock_all_auths(); + client.configure_approval_rule(&admin, &rule(&env, 2)); + + // The rule is stored, observable through its approver list. + assert_eq!( + client + .get_required_approvers_category(&category(&env)) + .len(), + 1 + ); +} + +#[test] +fn a_non_admin_cannot_configure_an_approval_rule_even_when_authenticated() { + // Authentication is necessary but not sufficient: the admin check must + // still apply to a caller who can legitimately sign for themselves. + let env = Env::default(); + let (client, _admin) = setup(&env); + let stranger = Address::generate(&env); + + env.mock_all_auths(); + let res = client.try_configure_approval_rule(&stranger, &rule(&env, 1)); + assert!( + res.is_err(), + "a signed call from a non-admin must still be rejected" + ); +} + +#[test] +fn approve_transfer_request_rejects_an_unauthenticated_caller() { + let env = Env::default(); + let (client, _admin) = setup(&env); + let approver = Address::generate(&env); + + // The request does not exist, so this would fail either way — but it must + // fail on authorization first, before any state is read. + let res = client.try_approve_transfer_request(&approver, &1); + assert!( + res.is_err(), + "approvals must not be forgeable by naming an approver" + ); +} + +#[test] +fn cancel_transfer_request_rejects_an_unauthenticated_caller() { + let env = Env::default(); + let (client, admin) = setup(&env); + + let res = client.try_cancel_transfer_request(&admin, &1); + assert!( + res.is_err(), + "cancellation must not be forgeable by naming the admin" + ); +} + +#[test] +fn reject_transfer_request_rejects_an_unauthenticated_caller() { + let env = Env::default(); + let (client, _admin) = setup(&env); + let approver = Address::generate(&env); + + let res = + client.try_reject_transfer_request(&approver, &1, &BytesN::from_array(&env, &[0u8; 32])); + assert!( + res.is_err(), + "rejections must not be forgeable by naming an approver" + ); +} + +#[test] +fn read_entrypoints_do_not_require_authorization() { + let env = Env::default(); + let (client, _admin) = setup(&env); + + // No auths are mocked; these must still answer. + assert_eq!(client.get_asset_history(&category(&env)).len(), 0); + assert_eq!( + client + .get_pending_transfers_approver(&Address::generate(&env)) + .len(), + 0 + ); +} diff --git a/contracts/multisig_transfer/src/lib.rs b/contracts/multisig_transfer/src/lib.rs index 5cb3fc176..06e0dad28 100644 --- a/contracts/multisig_transfer/src/lib.rs +++ b/contracts/multisig_transfer/src/lib.rs @@ -3,6 +3,7 @@ use soroban_sdk::{contract, contractimpl, Address, BytesN, Env, Vec}; mod approvals; +mod auth_tests; mod errors; mod events; mod registry; @@ -24,6 +25,10 @@ impl MultiSigTransferContract { // Init // ---------------------------- pub fn initialize(e: Env, admin: Address, asset_registry: Address) { + // Without this, whoever calls initialize first becomes admin of a + // freshly deployed contract, regardless of who deployed it. + admin.require_auth(); + storage::set_admin(&e, &admin); storage::set_registry(&e, &asset_registry); e.storage() @@ -39,6 +44,10 @@ impl MultiSigTransferContract { caller: Address, rule: ApprovalRule, ) -> Result<(), MultiSigError> { + // require_admin only compares `caller` against the stored admin. + // Without authenticating `caller` first, anyone could pass the admin's + // address and pass that check. + caller.require_auth(); utils::require_admin(&e, &caller)?; let mut rules_map = storage::rules_map(&e); @@ -63,6 +72,9 @@ impl MultiSigTransferContract { expires_at: u64, execute_after: Option, ) -> Result { + // The ownership check below compares the registry's owner against a + // caller-supplied address; authenticate it first. + caller.require_auth(); let (_admin, registry_addr) = utils::require_init(&e)?; if caller == new_owner { @@ -155,6 +167,10 @@ impl MultiSigTransferContract { caller: Address, request_id: u64, ) -> Result<(), MultiSigError> { + // The approver-membership check below compares against a + // caller-supplied address; authenticate it first or approvals can be + // forged by naming an authorized approver. + caller.require_auth(); let (_admin, _registry) = utils::require_init(&e)?; let mut requests = storage::requests_map(&e); @@ -211,6 +227,7 @@ impl MultiSigTransferContract { request_id: u64, reason_hash: BytesN<32>, ) -> Result<(), MultiSigError> { + caller.require_auth(); let (_admin, _registry) = utils::require_init(&e)?; let mut requests = storage::requests_map(&e); @@ -245,7 +262,12 @@ impl MultiSigTransferContract { // ---------------------------- pub fn execute_transfer(e: Env, caller: Address, request_id: u64) -> Result<(), MultiSigError> { let (_admin, registry_addr) = utils::require_init(&e)?; - let _ = caller; // anyone can execute, kept for audit if desired + // Deliberately permissionless, like multisig-wallet's execute_*: the + // authorization decision was already made by the approvers, and + // requiring one of them to also submit the execution adds liveness + // risk without adding security. `caller` is retained for the audit + // trail only and is intentionally not authenticated. + let _ = caller; let mut requests = storage::requests_map(&e); let mut req = requests @@ -291,6 +313,9 @@ impl MultiSigTransferContract { caller: Address, request_id: u64, ) -> Result<(), MultiSigError> { + // Cancellation is restricted to the requester or the admin, both + // compared against a caller-supplied address. + caller.require_auth(); let (admin, _registry) = utils::require_init(&e)?; let mut requests = storage::requests_map(&e);