fix(factory): use checked_add for PoolCount, add PoolCountOverflow er… #82

soroban/contracts/factory/Cargo.toml

@@ -12,4 +12,5 @@ doctest    = false
 soroban-sdk = { workspace = true }
 
 [dev-dependencies]
-soroban-sdk = { workspace = true, features = ["testutils"] }
+soroban-sdk    = { workspace = true, features = ["testutils"] }
+farming-pool   = { path = "../farming-pool" }

soroban/contracts/factory/src/lib.rs

@@ -286,6 +286,10 @@ impl Factory {
         env.events().publish(
             (symbol_short!("factory"), symbol_short!("pool_upg")),
             (pool_id, record.address, new_wasm_hash),
+        );
+        Ok(())
+    }
+
     /// Update the WASM hash used for future `create_pool` deployments. Admin-only.
     ///
     /// Allows the admin to point future pool deployments at a corrected or upgraded
@@ -358,6 +362,12 @@ impl Factory {
             .map_err(|_| FactoryError::MinLockPeriodOutOfRange)?;
 
         let pool_id: u32 = env.storage().instance().get(&DataKey::PoolCount).unwrap();
+        // Compute next count with checked arithmetic before any deployment
+        // side-effect so overflow is caught as a typed error rather than
+        // relying on the workspace Cargo profile's overflow-checks flag.
+        let next_count = pool_id
+            .checked_add(1)
+            .ok_or(FactoryError::PoolCountOverflow)?;
         let wasm_hash: BytesN<32> = env.storage().instance().get(&DataKey::WasmHash).unwrap();
         let salt = pool_salt(&env, pool_id);
 
@@ -397,7 +407,7 @@ impl Factory {
         bump_pool(&env, pool_id);
         env.storage()
             .instance()
-            .set(&DataKey::PoolCount, &(pool_id + 1));
+            .set(&DataKey::PoolCount, &next_count);
 
         // Emit enriched event so indexers get the full pool parameters in one shot.
         #[allow(deprecated)]

soroban/contracts/factory/src/test.rs

@@ -518,15 +518,15 @@ fn test_get_pools_by_asset_paginates_large_matching_registry() {
     let env = Env::default();
     env.mock_all_auths();
     let admin = Address::generate(&env);
-    let wasm_hash = upload_mock_pool_wasm(&env);
+    let wasm_hash = upload_farming_pool_wasm(&env);
     let factory_addr = env.register(Factory, ());
     let client = FactoryClient::new(&env, &factory_addr);
     client.initialize(&admin, &wasm_hash);
 
     // Create 25 pools all sharing the same asset
     let asset = Address::generate(&env);
     for i in 0..25 {
-        client.create_pool(&asset, &(100 + i as u128), &(10 + i as u64));
+        client.create_pool(&asset, &(1_728_000 + i as u128), &2u32, &(10 + i as u64));
     }
 
     // First page should return 20 records
@@ -716,7 +716,7 @@ fn test_refresh_pool_ttls_restores_ttl_for_unqueried_pool() {
     let t = setup();
     let id = t
         .client
-        .create_pool(&Address::generate(&t.env), &250u128, &50u64);
+        .create_pool(&Address::generate(&t.env), &1_728_000u128, &2u32, &50u64);
 
     // Initial TTL after creation
     assert_eq!(pool_record_ttl(&t.env, &t.factory_addr, id), TTL_EXTEND_TO);
@@ -855,6 +855,48 @@ fn test_create_pool_admin_matches_factory_admin_at_creation_time() {
     assert_ne!(pool_client.admin(), new_admin);
 }
 
+// ── PoolCount overflow (issue #82) ───────────────────────────────────────────
+
+/// Seed PoolCount to u32::MAX directly via `env.as_contract` — the same
+/// pattern used by `setup_with_pool_records` — then assert that the very next
+/// `create_pool` call returns `PoolCountOverflow` rather than wrapping to 0
+/// and silently clobbering pool 0's registry record.
+///
+/// This also verifies that the overflow check fires *before* any deployment
+/// side effect: because `checked_add` is evaluated ahead of `deploy_v2`,
+/// no pool contract is deployed and PoolCount remains at `u32::MAX`.
+#[test]
+fn test_create_pool_at_max_pool_count_returns_overflow_error() {
+    let t = setup();
+
+    // Seed the counter to the maximum representable value.
+    t.env.as_contract(&t.factory_addr, || {
+        t.env
+            .storage()
+            .instance()
+            .set(&DataKey::PoolCount, &u32::MAX);
+    });
+
+    let asset = Address::generate(&t.env);
+    let result = t
+        .client
+        .try_create_pool(&asset, &1_728_000u128, &1u32, &10u64);
+
+    // Must return the typed overflow error, not wrap to 0.
+    assert_eq!(result, Err(Ok(FactoryError::PoolCountOverflow)));
+
+    // PoolCount must remain unchanged — no partial write should have occurred.
+    t.env.as_contract(&t.factory_addr, || {
+        let count: u32 = t
+            .env
+            .storage()
+            .instance()
+            .get(&DataKey::PoolCount)
+            .unwrap();
+        assert_eq!(count, u32::MAX, "PoolCount must not be modified on overflow");
+    });
+}
+
 // ── set_pool_wasm_hash ────────────────────────────────────────────────────────
 
 #[test]

soroban/contracts/factory/src/types.rs

@@ -75,4 +75,12 @@ pub enum FactoryError {
     MinLockPeriodOutOfRange = 6,
     /// A function requiring initialization was called on an uninitialized factory.
     NotInitialized = 7,
+    /// `create_pool`'s PoolCount increment would overflow `u32::MAX`.
+    ///
+    /// Returned by `create_pool` when the running pool counter is already at
+    /// `u32::MAX` and a further increment would wrap around to 0, which would
+    /// silently clobber pool 0's registry record.  Provides an explicit, typed
+    /// failure instead of relying solely on the workspace Cargo profile's
+    /// `overflow-checks` flag.
+    PoolCountOverflow = 8,
 }