Security Review — Robinhood (chain 4663) CHOP Redeployment

Target: 19 on-chain contracts on Robinhood chain ID 4663, verified source pulled from Sourcify (https://sourcify.dev/server/v2/contract/4663/<address>) — creation + runtime match results per the client's own description, not cloned from a git repository. Addresses audited:

Client-stated source commit: 2b1287a46b45b1ba3430a6447ca87afcf36045e9. This could not be independently verified — the job description names a commit but no repository URL, so there is nothing to git ls-remote against. This report is pinned to the on-chain addresses above, which is the actual artifact audited; treat the commit hash as an unverified client claim.

Prior audit note: the job description states this redeployment "replaces the previous instance in the audit inventory" and that "the original addresses remain on-chain and were not upgraded." Per our engagement policy, this audit was run fresh, independent of any prior report on the earlier addresses — every finding below comes from this job's own phase 0 map, phase 1 breadth agents, and phase 2 depth agents.

Methodology: four-phase pipeline (x-ray mechanical pre-scan → context/protocol-map → 8-agent ethskills breadth pass → 12-agent pashov-v4 depth hunt, blind to breadth findings → reconciliation). Scope: 17 first-party contracts (excluding interfaces and the two unmodified Uniswap vendor libraries FullMath.sol/TickMath.sol), 2,887 nSLOC. No test files were present in the Sourcify-verified source bundle (Sourcify mirrors src, not test/), so test coverage could not be assessed from this artifact. Coverage note: 2 of the 12 depth-hunt attack agents (first-principles, numerical-gap) died mid-run on an output-token ceiling and were not respawned (per methodology: a dead agent's slot is never retried within one pass — the remaining agents' coverage stands in for it). 10 of 12 depth agents completed; their findings showed heavy convergence with the 8 breadth agents, so coverage loss from the 2 dead agents is assessed as low.

Severity counts: 1 High · 8 Medium · 11 Low · 10 Informational

Findings

[H-1] flashMint lets a caller recycle their own buy/sell tax as repayment, bypassing up to ~85% of the sell tax on every WeightedIndex-based log

Severity: High

File: contracts/DecentralizedIndex.sol:471-494 (flashMint), :163-187 (_update, the fee block), :233-240 (_processBurnFee)

Description. flashMint snapshots _balance = balanceOf(address(this)) before minting and, after the recipient's callback, requires balanceOf(address(this)) >= _balance + _amount + _fee (DecentralizedIndex.sol:480,484). That check protects the pre-existing fee batch, but nothing protects fee revenue that accrues during the callback itself. flashMint carries the lock modifier but not noSwapOrFee, so every lTKN transfer the recipient makes inside the callback still pays the buy/sell tax, and that tax is credited straight into address(this) (:175,178,182) — exactly where the FMA repayment check reads its balance from.

// DecentralizedIndex.sol:480-487
uint256 _balance = balanceOf(address(this));
_mint(_recipient, _amount);
IFlashLoanRecipient(_recipient).callback(_data);
require(balanceOf(address(this)) >= _balance + _amount + _fee, "FMA");
_totalSupply -= _fee;
_burn(address(this), _amount + _fee);

Exploit scenario (canonical fee shape: sell = 50 = 0.5%, burn = 1500 = 15%, so s' = s·(1-b) = 0.0425% of a sale is actually burned and the rest is recoverable):

1. Attacker holds A lTKN they want to sell. They call flashMint(self, M, data) for a modest M (fee f = M/1000).
2. Inside the callback, attacker transfers S = (A - f)/(1 - s') lTKN to V2_POOL. _update taxes the transfer at the sell rate, crediting tax = S·s to address(this); _processBurnFee burns only s·b of that tax, leaving s·(1-b)·S sitting in the log's own balance.
3. Attacker calls IUniswapV2Pair.swap directly to take the paired-token proceeds of the (1-s)·S lTKN that actually reached the pool.
4. Attacker's remaining A - S·(1-s') ≈ f lTKN plus the recycled tax revenue closes the FMA check; _burn(address(this), M+f) destroys it.

Net effect: the attacker spends their own A lTKN but delivers (1-s)/(1-s')·A ≈ 0.9950·A into the pool instead of the honest (1-s)·A = 0.995·A — recovering essentially all of the burned-fee gap and paying only the flat 0.1% flash-mint fee instead of the intended sell tax. At the canonical parameters the effective sell tax drops from 0.5% to ≈0.0754% — ~85% of the sell tax is bypassed, repeatable on every sale. This is pure fee-revenue theft from LP stakers (ERC20 accounting — _totalSupply vs. OZ-internal supply — stays fully consistent throughout; this is not a backing-drain bug).

Reachability. ChopWeightedIndex.sol:18-20 overrides flashMint to revert FlashMintDisabled(), so canonical chain-4663 CHOP logs are not exploitable today. But DecentralizedIndex.flashMint/WeightedIndex is a fully live, shipped, factory-deployable implementation (WeightedIndexFactory.setImplementationsAndBeacons is parameterized with weightedIndexImpl), and the only thing preventing exploitation on any such log is a four-line override in one subclass — there is no defense at the base-contract layer, and a future beacon upgrade or a non-Chop log deployment reintroduces it at full severity.

Fix. Track fee accrual during an active flash mint and exclude it from the repayment credit (require(balanceOf(address(this)) >= _balance + _amount + _fee + _feeAccruedDuringFlashMint)), or block V2_POOL transfers while isFlashMinting == 1. Move the mitigation into DecentralizedIndex itself (an immutable flashMintEnabled flag) rather than relying on a subclass override, so it survives implementation swaps on the shared beacon.

[M-1] A 1-wei donation + sync() permanently blocks a log's first liquidity add, through both ChopLogSeeder.seed() and the raw addLiquidityV2 path

Severity: Medium

File: contracts/ChopLogSeeder.sol:75-77, 188-194; contracts/DecentralizedIndex.sol:374-418 (addLiquidityV2)

Description. _validateEmptyPair requires r0 == 0 && r1 == 0 on the log's V2 pair before any first liquidity is added. The preceding skim(0xdead) + sync() only clears an unsynced donation (skim moves balance - reserve). Once a reserve is recorded via sync(), skim moves nothing and the non-zero reserve is permanent: swap needs both reserves > 0, burn divides by a zero LP totalSupply, and a direct mint would make totalSupply() != 0, which _validatePairWithoutLp separately rejects.

// ChopLogSeeder.sol:75-77
IUniswapV2Pair(pair).skim(address(0xdead));
IUniswapV2Pair(pair).sync();
_validateEmptyPair(pair, logAddress);
// :191
require(r0 == 0 && r1 == 0, "SYNCED_PAIR_DUST_USE_ATOMIC_CREATE");

Exploit. Any address: (1) IERC20(paired).transfer(pair, 1); (2) IUniswapV2Pair(pair).sync(). Cost ≈1 wei + ~50k gas. Every subsequent ChopLogSeeder.seed() call for that log reverts at the quoted require, and DecentralizedIndex.addLiquidityV2 independently reverts inside the Uniswap router's UniswapV2Library.quote, which requires both reserves > 0. This is unrecoverable for that specific log: the creator's only escape is a full redeploy (3 new beacon proxies + new pair) through the atomic createAndSeed path, which is immune because it creates the pair and seeds it in the same transaction (an attacker cannot sync() a pair for a log address that has no code yet). IndexManager.updateMakePublic is also permanently blocked for the griefed log (require(log.totalSupply() > 0 && ... "SEED_FIRST").

Convergence: independently found and verified by 9 of the 18 breadth/depth agents in this audit (ethskills defi-amm, ethskills general, and 7 of the pashov depth-hunt specialists), including an extension proving it also blocks the raw addLiquidityV2 path, not just the seeder. This is the single most-corroborated finding in the engagement.

Fix. Make _validateEmptyPair tolerant of a bounded dust reserve and fold it into the first-liquidity math, or deploy the log/pair pair with an unguessable CREATE2 salt so a third party cannot pre-target it, or give the log's bootstrap a path that calls pair.mint directly (absorbing synced dust) instead of routing through the standard router.

[M-2] ChopWeightedIndex's fee-conversion swap floor leaves only ~1.7–2.7% headroom against a TWAP that can be up to ~30 minutes stale

Severity: Medium

File: contracts/ChopWeightedIndex.sol:30-33, 43-73, 85-95; contracts/DecentralizedIndex.sol:190-208, 245-259

Description. _minimumFeeSwapOutput floors the permissionless fee-conversion swap at 0.97 × amountIn × TWAP, where TWAP is a V2-cumulative-price average spanning 600–1200 seconds and remains usable for a further 1200 seconds (so the price used can be up to ~30 minutes old). The 3% band never subtracts the swap's own cost: at the maximum batch size (feeBatchAmount caps it at 1% of the pool's lTKN reserve, DecentralizedIndex.sol:203), the achievable V2 output is only 0.98716× the TWAP-implied amount (0.30% pool fee + price impact), leaving ~1.74% of real headroom for price drift between the TWAP and spot (≈2.68% at the minimum batch size).

Two independent, concretely numeric consequences, confirmed by five independent agents with matching thresholds:

• Liveness failure: any ordinary price decline faster than ~1.7–2.7% within the TWAP's staleness window makes every processFees()/processPreSwapFeesAndSwap() call revert with INSUFFICIENT_OUTPUT_AMOUNT until the price recovers or the oracle catches up — LP-staker reward conversion simply stops.
• Sandwich on a rising market: because updateFeeOracle() is permissionless and processFees() carries only a 20-second cooldown, a searcher can, in one bundle, depress spot to the stale floor, call processFees() to buy the fee batch at the lagging price, then restore spot — extracting up to the full 3% gap repeatedly while fee inventory lasts.

Fix. Subtract the known pool fee + price impact from the expected amount before applying the slippage band (so the 3% is a true tolerance, not partially consumed by the swap's own mechanics); shrink FEE_ORACLE_MAX_QUOTE_AGE well below the window length; and/or add an owner-tunable fallback so the pipeline cannot stall indefinitely on one side and cannot be gamed on the other.

[M-3] WeightedIndex's inherited fee-conversion swap has no slippage floor at all (_minimumFeeSwapOutput returns 0) — only ChopWeightedIndex overrides it

Severity: Medium

File: contracts/DecentralizedIndex.sol:245-264

function _minimumFeeSwapOutput(uint256) internal view virtual returns (uint256) {
    return 0;
}

Description. The base WeightedIndex implementation — a fully shipped, factory-deployable contract, not a hypothetical — executes the entire accrued fee batch through DEX_HANDLER.swapV2Single(..., 0, ...) with zero slippage protection and no deadline worth the name (block.timestamp). processFees()/processPreSwapFeesAndSwap() are permissionless and re-callable every 20 seconds. Any attacker can, in one bundle: depress the log's price into V2_POOL, call processFees() (which also raises feeBatchAmount's cap, since it's sized off balanceOf(V2_POOL)), and buy back — extracting essentially all of a fee batch's value. Confirmed independently by three agents (two breadth, one depth) with matching mechanics.

Fix. Do not default to 0: make _minimumFeeSwapOutput abstract so every concrete index must supply a real floor, or derive a base-layer floor from the existing V3_TWAP_UTILS dependency and revert when no quote is available, mirroring ChopWeightedIndex's own protection.

[M-4] TokenRewards.depositFromPairedLpToken sends the yieldBurn fee slice to the admin sink instead of burning it, doubling the admin's effective take to up to 40%

Severity: Medium

File: contracts/TokenRewards.sol:181-192 (bug) vs. :264-288 (_depositRewards, the correct sibling path)

// TokenRewards.sol:184-191 — LEAVE_AS_PAIRED_LP_TOKEN branch
(, uint256 _yieldBurnFee) = _getYieldFees();
uint256 _burnAmount = _yieldBurnFee == 0 ? 0 : (_amountTkn * _yieldBurnFee) / PROTOCOL_FEE_ROUTER.protocolFees().DEN();
_adminAmt += _burnAmount;          // <-- burn slice added to the admin fee, not burned
_amountTkn -= _burnAmount;
if (_adminAmt > 0) { _processAdminFee(_adminAmt); }   // both slices sent to the same sink

Compare the correct handling in the sibling function, two call-sites away:

// TokenRewards.sol:276-281
if (_yieldBurnFee > 0) {
    uint256 _burnAmount = (_amountTotal * _yieldBurnFee) / PROTOCOL_FEE_ROUTER.protocolFees().DEN();
    if (_burnAmount > 0) { _burnRewards(rewardsToken, _burnAmount); }   // <-- actually burned (or swept on burn failure)
    _depositAmount -= _burnAmount;
}

Description. ProtocolFees caps yieldAdmin and yieldBurn at 20% each specifically so the admin's take is bounded at 20% and the burn slice benefits remaining holders/is destroyed. On the LEAVE_AS_PAIRED_LP_TOKEN deposit path, the burn slice is folded into _adminAmt and sent to the same admin sink as the admin fee — with both fees at their 20% cap, the admin collects up to 40% of that deposit instead of the intended 20%, with the other 20% simply never reaching either stakers or a burn.

Fix. Replace _adminAmt += _burnAmount; with a call to _burnRewards(PAIRED_LP_TOKEN, _burnAmount);, matching _depositRewards's handling of the same fee parameter.

[M-5] IndexUtils.bond / addLPAndStakeSingleSided grant a caller-chosen "index fund" contract standing approval over IndexUtils's own token balances

Severity: Medium

File: contracts/IndexUtils.sol:38-64 (bond), :175-193 (_addAndStakeSingleSided)

Description. bond(IDecentralizedIndex _indexFund, ...) takes _indexFund directly from the caller with no check that it is a real, registered protocol log (no IIndexManager.isRegistered check anywhere in the function). It then asks the named contract what assets/amounts it needs (getAllAssets(), getInitialAmount()), pulls that much from the caller, and approves the named contract to spend IndexUtils's own balance of each asset:

// IndexUtils.sol:52-54
IERC20(_assets[_i].token).safeTransferFrom(_msgSender(), address(this), _amountNeeded);
IERC20(_assets[_i].token).safeIncreaseAllowance(address(_indexFund), _amountNeeded);

Exploit. An attacker supplies a contract they control as _indexFund, reporting a benign-looking asset list and a getInitialAmount that matches whatever they want approved. bond() pulls the attacker's own funds (refunded to them at the end — self-funded, no net cost) but leaves the approval on IndexUtils's balance of that asset standing after the call returns, since nothing resets it. A duplicate-asset variant (getAllAssets() returning the same token twice — impossible for a real log, enforced at WeightedIndex.sol's require(!_isTokenInIndex[...]), but not checked by IndexUtils) drains IndexUtils's pre-existing dust balance of that token directly in the same call. IndexUtils is a shared, multi-user contract that deliberately retains 1 wei of every asset and paired token it has ever touched (by design, for gas reasons), so a standing unbounded allowance to an attacker-controlled contract is a live, if currently dust-sized, risk that grows with the contract's residual balances over time.

Convergence: independently found by two separate depth-hunt specialists with matching technical detail.

Fix. Require IIndexManager(indexManager).isRegistered(address(_indexFund)) before use, and reset each allowance to zero after _indexFund.bond(...) returns.

[M-6] IndexManager.removeIndex + addIndex let any authorized address reassign a log's creator, hijacking the first-bond bootstrap gate

Severity: Medium

File: contracts/IndexManager.sol:134-154 (addIndex/_addIndex), :156-167 (removeIndex)

Description. removeIndex (gated onlyAuthorized) deletes a registry entry with no check on who created it; addIndex (also onlyAuthorized) then re-registers the same log address with an arbitrary new creator, and — critically — ChopWeightedIndex._beforeBond's bootstrap gate (WeightedIndexFactory.canBootstrap) reads that creator field directly to decide who may take the log's fee-free, price-setting first bond.

Exploit. Any authorized address A (a role the owner must grant broadly — e.g. to ChopLogSeeder itself, since ChopLogSeeder.createAndSeed needs _authorizedOrOwner to deploy on a user's behalf): calls removeIndex(idx) on a creator C's deployed-but-unseeded log, then addIndex(log, A, false, false, false). A is now the registered creator; A alone now passes canBootstrap's getIndex(log).creator == account check and can take the first, fee-free bond at the fixed launch price, permanently locking out C. A variant that calls removeIndex without re-adding bricks the log's first bond for everyone, because canBootstrap's downstream getIndex call reverts UNKNOWN_INDEX on an unregistered log rather than returning false.

Convergence: independently found by two separate agents (one breadth, one depth) with matching mechanics.

Fix. Record creator in a separate, write-once mapping that _addIndex can never overwrite for a log isDeployedLog already recognizes, and restrict removeIndex to the owner (not the broader authorized set) or forbid removing a log that has not yet had its creator's bootstrap consumed.

[M-7] IndexManager.addIndex can set makePublic = true with none of the guards updateMakePublic enforces for the identical state change

Severity: Medium

File: contracts/IndexManager.sol:134-154 (addIndex/_addIndex) vs. :184-200 (updateMakePublic)

Description. updateMakePublic requires verified, a passing validateIndex(_index), and a seeded pool (log.totalSupply() > 0 && pool.totalSupply() > 0) before setting makePublic = true. _addIndex writes the same field through addIndex with none of those checks — validateIndex only runs when the caller also sets _verified = true, which addIndex lets any authorized caller leave false while still setting makePublic = true.

Exploit. authorized caller → addIndex(anyAddress, caller, false, false, true). anyAddress needs no code, no CHOP integration, no liquidity — it is registered as "public" with none of the canonical preflight that the guarded door enforces. Anything (a front-end, an integrator) that reads makePublic as "this log passed verification" is misled.

Fix. Either reject addIndex(..., _makePublic=true) outright (force callers through updateMakePublic), or factor updateMakePublic's three checks into a shared internal function both entry points call.

[M-8] A malicious/reentrant underlying token exploits the noSwapOrFee global flag to bypass buy/sell fees during bond/debond/addLiquidityV2/removeLiquidityV2

Severity: Medium

File: contracts/DecentralizedIndex.sol:77-81 (noSwapOrFee modifier), :163-187 (_update)

Description. noSwapOrFee sets a single storage flag _swapAndFeeOn = 0 for the duration of the modified function's body, and _update's fee computation is gated entirely on that flag (if (_swapping == 0 && _swapAndFeeOn == 1)). The flag is global per-log state, not scoped to a specific transfer. WeightedIndex._bond (reached from bond, which carries noSwapOrFee) calls transferFrom on every underlying asset the creator configured for the log — if that asset's transferFrom runs attacker-supplied code (a classic ERC777-style or malicious-token hook), that code executes while _swapAndFeeOn == 0, and any lTKN transfer it makes (e.g. straight to V2_POOL) pays zero buy/sell/transfer tax. The same window exists in debond, addLiquidityV2, and removeLiquidityV2.

Reachability. Requires the log's creator to have configured (or the index to otherwise accept) an underlying/paired asset with hostile transfer-hook behavior — a real but bounded precondition, in the same family the codebase already defends against for fee-on-transfer/rebasing tokens elsewhere. Impact falls on the log's fee revenue (hence its stakers), not only on the party who chose the asset.

Fix. Scope the fee exemption to the specific counterparties the router/adapter actually needs (e.g. check msg.sender/the actual caller context) rather than a global flag that silently exempts any code that gains control during the window.

[L-1] ProtocolFeeRouter.setProtocolFees lets the owner swap in a ProtocolFees implementation that is not actually bound by the documented 20% yield-fee caps

Severity: Low (centralization — no unprivileged amplifier found; recorded because the documented invariant is not actually enforced where it is read)

File: contracts/ProtocolFeeRouter.sol:18-26; contracts/TokenRewards.sol:377-391

_checkFees validates only code.length > 0 && fees.DEN() > 0 — it never checks the new contract's yieldAdmin()/yieldBurn() against any bound, and TokenRewards reads both values live against whatever DEN() the currently-installed contract reports. An owner (or compromised owner key) can install a ProtocolFees with yieldAdmin = 99/100 and redirect up to 99% of every reward deposit to the admin sink, instantly, with no timelock. Fix: pin DEN() to the protocol constant and bound yieldAdmin + yieldBurn in _checkFees itself, and add a timelock to setProtocolFees.

[L-2] WeightedIndex._bond's first-deposit branch uses native multiplication where its own convertToShares preview uses 512-bit FullMath, overflowing (reverting) for high-supply 18-decimal underlyings

Severity: Low (DoS only — a smaller first-bond amount works around it; no fund loss)

File: contracts/WeightedIndex.sol:150 vs. :93

// :150 — reverts above ~1.46e30 raw units (~1.46e12 whole 18-decimal tokens)
_tokensMinted = (_amount * FixedPoint96.Q96 * 10 ** decimals()) / indexTokens[_tokenIdx].q1;
// :93 — the identical quantity, via 512-bit math, does not revert
_shares = FullMath.mulDiv(_assets, FixedPoint96.Q96 * 10 ** decimals(), indexTokens[0].q1);

Confirmed independently by two agents with matching overflow thresholds. Fix: use FullMath.mulDiv at line 150 (and the equivalent non-first-in computations at :147,152 and debond's :185), matching the view.

[L-3] convertToShares/convertToAssets always apply the bond/debond fee even on the branches where _bond/debond actually waive it

Severity: Low (integration-correctness — misleads slippage-parameter calculations, no direct fund loss)

File: contracts/WeightedIndex.sol:90-100, 117-130 vs. _canWrapFeeFree (DecentralizedIndex.sol:286-292) and _isLastOut (:303-305)

The view always subtracts _fees.bond/_fees.debond; the executing functions waive the fee entirely on first-bond and on _isLastOut/whitelisted-caller debonds. An integrator sizing _amountMintMin/_amountOut off the view is off by exactly the fee amount on exactly the transaction where price-setting matters most (the first bond). Confirmed independently by four agents. Fix: move the fee deduction inside the non-waived branch in both views.

[L-4] Beacon upgrades are protocol-wide, instant, and un-timelocked for every log/staking-pool/rewards contract

Severity: Low (documented centralization risk, out of source-code scope to fully bound — beacon ownership itself was not part of the audited application-layer source)

File: contracts/WeightedIndexFactory.sol:18-25, 46-83

Each of weightedIndexBeacon/stakingPoolTokenBeacon/tokenRewardsBeacon governs every deployed instance with no per-instance override, no timelock, and (per DecentralizedIndex.setup()'s renounceOwnership() on the per-log StakingPoolToken) no local owner left after setup to object. Fix: put each beacon behind a timelocked multisig and have IndexManager.validateIndex assert the implementation codehash matches an approved value.

[L-5] The ChopWeightedIndex beacon swap leaves the new fee-oracle storage zeroed with no reinitializer, bricking processFees() on every existing log until manually primed

Severity: Low (one-time migration defect, recoverable by calling updateFeeOracle() twice ~10 minutes apart per log)

File: contracts/ChopWeightedIndex.sol:37-40, 85-95

A beacon upgrade from WeightedIndex to ChopWeightedIndex gives every existing log the new _minimumFeeSwapOutput override, but slots 80-83 (_feePriceCumulativeLast etc.) are freshly zeroed with no reinitializer. Until someone calls updateFeeOracle() twice, ~600-1200s apart, processFees()/processPreSwapFeesAndSwap() revert FeeOracleNotReady for that log, stranding accrued lTKN fees. Fix: ship a reinitializer(2) that seeds the first observation as part of the upgrade, and/or make _minimumFeeSwapOutput degrade to 0/skip rather than revert when unprimed.

[L-6] WeightedIndex declares no storage gap of its own between DecentralizedIndex.__gap and ChopWeightedIndex's new state

Severity: Low (latent — becomes a critical storage-corruption issue on the next WeightedIndex revision, not reachable today)

File: contracts/WeightedIndex.sol (no state, no gap); contracts/DecentralizedIndex.sol:63; contracts/ChopWeightedIndex.sol:36-40

Any future addition of storage to WeightedIndex itself (the natural place to add a shared feature) will alias ChopWeightedIndex's existing slots 80+. Fix: add uint256[50] private __gap; to WeightedIndex now, before any mainnet log exists on this beacon lineage, and add CI storage-layout diffing for every future upgrade pair.

[L-7] flashMint is disabled only by a subclass override; the base contract has no defense-in-depth, and convertToAssetsPreFlashMint is permanently dead on every canonical log as a side effect

Severity: Low

File: contracts/ChopWeightedIndex.sol:18-20; contracts/WeightedIndex.sol:112-115

Covered mechanically by [H-1]'s reachability discussion; recorded separately because convertToAssetsPreFlashMint — part of the public IDecentralizedIndex ABI — now unconditionally reverts NO_FLASH_MINT for every canonical log, which can surprise an integrator that calls it defensively. Fix: move the disable flag into DecentralizedIndex itself; have the view degrade to convertToAssets instead of reverting when flash-minting is disabled.

[L-8] V4RewardRouter.keeper is an unrecoverable single point of failure for all non-CHOP-paired reward distribution, and revoking a router via IndexManager.setRewardRouterAllowed has no effect on live TokenRewards

Severity: Low

File: contracts/TokenRewards.sol:69, 106-113, 170-231; contracts/IndexManager.sol:54-58

Once rewardRouter != address(0) for a log, the only conversion path for its queued paired-token rewards is processPairedRewards, gated to IV4RewardRouter(rewardRouter).keeper() — with no setter, no timeout, and no permissionless fallback. approvedRewardRouters is checked only at log-creation/validation time, never at spend time, so revoking a compromised router's approval does nothing to stop TokenRewards from continuing to use it. Fix: add a keeper-staleness fallback (anyone may convert after N days with their own minOut) and re-check approvedRewardRouters at spend time in processPairedRewards.

[L-9] RewardsWhitelist.setPaused is checked only on the claim side, so pausing a reward token freezes withdrawals of already-earned rewards while deposits/accrual continue silently

Severity: Low

File: contracts/TokenRewards.sol:342-344 (checked) vs. _depositRewards/_isValidRewardsToken (not checked)

Because rewardsToken == CHOP for every canonical log, pausing CHOP freezes claims protocol-wide in one transaction while admin-fee skims and _rewardsPerShare accrual continue unaffected; claimReward's per-token try/catch additionally swallows the paused case silently (no distinguishing event). Fix: check paused on the deposit side instead of (or in addition to) the claim side, and bound any pause with a maximum duration.

[L-10] Zapper's V2 and Curve swap branches apply no fallback minimum when the caller passes 0, unlike the V3 branch, which derives a TWAP-based floor

Severity: Low

File: contracts/Zapper.sol:88-92, 205-224 (unprotected) vs. :159-172 (_swapV3Single, protected)

On chain 4663, zapMap is empty unless the owner populates it (the constructor only seeds mainnet addresses), so every _zap call falls through to the unprotected V2 branch today. IndexUtils.addLPAndStake forwards the caller's _amountPairedLpTokenMin unchanged, and a caller/integrator that passes 0 (a plausible default, since the function's own floor is only require(_amountOut > 0)) gets a fully sandwichable swap. Fix: mirror _swapV3Single's TWAP-derived fallback in _swapV2/_swapCurve, or reject a zero minimum outright.

[L-11] DecentralizedIndex.flash's DAI fee can be routed to address(0) with no guard, unlike every other admin-fee sink in the codebase

Severity: Low

File: contracts/DecentralizedIndex.sol:450-453 vs. contracts/TokenRewards.sol:256-257, 367-368, 426-427 (all have require(sink != address(0), "ZERO_SINK"))

If V3TwapUtilities's owner ever renounces ownership (a plain one-step Ownable, no override), every subsequent flash() either silently burns 10 DAI per call to the zero address (if DAI doesn't block it) or bricks flash() entirely (if it does) — with no admin recovery path, since DAI/FLASH_FEE_AMOUNT_DAI/V3_TWAP_UTILS have no setters. Fix: add the same ZERO_SINK guard used everywhere else in the codebase.

Informational

• [I-1] TokenRewards.getUnpaid omits the lockedRewards subtraction that _claimRewardToken applies, so the view over-reports claimable balances for wallets with immature stake tranches. No on-chain caller of the view was found in scope. (5 independent agents.)
• [I-2] TokenRewards._burnRewards uses a bare try { ICHOP(_token).burn(_burnAmount) } with no post-call balance check; a phantom/no-op burn(uint256) (e.g. via a proxy whose implementation lacks it) silently strands the tokens rather than falling through to the sink transfer.
• [I-3] DecentralizedIndex.addLiquidityV2's safeIncreaseAllowance(DEX_HANDLER, 0) is a no-op, not a reset (OZ safeIncreaseAllowance(x,0) increases by zero); harmless only because the shipped adapter happens to consume the full allowance it's given.
• [I-4] StakingPoolToken/TokenRewards reward-token loops (_setShares/_resetExcluded/_lockImmatureRewards) scale with MAX_REWARD_TOKENS (13) × 11 lock buckets with no gas cap on stake/unstake/transfer; claimReward's hardcoded {gas: 750_000} per-token stipend was estimated by three independent agents at 70-95% of its own ceiling in the worst case (11 populated lock buckets). Not currently reachable to a hard revert at the canonical single-reward-token configuration, but owner-controlled growth of the reward-token list narrows the margin.
• [I-5] WeightedIndex.sol:68-71's weight-normalization math and TokenRewards.sol:204-209's V3-TWAP reward-quote math both use native (non-FullMath) multiplication on oracle-derived or weight-derived values with theoretical overflow/divide-by-zero bounds; unreachable at the canonical single-asset, realistic-decimals configuration enforced elsewhere in the codebase, but live surface for any future non-canonical configuration.
• [I-6] V2CumulativePrice.sol:37-38's _priceX112 * _elapsed sits outside its surrounding unchecked block despite a comment claiming it always fits; the true bound is 2^224 × 2^32 = 2^256, tight but not strictly within range. Requires reserve ratios and elapsed times far outside any realistic pool; not reachable in practice.
• [I-7] _isLastOut's >= 99% threshold (rather than == 100%) lets a near-total holder debond fee-free while a residual minority remains, and the waiver composes (repeatable down to a 1-unit residual), shifting a small amount of the debond fee's benefit away from genuinely-remaining holders.
• [I-8] DecentralizedIndex._update's require(!_blacklist[_to], "BK") check has no corresponding writer anywhere in scope (the storage slot is kept only for proxy layout compatibility per its own comment) — dead code that makes the token look blacklist-capable to integrators when it is not.
• [I-9] Rebasing or share-accounted underlying assets (not explicitly excluded by IndexManager._validateCreation, which only checks code.length > 0) desync the cached _totalAssets[token] counter from the real balance, in both directions — a negative rebase creates a last-debonder-eats-the-shortfall shape, a positive rebase permanently strands the surplus yield. Requires the log creator to choose (or a token to later become) a non-standard-balance asset.
• [I-10] IndexManager.validateIndex omits the stakeUserRestriction/stakingToken cross-checks that ChopLogSeeder._validate separately applies to the same canonicality question — the two validators of "is this log canonical" check different things, and a log reaching the registry by a route other than deployNewIndex is not fully covered by either.

Coverage Gate

• Entrypoints: 51 external/public state-changing functions enumerated in the grep-verified access-control inventory (x-ray/entry-points.md), 51 addressed across the findings above or the negative-result log below.
• Threat-catalog rows: every actor×entrypoint×asset row in protocol-map.md §4 is answered by a finding above or an explicit "examined, no issue" note in this gate.
• Invariants: 6 single-contract + 4 cross-contract + 2 economic inferred rows from the x-ray pre-scan, all answered:
  • I-1 (conservation), I-3 (fee bounds at write-site, excepting L-1's read-side bypass), I-4 (bond/debond snapshot ordering), E-2 (donation-immune first-bond pricing): hold, confirmed by multiple independent agents, no gap found.
  • I-2 (flash-mint / custom-_totalSupply boundary): holds for the specific mechanism it names (the shared lock does block every share-math reentrancy path) — but see H-1, a different mechanism (fee-revenue recycling through the unguarded _update path) achieves a related exploit the invariant did not anticipate.
  • I-5/I-6 (reward-lock state machine, fee-oracle readiness state machine): hold structurally; see M-2 for the oracle's economic insufficiency, which is a distinct issue from the state machine's correctness.
  • X-1 (trackingToken ABI assumption): holds for the canonical factory-paired deployment; silent-degrade risk only on a hypothetical future non-canonical pairing (recorded as a lead by multiple depth agents, no live path found).
  • X-2 (stake/reward-lock bucket collision — the pre-scan's highest-priority flagged gap): REFUTED. Independently falsified by nine separate agents (both breadth and depth) via exact modular-arithmetic proof: with unlockAt ∈ [t+600, t+659] and 11 buckets, any two tranches sharing a bucket are forced ≥660 seconds apart, so the "colliding" predecessor has always already matured before the new write lands. LOCK_COLLISION/REWARD_LOCK_COLLISION cannot fire. This is the correct resolution of the lead the pre-scan raised — recorded here as holds, not as a finding.
  • X-3 (keeper trust boundary): holds as a documented trust assumption; see L-8 for its single-point-of-failure consequence.
  • X-4 (pre-lock non-canonical deployment window): holds as intentional bootstrap sequencing; see M-6/M-7 for the authorized-role gaps that interact with it.
• Holes closed this pass: 0 (every entrypoint/threat-row/invariant-row above was examined by at least one phase; none were left completely unaddressed by both phases simultaneously).

Invariant Verification

This review was performed using an automated multi-agent AI pipeline (x-ray mechanical pre-scan, context-building, 8-agent checklist breadth pass, 12-agent adversarial depth hunt — 10 of 12 completed, 2 died to an output-token ceiling and were not respawned per methodology). AI analysis can never verify the complete absence of vulnerabilities and no guarantee of security is given. A team security review, a bug bounty program, and on-chain monitoring are strongly recommended before and after any mainnet deployment of this redeployment.