fix: beneficiary validation, clawback access-control audit, lock_assets reentrancy docs, asset index tests (#406, #405, #398, #397)

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)

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<u32>`
+    /// (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(

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<u32>` (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<u32> = 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<u32> = 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<u32> = 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<u32> = 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<u32> = 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"
+    );
+}

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)?;

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);