farming-pool: min_lock_period has no upper-bound ceiling anywhere (unlike global_multiplier/credit_rate post-#89), letting an admin set an effectively permanent lock

[prodbycorne]

Overview

#89 established a clear precedent for this codebase: numeric admin-configurable parameters that can cause severe user-facing harm at extreme values need an explicit, documented upper ceiling, not just a lower-bound sanity check. global_multiplier and credit_rate both received MAX_GLOBAL_MULTIPLIER/MAX_CREDIT_RATE ceilings as a result. min_lock_period — a parameter with an even more direct fund-safety consequence, since it gates whether a user can ever get their principal back at all — never received the equivalent treatment, in either initialize or set_min_lock_period:

// soroban/contracts/farming-pool/src/lib.rs:817-831
pub fn set_min_lock_period(env: Env, new_period: u32) -> Result<(), PoolError> {
    require_initialized(&env)?;
    get_admin(&env)?.require_auth();
    bump_instance(&env);
    let old_period = read_min_lock_period(&env);
    env.storage().instance().set(&DataKey::MinLockPeriod, &new_period);   // no bound on new_period at all
    ...
}

// initialize, lib.rs:298-339 — min_lock_period (u32) stored directly at line 328-330, no validation

min_lock_period accepts the full u32 range unconditionally — up to 4_294_967_295 ledgers, or roughly 681 years at Stellar's ~5-second-per-ledger target. Since min_lock_period changes only apply to new positions going forward (existing positions keep their originally-computed unlock_ledger, per #17's original fix and the explicit test_min_lock_period_change_does_not_affect_existing_position_unlock_ledger test), this doesn't retroactively trap already-locked users — but any user who locks after an admin sets an extreme min_lock_period (whether through a fat-finger error, exactly the #89 threat model of an admin mistake, or a deliberate rug-pull-style action) has their principal locked for a period that is, for any practical purpose, permanent. unlock_assets's only gate is a plain current >= position.unlock_ledger assertion (lib.rs:466-470) — there is no admin override, no separate unlock mechanism tied to min_lock_period specifically. The only way out for an affected user is the same admin-gated, pause-requiring emergency_withdraw escape hatch every other fund-safety issue in this batch converges on — meaning a legitimate depositor's only recovery path from an extreme min_lock_period is trusting the same admin who could have set it in the first place to notice, pause the pool, and manually rescue them.

Requirements

• Add a MAX_MIN_LOCK_PERIOD ceiling (documented with the same worked-derivation approach #89 established — e.g. bounded to a generous but clearly-finite multi-year window, not an open-ended u32 range) enforced in both initialize and set_min_lock_period.
• Mirror the same ceiling in factory::create_pool's min_lock_period: u64 parameter (which today only validates that the value fits in u32, per MinLockPeriodOutOfRange — not that it's within any reasonable range), consistent with the companion issue in this batch about create_pool not mirroring farming-pool's other post-farming-pool: set_global_multiplier and set_credit_rate accept unbounded values with no sanity ceiling, guaranteeing compute_credits overflow at scale #89 ceilings.

Acceptance Criteria

• set_min_lock_period and initialize both reject a min_lock_period above the new ceiling with a typed PoolError.
• factory::create_pool rejects a min_lock_period above the same ceiling with a typed FactoryError, before deploying/initializing a pool with it.
• New tests test_set_min_lock_period_rejects_above_ceiling, test_initialize_rejects_min_lock_period_above_ceiling, and a factory-side equivalent.
• The chosen ceiling and its rationale are documented with the same rigor as #89's derivation comment.

Additional Notes

More precise references

• soroban/contracts/farming-pool/src/lib.rs:817-831 (set_min_lock_period) — confirmed zero validation on new_period beyond it being a valid u32.
• lib.rs:298-339 (initialize), lines 328-330 — confirmed min_lock_period is stored directly with no bound, unlike global_multiplier/credit_rate immediately above it in the same function, which both gained explicit ceilings per #89.
• lib.rs:455-495 (unlock_assets), lines 466-470 — confirmed the only lock-period gate is a plain assertion with no admin bypass specific to an extreme configured period (the only override is the pool-wide, pause-requiring emergency_withdraw).
• soroban/contracts/factory/src/lib.rs:417-424 and :434-436 (create_pool's min_lock_period: u64 parameter, converted via min_lock_period.try_into().map_err(|_| FactoryError::MinLockPeriodOutOfRange)?) — confirmed this only validates the value fits in u32, not that it's within any reasonable operational range.
• #17 (closed, originally made min_lock_period mutable at all) and farming-pool/src/test.rs:1046-1071 (test_min_lock_period_change_does_not_affect_existing_position_unlock_ledger, test_new_positions_use_updated_min_lock_period) — confirmed the existing, correct behavior that changes only apply prospectively, which is what confines this issue's impact to newly locked positions rather than retroactively trapping existing ones.

Additional edge cases

• Since min_lock_period changes are prospective-only, the practical exploit window is narrower than #89's (which could retroactively affect every existing uncheckpointed staker in one call) — this issue's severity is best framed as "any user who locks between the extreme min_lock_period being set and it being corrected is effectively trapped," not "every existing depositor is immediately affected." Worth being precise about this distinction relative to #89's framing when prioritizing.
• A reasonable ceiling should still be generous enough for legitimate long-duration campaigns (e.g. multi-year vesting-style lock campaigns are a plausible real use case for this contract, given the existence of a separate dedicated vesting-wallet contract in this same workspace for the pure vesting use case suggests farming-pool's lock system is meant for shorter, campaign-style commitment periods) — the derivation should pick a ceiling that's clearly "no reasonable business case needs more than this" (mirroring #89's own framing for its constants) rather than an arbitrary round number.

Implementation sketch

/// No reasonable campaign needs a lock longer than this; see #89 for the
/// sibling-parameter derivation style this mirrors.
const MAX_MIN_LOCK_PERIOD: u32 = 12_614_400; // ~2 years at 5s/ledger

pub fn set_min_lock_period(env: Env, new_period: u32) -> Result<(), PoolError> {
    require_initialized(&env)?;
    get_admin(&env)?.require_auth();
    if new_period > MAX_MIN_LOCK_PERIOD {
        return Err(PoolError::MinLockPeriodAboveCeiling);
    }
    ...
}

Test/reproduction plan

• test_set_min_lock_period_rejects_above_ceiling / test_set_min_lock_period_accepts_exactly_the_ceiling (boundary-inclusive, mirroring test_set_credit_rate_accepts_exactly_the_ceiling's style).
• test_initialize_rejects_min_lock_period_above_ceiling.
• test_create_pool_rejects_min_lock_period_above_ceiling (factory side), extending test_create_pool_rejects_min_lock_period_out_of_u32_range's existing coverage to the new, tighter ceiling rather than only the raw u32-fit check.

Cross-references

• farming-pool: set_global_multiplier and set_credit_rate accept unbounded values with no sanity ceiling, guaranteeing compute_credits overflow at scale #89 (closed) — the precedent worked-derivation approach and threat model (admin fat-finger or malice) this issue extends to an overlooked parameter.
• Companion issue in this batch ("factory: create_pool doesn't mirror MAX_GLOBAL_MULTIPLIER/MAX_CREDIT_RATE ceilings") — the same "factory doesn't mirror pool-side ceilings" pattern, extended here to a third parameter.

[devkryssie]

I’m interested in this issue because it directly addresses user fund safety and secure contract configuration. I have experience with TypeScript, JavaScript, Python, Web3, and blockchain development, and I’m comfortable working with smart-contract logic and tests. I can implement the ceiling consistently across the farming-pool and factory contracts, add the required validation and regression tests, and submit a complete PR within two days.

[grantfox-oss]

🦊 GrantFox — @devkryssie has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #132)
2. Your PR will be reviewed by the SmartDropLabs maintainers

Good luck! Track your progress on GrantFox.

[devkryssie]

please kindly merge my PR