CHA-620: Build Regular contract for Stake NODL tokens

[Douglasacost]

No description provided.

[linear]

CHA-620

[github-actions]

LCOV of commit 6420108 during checks #742

Summary coverage rate:
  lines......: 27.1% (939 of 3463 lines)
  functions..: 27.5% (150 of 546 functions)
  branches...: 28.1% (164 of 583 branches)

Files changed coverage rate:
                                                    |Lines       |Functions  |Branches    
  Filename                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================
  src/Staking.sol                                   | 0.0%    101| 0.0%    13| 0.0%     25

[aliXsed]

Reviewed after merging latest main into the branch (42826db): resolved .cspell.json, accepted the deploy/ → hardhat-deploy/ rename for the new script, bumped the pragma to ^0.8.26 to match main's pinned solc, and converted the 20 testFail* tests to vm.expectRevert (removed in main's newer forge). Two of those tests were passing spuriously under testFail semantics — testFailClaimInsufficientRewards (the claim actually succeeded; the failed inner expectRevert was what made it "pass") and testFailClaimBeforeDuration (_skipTime(DURATION - 1) warps ~7,100 years, not 30 days − 1s) — both now test their intended scenario.

Summary

The PR adds a fixed-rate NODL staking contract (min/max stakes, pool cap, fixed duration, admin-gated unstake, emergency withdraw), a 1,000-line forge test suite, a hardhat deploy script, and README docs. Findings below, most severe first — the top two are funds-safety issues.

#	Where	Issue
1	src/Staking.sol:121	Rewards are checked but never reserved at stake time — the pool oversubscribes and late claimers' principal is locked behind InsufficientRewardBalance
2	src/Staking.sol:195	emergencyWithdraw() sweeps user principal, leaves accounting pointing at phantom funds, callable while live, no recovery path
3	src/Staking.sol:90	Natspec says _duration is days; code/README/deploy treat it as seconds — an operator following the natspec deploys an 86,400× shorter lock
4	src/Staking.sol:136	Raw transfer/transferFrom on an env-configured token address; repo convention (Grants, Payment, EnvelopeLinks) is SafeERC20
5	src/Staking.sol:171	Unreachable AlreadyUnstaked, unused errors, redundant allowance pre-check, <= 0 on uints, no-op PRECISION factor
6	src/Staking.sol:261	getStakeInfo copies the whole stake array per call; no stakes.length getter for integrators
7	README.md:275	Stale duplicate deploy section with nonexistent path/network; DEPLOYER_PRIVATE_KEY undocumented
8	hardhat-deploy/deploy_staking.dp.ts:11	Dead re-verify toggle; no env validation (missing vars fail as NaN deep in ABI encoding)

Suggestions / design questions (not fixing, need author's call)

• Trust model disclosure: unstakeAllowed defaults to false, claim/unstake are whenNotPaused, and one admin holds all three roles — so a single key can freeze exits indefinitely and (with emergencyWithdraw) sweep the pool. If that's the intended custody model, the README should say so; a timelock or role separation would materially de-risk it. Note updateUnstakeAllowed is itself whenNotPaused, so the escape hatch can't even be opened while paused.
• Emergency semantics: consider sweeping only unreserved rewards, or pairing the sweep with a user refund path — right now recovery from a benign emergency still bricks every stake.
• Reward economics: the reward is a flat REWARD_RATE% per stake regardless of duration — with the README example (DURATION=3600, REWARD_RATE=12) that's 12% per hour. If the intent is an APR, the formula needs a time term.
• Upgradeability: repo convention for new user-facing contracts is UUPS (src/swarms/*Upgradeable.sol); this one is immutable. Deliberate?
• MAX_POOL_STAKE: the headline cap is a hardcoded 5M default (not a constructor param, absent from the deploy interface) yet admin-mutable, while lower-stakes params are immutable — and SCREAMING_CASE conventionally signals immutability it doesn't have.
• Foundry-first: the repo rules prefer forge script deployment; a script/DeployStaking.s.sol would match the sibling contracts (the hardhat script does follow the existing hardhat-deploy/ pattern, so this is a soft point).

I'll push fixes for findings 1-8 to this branch shortly (for #2, the minimal whenPaused gate — the redesign is yours to decide).

[aliXsed]

Rewards over-subscription — later claims revert and lock principal. This check compares against availableRewards but never reserves the reward: availableRewards is only decremented at claim() time. So N stakes can all pass the check against the same unreserved pot.

Concrete failure: fund 100 NODL of rewards at 10%; user A stakes 1000 NODL (needs 100) and user B stakes 1000 NODL (needs 100) — both pass since availableRewards is still 100 at each stake. A claims first and drains the pot to 0; B's claim() then reverts with InsufficientRewardBalance forever, and because claim is all-or-nothing B can't even recover principal unless the admin flips unstakeAllowed. This directly breaks the README's "Guaranteed yield" property.

Fix: reserve at stake time — availableRewards -= reward here (the check already shows this was the intent), drop the re-check/decrement in claim() (the reward is guaranteed by construction), and credit the forfeited reward back in unstake().

[aliXsed]

emergencyWithdraw() sweeps user principal and permanently bricks accounting. It transfers the contract's entire balance (staked principal included, up to MAX_POOL_STAKE = 5M NODL) to msg.sender, zeroes availableRewards, but leaves stakes[], totalStakedInPool, and totalStakedByUser untouched. Afterward every claim()/unstake() reverts on transfer, getStakeInfo still shows active stakes, and there is no repair path — fundRewards() is the only inflow and it credits everything to availableRewards, so re-funding principal corrupts the reward accounting.

It's also callable while the pool is live (no pause required), so a compromised EMERGENCY_MANAGER_ROLE key can pause() (blocking user exits) then sweep everything in two transactions — and note the same admin holds all three roles per the constructor.

Minimal hardening I'll apply: gate it whenPaused so a sweep requires an explicit prior pause. The deeper design questions — should it sweep only rewards rather than principal? should there be a recovery/refund path? — are the author's call; the current shape needs at least documenting in the README's trust assumptions (admin can pause exits + withdraw all funds; unstakeAllowed defaults to false so pre-maturity exit is admin-gated).

[aliXsed]

Duration units contradiction. The constructor natspec says _duration is "represented in days", but _duration * 1 seconds is a no-op multiplier — the value is used as raw seconds. The README documents DURATION: Duration of the staking period in seconds (example DURATION=3600) and the deploy script passes the env value through unconverted, so the code path is seconds — but an operator following the natspec and passing 90 for a 90-day pool would deploy an immutable 90-second lock, letting everyone claim the full fixed reward almost instantly and draining the pool at ~86,400× the intended rate.

Since README + deploy script + tests all treat it as seconds, the fix is to correct the natspec (and the deploy script should stop pretending otherwise). Worth double-checking the intended mainnet parameterization before deploy regardless.

[aliXsed]

Raw transfer/transferFrom without SafeERC20, on an arbitrary token address. The token comes from the STAKE_TOKEN env var and is only cast (token = NODL(nodlToken)) — nothing guarantees it's the OZ-v5 NODL that reverts on failure. If it's ever pointed at a false-returning ERC20, this transferFrom returns false without reverting and stake() still records the stake — the user gets credited principal + future reward for tokens never received. Same unchecked pattern at lines 156, 186, 199, 234.

Every other funds-handling contract in this repo (Grants.sol, Payment.sol, EnvelopeLinks.sol) uses using SafeERC20 for IERC20 with safeTransfer/safeTransferFrom — this contract should too. Relatedly, only the IERC20 surface is used, so the field should be IERC20 public immutable token (dropping the NODL import/coupling), which is also what SafeERC20 needs.

[aliXsed]

Unreachable check + dead-code cluster. unstake() zeroes s.amount before setting s.unstaked, so this function's s.amount == 0 check always fires first — AlreadyUnstaked can never be emitted, and a user who unstaked and retries claim gets the misleading NoStakeFound. Reordering the checks (claimed/unstaked before amount == 0) makes the accurate error reachable; after the reorder, the amount == 0 case is impossible (stakes are always created with amount ≥ MIN_STAKE and only zeroed together with a flag), so NoStakeFound can go entirely.

Related dead weight in the same family:

• AlreadyStaked and InsufficientTotalStaked errors are declared but never used anywhere (and the stake() natspec "can only be staked if the user has not already staked" describes a rule the code doesn't have — multiple stakes are allowed by design).
• fundRewards' manual allowance pre-check duplicates what OZ v5 transferFrom enforces atomically (ERC20InsufficientAllowance) — TOCTOU-illusory and costs an extra external call; InsufficientAllowance goes with it.
• _rewardRate <= 0 (line 80) and newMaxPoolStake <= 0 (line 309) on uint256 are just == 0.
• PRECISION in _calculateReward multiplies by 1e18 and divides it right back out — the formula is exactly amount * REWARD_RATE / 100; the "high precision" comment describes handling that doesn't exist, and the extra factor only shrinks overflow headroom.

[aliXsed]

getStakeInfo copies the user's entire stake array to memory to read one element. StakeInfo[] memory userStakes = stakes[user] costs O(number of stakes) storage reads per call — a user with hundreds of stakes makes any on-chain caller (or gas-capped eth_call) pay for the full copy, and a frontend enumerating stakes per index pays O(N²) total. Index storage directly instead.

Also missing: a view returning stakes[user].length. The auto-generated stakes(address,uint256) getter reverts past the end, so integrators must probe indices until a revert just to count a user's stakes — one stakesCount(address) view fixes that.

[aliXsed]

Stale duplicate deploy section. This second "## Deploying Staking contract" heading contradicts the full section at line 154: the path deploy/deploy_staking.dp.ts doesn't exist (the script lives in hardhat-deploy/), no network named zkSync is configured in hardhat.config.ts, and hardhat run doesn't invoke deploy-zksync-style scripts at all — an operator following this gets errors or a silent no-op. Should be deleted in favor of the line-154 section.

That section, in turn, omits DEPLOYER_PRIVATE_KEY from the documented env vars even though the script requires it (new Wallet(process.env.DEPLOYER_PRIVATE_KEY!, ...)) — a verbatim copy of the documented command throws a cryptic ethers "invalid private key" with no hint which var is missing.

[aliXsed]

Dead deploy toggle + zero env validation. CONTRACT_ADDRESS = "" / SHOULD_DEPLOY = !CONTRACT_ADDRESS is an edit-the-source re-verify switch that's always true as committed — it invites someone to paste a testnet address into the source and ship it. Straight-line deploy + verify (the pattern in DeployEnvelope.ts) does the same job without the trap.

The seven process.env.X! reads are compile-time assertions only: a missing MIN_STAKE crashes as BigInt(undefined), a missing DURATION/REWARD_RATE becomes Number(undefined) = NaN and only blows up deep inside ethers ABI encoding during fee estimation — nothing names the missing variable. A small required-env check at the top (throw listing missing names) turns these into actionable errors. Also note REWARD_RATE=12.5 passes Number() but is rejected by the ABI encoder — the rate is an integer percent, worth documenting.

[aliXsed]

Pushed fixes for the review findings in 6420108. All checks green: Tests ✓ Coverage ✓ Specification-PDF ✓ (verified locally too — 1249 forge tests, spellcheck, lint).

Funds-safety

• Rewards are now reserved from availableRewards at stake time (and refunded on unstake), so the pool can't be oversubscribed and a matured stake is always claimable. Added test_RevertWhen_StakeExhaustsReservedRewards and test_ClaimSucceedsAfterRewardsFullyReserved.
• emergencyWithdraw is now gated whenPaused (minimal hardening — the deeper redesign is your call, see below).
• Switched to SafeERC20 (safeTransfer/safeTransferFrom) and typed the token as IERC20.

Correctness / cleanup

• Duration units: dropped the no-op * 1 seconds, corrected the natspec to say seconds (matches README/deploy/tests).
• Reordered claim/unstake checks so AlreadyUnstaked is reachable; removed unused errors (AlreadyStaked, NoStakeFound, InsufficientAllowance, InsufficientTotalStaked) and the no-op PRECISION factor; == 0 on uints.
• getStakeInfo reads storage directly instead of copying the whole array; added stakesCount(address) for integrators.

Deploy / docs

• Deploy script validates required env vars (named errors) and rejects non-integer REWARD_RATE/DURATION; removed the dead re-verify toggle.
• Removed the stale duplicate README deploy section, documented DEPLOYER_PRIVATE_KEY, and added a "Trust assumptions" note.

Also merged latest main into the branch (42826db) — resolved .cspell.json, moved the deploy script to hardhat-deploy/ per main's rename, bumped the pragma to ^0.8.26, and converted the 20 testFail* tests to vm.expectRevert (two were previously passing spuriously).

Left for you to decide (design, not fixed): whether emergencyWithdraw should sweep only rewards / pair with a user-refund path; whether the reward should be time-weighted (APR) rather than a flat per-stake percent; whether this should be UUPS like the repo's other user-facing contracts; and role separation / timelock for the admin. Happy to follow up on any of these.