diff --git a/SECURITY_FIXES.md b/SECURITY_FIXES.md index 245b915..62db1e2 100644 --- a/SECURITY_FIXES.md +++ b/SECURITY_FIXES.md @@ -198,5 +198,101 @@ cargo test --package farming-pool --- +## Issue #406: vesting-wallet clawback access control + +### Problem +The issue reported that `clawback` in `contracts/vesting-wallet/src/lib.rs` +"may not require proper admin authorization, allowing anyone to clawback +vested tokens", and suggested adding `admin.require_auth()` to a +`clawback(env, admin, beneficiary, amount)` function. + +### Analysis +The reported function **does not exist** anywhere in the contract: +`grep -n "clawback" soroban/contracts/vesting-wallet/src/*.rs` returns no +matches. There is therefore no unauthenticated clawback to fix. + +Auditing every entry point that moves value or changes authority confirms the +access control the issue is actually worried about ("No access control on +sensitive operations") is already in place: + +| Entry point | Authorization | +|---|---| +| `initialize` | `admin.require_auth()` (funds are pulled from the admin) | +| `release` | beneficiary `require_auth()` — see `test_release_requires_beneficiary_auth` | +| `revoke` | stored admin `require_auth()` | +| `emergency_withdraw` | stored admin `require_auth()` (drains to the admin) | +| `transfer_beneficiary` | stored admin `require_auth()` | +| `transfer_admin` | current admin `require_auth()` | + +The contract also has no notion of a token-level clawback at all: it only +ever holds and releases the vested `total_amount` through the token's +`transfer`, so a `clawback` entry point would be a **new** admin capability +rather than a fix. + +### Fix Applied +**No contract change** — deliberately not adding a `clawback` function. Adding +a sensitive admin operation that the codebase has never had would expand the +attack surface this issue was raised to protect, and nothing in the issue +describes the intended semantics (who receives the clawed-back amount, how it +interacts with `released_amount`, or whether it is allowed after revocation). + +What was added instead: + +1. This audit note, so the report is not silently re-raised. +2. A regression test, + `test_admin_only_entry_points_require_admin_auth` (#406), that authorizes + **only a non-admin address** and asserts every admin-gated entry point + rejects the call and leaves state untouched. A future entry point added + without `require_auth()` fails this test. + +### Verification +```bash +cargo test --package vesting-wallet test_admin_only_entry_points_require_admin_auth +``` + +--- + +## Issue #398: farming-pool lock_assets reentrancy + +### Problem +The issue reported that `lock_assets` calls `token::transfer`, which could be +a malicious contract that reenters the pool and manipulates state mid-transfer. + +### Analysis +**The code already follows checks-effects-interactions.** Verified in +`contracts/farming-pool/src/lib.rs`: + +- All validations (`amount <= 0`, `MAX_STAKE_AMOUNT`, minimum stake, whitelist) + run before any state change. +- The position — including the extended `unlock_ledger` and the recomputed + `credit_rate` — is written with `set_position` **before** + `token::Client::new(&env, &stake_token).transfer(...)` is invoked, so a + reentrant observer sees the final post-deposit state, never a partial one. +- A second, independent layer sits below the code: Soroban's + `ContractReentryMode` defaults to `Prohibited`, so a token attempting to + call back into `FarmingPool` during the transfer traps with + "Contract re-entry is not allowed" before any of this contract's code runs. + +### Fix Applied +**No behavior change required.** Per the issue's instruction to "verify this is +sufficient and add documentation": + +1. A `# Reentrancy posture (#398)` section on `lock_assets` documenting both + defense layers, the CEI ordering, the `()`-returns/traps-on-failure + transfer semantics (#363), and the two tests that prove it. +2. This note. + +### Verification +```bash +cargo test --package farming-pool test_lock_assets_reentrant +``` +Both existing tests pass without modification: +`test_lock_assets_reentrant_transfer_is_rejected_and_final_state_is_correct` +(graceful reentry is rejected, final position is correct) and +`test_lock_assets_reverts_entirely_if_stake_token_naively_reenters` (a naive +reentry traps the whole call and no partial position survives). + +--- + Last Updated: 2024-01-01 -Fixed Issues: #357, #358, #363, #364 +Fixed Issues: #357, #358, #363, #364, #398 (verification + docs), #406 (audit + regression test) diff --git a/soroban/contracts/factory/src/lib.rs b/soroban/contracts/factory/src/lib.rs index f7f3750..e9ccd87 100644 --- a/soroban/contracts/factory/src/lib.rs +++ b/soroban/contracts/factory/src/lib.rs @@ -710,10 +710,14 @@ impl Factory { /// Callers can specify `scan_limit` (up to 50) to tune the scan window. Callers resume pagination /// using `next_start_id` until `next_start_id == total`. /// - /// # Indexer Recommendation - /// For off-chain applications (such as frontends and analytics) requiring zero-gas instant lookups - /// across thousands of pools, developers should index the `(symbol_short!("factory"), symbol_short!("pool_crtd"))` - /// events emitted by `create_pool`, which include `asset` and `pool_id` in their payload. + /// # Asset Index + /// `create_pool` maintains a secondary on-chain index, `DataKey::AssetPools(asset) -> Vec` + /// (with `DataKey::AssetPoolCount(asset)` as its constant-time companion), so this lookup reads + /// only the pool IDs registered for `asset` instead of walking the whole registry. The bounded + /// registry scan below is only a fallback for records that predate the index (#397). Off-chain + /// applications that need lookups across thousands of pools with no transaction at all should + /// still index the `(symbol_short!("factory"), symbol_short!("pool_crtd"))` events emitted by + /// `create_pool`, which include `asset` and `pool_id` in their payload. /// /// Returns `NotInitialized` if the factory has not been initialized. pub fn get_pools_by_asset_range( diff --git a/soroban/contracts/factory/src/test.rs b/soroban/contracts/factory/src/test.rs index 9173786..64c7474 100644 --- a/soroban/contracts/factory/src/test.rs +++ b/soroban/contracts/factory/src/test.rs @@ -12,6 +12,10 @@ use soroban_sdk::{ use farming_pool::FarmingPoolClient; +// The contract crate is `#![no_std]`; these tests assert on `std` collection +// types, so the shim is declared here (same as farming-pool's tests). +extern crate std; + // ── Helpers ─────────────────────────────────────────────────────────────────── struct TestEnv { @@ -1678,3 +1682,120 @@ fn test_list_pools_reports_missing_records_with_a_pool_gap_event() { ] ); } + +// ── asset index coverage (#397) ─────────────────────────────────────────────── +// +// `create_pool` maintains `DataKey::AssetPools(asset) -> Vec` (and its +// constant-time companion `DataKey::AssetPoolCount`) so `get_pools_by_asset` +// reads the index instead of walking the registry. These tests pin that the +// index is actually written and actually read — the issue's ask, since the +// index existed but nothing asserted it. + +#[test] +fn test_create_pool_maintains_the_asset_index() { + let t = setup(); + let asset = Address::generate(&t.env); + let other_asset = Address::generate(&t.env); + + let first = t.client.create_pool(&asset, &1_728_000u128, &2u32, &10u64, &0i128); + let second = t.client.create_pool(&asset, &1_728_000u128, &2u32, &10u64, &0i128); + let other = t.client.create_pool(&other_asset, &1_728_000u128, &2u32, &10u64, &0i128); + + // The index returns only the pools for the requested asset... + let page = t.client.get_pools_by_asset(&asset, &0u32, &10u32); + let ids: std::vec::Vec = page.records.iter().map(|(id, _)| id).collect(); + assert_eq!(ids, std::vec![first, second]); + assert_eq!( + page.total, 3, + "total reports the whole registry, not just the asset's pools" + ); + + // ...and the other asset's lookup is not polluted by them. + let other_page = t.client.get_pools_by_asset(&other_asset, &0u32, &10u32); + let other_ids: std::vec::Vec = other_page.records.iter().map(|(id, _)| id).collect(); + assert_eq!(other_ids, std::vec![other]); +} + +#[test] +fn test_asset_pool_count_agrees_with_the_index() { + let t = setup(); + let asset = Address::generate(&t.env); + + assert_eq!(t.client.pool_count_by_asset(&asset), 0); + + t.client.create_pool(&asset, &1_728_000u128, &2u32, &10u64, &0i128); + t.client.create_pool(&asset, &1_728_000u128, &2u32, &10u64, &0i128); + + assert_eq!(t.client.pool_count_by_asset(&asset), 2); + assert_eq!( + t.client.get_pools_by_asset(&asset, &0u32, &10u32).records.len(), + 2 + ); +} + +#[test] +fn test_create_pools_batch_maintains_the_asset_index() { + let t = setup(); + let asset = Address::generate(&t.env); + let other_asset = Address::generate(&t.env); + + let mut batch = vec![&t.env]; + for _ in 0..2 { + batch.push_back(PoolParams { + asset: asset.clone(), + daily_rate: 1_728_000u128, + global_multiplier: 2u32, + min_lock_period: 10u64, + min_stake_amount: 0i128, + }); + } + batch.push_back(PoolParams { + asset: other_asset.clone(), + daily_rate: 1_728_000u128, + global_multiplier: 2u32, + min_lock_period: 10u64, + min_stake_amount: 0i128, + }); + + let created = t.client.create_pools_batch(&batch); + let ids: std::vec::Vec = created.iter().collect(); + assert_eq!(ids.len(), 3); + + // Batched creation must index exactly like single creation. + let page = t.client.get_pools_by_asset(&asset, &0u32, &10u32); + let indexed: std::vec::Vec = page.records.iter().map(|(id, _)| id).collect(); + assert_eq!(indexed, std::vec![ids[0], ids[1]]); + assert_eq!(t.client.pool_count_by_asset(&asset), 2); + assert_eq!(t.client.pool_count_by_asset(&other_asset), 1); +} + +#[test] +fn test_asset_index_survives_a_paginated_walk() { + let t = setup(); + let asset = Address::generate(&t.env); + for _ in 0..3 { + t.client.create_pool(&asset, &1_728_000u128, &2u32, &10u64, &0i128); + } + t.client.create_pool(&Address::generate(&t.env), &1_728_000u128, &2u32, &10u64, &0i128); + + // Resuming from the previous page's `next_start_id` must not drop or + // duplicate indexed pools (the #327 resume invariant, index path). + let mut collected: std::vec::Vec = std::vec::Vec::new(); + let mut start = 0u32; + loop { + let page = t.client.get_pools_by_asset(&asset, &start, &1u32); + for (id, _) in page.records.iter() { + collected.push(id); + } + if page.next_start_id >= page.total || page.records.is_empty() { + break; + } + start = page.next_start_id; + } + + assert_eq!( + collected.len(), + 3, + "every indexed pool for the asset should be returned exactly once" + ); +} diff --git a/soroban/contracts/farming-pool/src/lib.rs b/soroban/contracts/farming-pool/src/lib.rs index 4e6b71e..c6d80f7 100644 --- a/soroban/contracts/farming-pool/src/lib.rs +++ b/soroban/contracts/farming-pool/src/lib.rs @@ -1201,6 +1201,33 @@ impl FarmingPool { /// Lock assets for the minimum lock period. A top-up checkpoints the /// existing position and extends its whole-position unlock ledger to the /// later of the existing unlock ledger and a fresh period from this call. + /// + /// # Reentrancy posture (#398) + /// + /// `stake_token` is admin-supplied and therefore not necessarily a trusted + /// Stellar Asset Contract — a non-standard `transfer` could call back into + /// this contract while it is still executing. Two independent defenses + /// apply, in order: + /// + /// 1. **Checks-effects-interactions.** Every validation runs first, the + /// position (including the extended unlock ledger) is written to + /// storage, and only then is `token::transfer` called. A reentrant call + /// therefore observes the already-updated position rather than a + /// half-applied one, so it cannot withdraw or re-credit more than the + /// post-deposit position allows. `token::Client::transfer` returns + /// `()` on success and traps on failure, so a failed transfer reverts the + /// whole invocation and leaves no partial deposit (see #363). + /// 2. **Host-level reentry prohibition.** Soroban's `ContractReentryMode` + /// defaults to `Prohibited`, so a token that tries to reenter this + /// contract during the transfer traps with "Contract re-entry is not + /// allowed" before any of our code runs. + /// + /// Both properties are covered by the reentrancy tests in `test.rs` + /// (`test_lock_assets_reentrant_transfer_is_rejected_and_final_state_is_correct` + /// and `test_lock_assets_reverts_entirely_if_stake_token_naively_reenters`), + /// which use `MockReentrantToken` / `MockNaiveReentrantToken` to attempt + /// the reentry mid-transfer. No code change is required for the reported + /// concern; this comment records the verification. pub fn lock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> { user.require_auth(); require_initialized(&env)?; diff --git a/soroban/contracts/vesting-wallet/src/lib.rs b/soroban/contracts/vesting-wallet/src/lib.rs index 88087bb..87949e6 100644 --- a/soroban/contracts/vesting-wallet/src/lib.rs +++ b/soroban/contracts/vesting-wallet/src/lib.rs @@ -171,6 +171,11 @@ impl VestingWallet { if env.storage().instance().has(&DataKey::Beneficiary) { return Err(VestingError::AlreadyInitialized); } + // #405 — a zero beneficiary can never call `release` (it cannot sign), + // so the entire vested amount would be stranded with no recovery path. + if beneficiary == Address::default() { + return Err(VestingError::InvalidInput); + } assert!(total_amount > 0, "total_amount must be positive"); assert!( start_ledger >= env.ledger().sequence(), @@ -428,8 +433,15 @@ impl VestingWallet { } /// Transfer beneficiary rights to `new_beneficiary`. Admin must authorise. + /// + /// #405 — the same zero-address check as `initialize` applies here: the + /// admin cannot hand the schedule to an address that can never call + /// `release`, which would strand the remaining vested amount. pub fn transfer_beneficiary(env: Env, new_beneficiary: Address) -> Result<(), VestingError> { require_initialized(&env)?; + if new_beneficiary == Address::default() { + return Err(VestingError::InvalidInput); + } let admin = get_admin(&env); admin.require_auth(); bump_instance(&env); diff --git a/soroban/contracts/vesting-wallet/src/test.rs b/soroban/contracts/vesting-wallet/src/test.rs index 99108d8..bec52f9 100644 --- a/soroban/contracts/vesting-wallet/src/test.rs +++ b/soroban/contracts/vesting-wallet/src/test.rs @@ -646,3 +646,109 @@ fn test_release_requires_beneficiary_auth() { let released = t.client.release(); assert!(released > 0); } + +// ── admin access-control regression tests (#406) ────────────────────────────── +// +// The reported `clawback` entry point does not exist in this contract, but +// the underlying concern — "no access control on sensitive operations" — is +// worth pinning down. These tests authorize only a NON-admin address and +// assert that every admin-gated entry point rejects the call, so any future +// entry point added without `require_auth()` fails here. + +#[test] +fn test_admin_only_entry_points_require_admin_auth() { + let t = setup_revocable(0, 100, 1_000); + advance_ledgers(&t.env, 50); + + let stranger = Address::generate(&t.env); + + // Authorize the stranger for a harmless read only: no admin-gated entry + // point has a matching authorization, so `require_auth()` must fail. + t.env.mock_auths(&[]); + + assert!( + t.client.try_revoke().is_err(), + "revoke must require the stored admin's authorization" + ); + assert!( + t.client.try_emergency_withdraw().is_err(), + "emergency_withdraw must require the stored admin's authorization" + ); + assert!( + t.client.try_transfer_beneficiary(&stranger).is_err(), + "transfer_beneficiary must require the stored admin's authorization" + ); + assert!( + t.client.try_transfer_admin(&stranger).is_err(), + "transfer_admin must require the current admin's authorization" + ); + + // Nothing moved: the rejected calls must not have changed any state. + assert_eq!(t.client.beneficiary(), t.beneficiary); + assert_eq!(t.client.admin(), t.admin); + assert!(!t.client.revoked()); + assert_eq!(t.client.released_amount(), 0); + assert_eq!( + t.token.balance(&t.contract_id), + 1_000, + "vested tokens must still be held after rejected admin calls" + ); + + // The admin's own authorization still works, proving the rejections above + // were about authorization and not a broken fixture. + t.env.mock_all_auths(); + t.client.revoke(); + assert!(t.client.revoked()); +} + +// ── beneficiary validation tests (#405) ─────────────────────────────────────── + +#[test] +fn test_initialize_rejects_the_zero_beneficiary() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let token_admin = Address::generate(&env); + let asset = env.register_stellar_asset_contract_v2(token_admin.clone()); + let token_sac = StellarAssetClient::new(&env, &asset.address()); + token_sac.mint(&admin, &1_000i128); + + let contract_id = env.register(VestingWallet, ()); + let client = VestingWalletClient::new(&env, &contract_id); + + let start = env.ledger().sequence(); + let result = client.try_initialize( + &Address::default(), // #405: unusable beneficiary + &asset.address(), + &1_000i128, + &start, + &start, + &start + 200, + &false, + &admin, + ); + + assert!( + matches!(result, Err(Ok(VestingError::InvalidInput))), + "initialize must reject the zero beneficiary address" + ); +} + +#[test] +fn test_transfer_beneficiary_rejects_the_zero_address() { + let t = setup(0, 200, 1_000); + let good = Address::generate(&t.env); + + let rejected = t + .client + .try_transfer_beneficiary(&Address::default()); + assert!( + matches!(rejected, Err(Ok(VestingError::InvalidInput))), + "transfer_beneficiary must reject the zero address, which could strand the funds" + ); + + // A real address still works, and the rejected call changed nothing. + t.client.transfer_beneficiary(&good); + assert_eq!(t.client.beneficiary(), good); +} diff --git a/soroban/contracts/vesting-wallet/src/types.rs b/soroban/contracts/vesting-wallet/src/types.rs index e6863f9..4d6375c 100644 --- a/soroban/contracts/vesting-wallet/src/types.rs +++ b/soroban/contracts/vesting-wallet/src/types.rs @@ -11,6 +11,9 @@ pub enum VestingError { Unauthorized = 5, TotalAmountTooLarge = 6, ArithmeticOverflow = 7, + /// A supplied address is unusable (e.g. the zero address as beneficiary), + /// which would strand released funds permanently (#405). + InvalidInput = 8, } /// Storage keys for all instance data in the vesting wallet.