factory: PoolCount increment via pool_id + 1 has no checked_add, risking wraparound and clobbering pool 0's record #82

Problem

create_pool increments PoolCount with a bare, unchecked addition:

pub fn create_pool(env: Env, asset: Address, daily_rate: u128, min_lock_period: u64) -> u32 {
    // ...
    let pool_id: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap();
    // ...
    env.storage()
        .instance()
        .set(&DataKey::PoolCount, &(pool_id + 1));   // <-- unchecked u32 addition
    // ...
    pool_id
}

There is no checked_add/saturating_add anywhere in the factory contract's PoolCount handling (confirmed: grep -n "checked_\|saturating_" factory/src/lib.rs only matches the unrelated list_pools pagination logic). If pool_id ever reaches u32::MAX (4,294,967,295), pool_id + 1 overflows. Rust's default behavior differs by build profile: in the workspace's [profile.release] (overflow-checks = true), this traps the transaction; in a cargo test/dev build (overflow-checks defaults on in debug too) it also panics — but if the contract is ever built or deployed under any profile where overflow-checks = false (e.g. a custom profile, or a future toolchain/config change that doesn't preserve today's explicit setting), the addition silently wraps to 0, and the very next create_pool call would then overwrite DataKey::Pool(0) — the registry's genesis pool record — with a brand-new, unrelated pool's data, permanently destroying the original pool's on-chain metadata (address, asset, daily_rate, min_lock_period) as far as the factory's registry is concerned.

While reaching 4.29 billion pools is impractical today, this is exactly the kind of latent unchecked-arithmetic defect that a security review should flag regardless of current practical reachability — the fix is cheap, and relying on a Cargo profile flag staying correctly configured forever as the only thing standing between "graceful trap" and "silent registry corruption" is fragile.

Impact

• Registry corruption at the overflow boundary if the contract is ever built without overflow-checks: pool ID 0's record is silently clobbered by whatever pool is created at the wraparound point, with no error, no event indicating the collision, and no way to recover the original pool 0's data from the factory alone.
• Defense-in-depth gap: the contract currently relies entirely on a workspace-level Cargo profile setting for overflow safety in this specific line, with no in-code enforcement.

Fix

let pool_id: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap();
let next_count = pool_id.checked_add(1).ok_or(FactoryError::PoolCountOverflow)?;
// ...
env.storage().instance().set(&DataKey::PoolCount, &next_count);

Add FactoryError::PoolCountOverflow (or reuse a generic overflow variant) so the failure is a typed, matchable error rather than depending on build-profile configuration alone. Also change create_pool's return type to Result<u32, FactoryError> if it isn't already (see companion issues touching this signature).

Acceptance Criteria

• PoolCount increment uses checked_add and returns a typed error on overflow instead of relying solely on the Cargo profile's overflow-checks setting.
• Add a test that seeds DataKey::PoolCount to u32::MAX directly via env.as_contract (mirroring the existing setup_with_pool_records pattern) and asserts create_pool returns the typed overflow error rather than wrapping to 0.
• Confirm no other unchecked arithmetic exists in factory/src/lib.rs outside the already-saturating_-protected list_pools pagination logic.

Additional Notes

Precise references: the unchecked increment is soroban/contracts/factory/src/lib.rs:199-201 (env.storage().instance().set(&DataKey::PoolCount, &(pool_id + 1));), reading pool_id from line 178 (env.storage().instance().get(&DataKey::PoolCount).unwrap()). Confirmed via grep -n "checked_\|saturating_" soroban/contracts/factory/src/lib.rs that the only existing bounded arithmetic in the file is list_pools's start_id.saturating_add(capped_limit) at line 109 — the issue body's claim is accurate.

Edge cases beyond the issue body

• Interaction with factory: the entire contract has no NotInitialized guard — admin(), pool_wasm_hash(), and create_pool() panic via unwrap() before initialize() #81's guard restructure: if factory: the entire contract has no NotInitialized guard — admin(), pool_wasm_hash(), and create_pool() panic via unwrap() before initialize() #81 lands first, create_pool's error-handling shape already changes (adding require_initialized(&env)? and converting load_admin's panic into a ?-propagated error) — this issue's checked_add/FactoryError::PoolCountOverflow fix should slot into that same ?-chain rather than being written independently against the current panicking version, to avoid a merge conflict rewriting the same lines twice.
• pool_id reuse after overflow is worse than the issue body implies in one respect: the issue focuses on DataKey::Pool(0) being clobbered, but note pool_salt (lib.rs:29-33) derives the deployed pool's address from pool_id too — if PoolCount wraps to 0 and a new create_pool call reuses pool_id = 0, pool_salt(0) reproduces the exact same salt, and deploy_v2 at a colliding salt against an already-occupied contract address would itself fail (Soroban's deployer rejects redeploying at an existing address) — meaning the actual failure mode at the wraparound boundary is more likely a hard deploy-time trap than a silently-succeeding overwrite, unless pool 0's original deployed contract was somehow removed/self-destructed in the interim (Soroban contracts have no self-destruct, so this sub-case is likely unreachable) — worth verifying this interaction explicitly in the test added for this issue rather than assuming the "silently clobbers pool 0's record" framing is the complete picture; the PoolRecord overwrite and the deploy_v2 collision are two separate failure surfaces at the same boundary and the test should check which one actually fires first.
• u32::MAX reachability context: while genuinely impractical to reach via legitimate create_pool calls, note this is directly testable without 4 billion calls — the acceptance criteria correctly suggests seeding storage directly via env.as_contract, which is the only practical way to exercise this path.

Test plan

• test_create_pool_at_max_pool_count_returns_overflow_error (per issue body's acceptance criteria): use env.as_contract(&factory_address, || env.storage().instance().set(&DataKey::PoolCount, &u32::MAX)) (mirroring setup_with_pool_records's direct-storage-seeding pattern, factory/src/test.rs:94) then call create_pool and assert the typed error, and separately assert whether the failure is the new PoolCountOverflow check firing first (correct) vs. some other panic — the checked_add should be evaluated before the deploy_v2 call in the fixed code so the overflow is caught before any deployment side effect occurs.
• Cross-references: factory: the entire contract has no NotInitialized guard — admin(), pool_wasm_hash(), and create_pool() panic via unwrap() before initialize() #81 (shared restructure of create_pool's error handling), factory: create_pool deploys the pool via deploy_v2(wasm_hash, ()) without ever calling initialize, leaving every deployed pool permanently unusable #78/factory: a newly deployed, uninitialized farming-pool contract has no admin — anyone can front-run initialize() to seize control #79/factory: create_pool has no parameters (nor code path) to configure a deployed pool's admin, global_multiplier, or credit_rate #80 (all touch the same function body — recommend reviewing this whole cluster of factory issues as one coordinated PR given the amount of line-level overlap in create_pool).