harb/onchain/analysis/SECURITY_REVIEW.md
openhands 10c90e4c50 fix: address AI review findings on SECURITY_REVIEW.md and deployment.md (#838)
- M-2: update body to show current deployer-only setFeeDestination()
  implementation and conditional locking; mark as partially resolved;
  downgrade severity from Medium to Low; update conclusion entry
- I-1: mark as resolved — Recentered event declared at line 66 and
  emitted at line 224 of LiquidityManager.sol
- I-2: correct VWAP direction (records on sells/ETH outflow, not buys);
  update stale line reference from 146-158 to 177-191
- deployment.md §6.5: replace vague 'assess severity' step 1 with
  concrete action (upgrade optimizer to bear defaults via §6.2)
- deployment.md §8 timeline: remove stale 'Set recenter access' row;
  update 'First recenter' dependency

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-03-16 15:15:33 +00:00

11 KiB

Security Review: KRAIKEN Protocol Contracts

Date: 2026-02-13 Branch: launch/protocol-readiness Reviewer: Automated deep review (Claude) Scope: LiquidityManager, OptimizerV3, ThreePositionStrategy, VWAPTracker, PriceOracle, Kraiken, Stake


Executive Summary

The KRAIKEN protocol contracts are well-structured with clear separation of concerns. No critical exploitable vulnerabilities were found that would block mainnet launch. Two medium-severity issues should be addressed before deployment: a TWAP calculation bug in the PriceOracle fallback path, and the lack of access control on one-time setter functions during deployment. Several low-severity and informational findings are documented below.

The previously identified Floor Ratchet Extraction Attack (branch fix/floor-ratchet) remains the highest-priority issue and is tracked separately.


Findings

M-1: PriceOracle Fallback TWAP Uses Wrong Divisor

Severity: Medium File: src/abstracts/PriceOracle.sol:37-40

// Fallback to longer timeframe if recent data unavailable
secondsAgo[0] = PRICE_STABILITY_INTERVAL * 200;  // 60,000 seconds
(int56[] memory tickCumulatives,) = pool.observe(secondsAgo);
int56 tickCumulativeDiff = tickCumulatives[1] - tickCumulatives[0];
averageTick = int24(tickCumulativeDiff / int56(int32(PRICE_STABILITY_INTERVAL)));  // divides by 300, not 60000

Issue: The fallback path observes a 60,000-second window but divides by 300 (the original 5-minute interval). This produces an averageTick that is 200x the actual TWAP value.

Impact: When the fallback triggers (new pool with <5min of observation history), _isPriceStable() always returns false, making recenter() permanently blocked until enough observations accumulate. This is a liveness issue for newly deployed pools — the protocol cannot perform its first recenter until the pool has accumulated sufficient TWAP history.

Note: This is fail-safe (blocks recenter rather than allowing manipulation), so it's not exploitable. However, it could delay mainnet activation.

Fix:

uint32 fallbackInterval = PRICE_STABILITY_INTERVAL * 200;
secondsAgo[0] = fallbackInterval;
(int56[] memory tickCumulatives,) = pool.observe(secondsAgo);
int56 tickCumulativeDiff = tickCumulatives[1] - tickCumulatives[0];
averageTick = int24(tickCumulativeDiff / int56(int32(fallbackInterval)));

M-2: One-Time Setters Lack Access Control (Deployment Race) (Partially addressed)

Severity: MediumLow (partially resolved) Files:

  • src/LiquidityManager.sol:123-136setFeeDestination()resolved
  • src/Kraiken.sol:64-68setLiquidityManager() — open
  • src/Kraiken.sol:76-80setStakingPool() — open

Original issue: These set-once functions had no msg.sender restriction (first caller wins).

Resolution for setFeeDestination(): The function is now deployer-only with a conditional locking mechanism — EOA addresses may be updated, but once a contract address is assigned the destination is permanently locked:

function setFeeDestination(address feeDestination_) external {
    require(msg.sender == deployer, "only deployer");
    if (address(0) == feeDestination_) revert ZeroAddressInSetter();
    require(
        !feeDestinationLocked && (feeDestination == address(0) || feeDestination.code.length == 0),
        "fee destination locked"
    );
    feeDestination = feeDestination_;
    if (feeDestination_.code.length > 0) {
        feeDestinationLocked = true;
    }
}

Remaining exposure: setLiquidityManager() and setStakingPool() on Kraiken.sol remain set-once with no caller restriction. The mitigating factors from the original finding still apply — DeployBase.sol calls all setters atomically within the same broadcast, and Base L2 sequencer ordering reduces frontrunning risk.

Recommendation: No action required if deployment uses bundled transactions. Optionally restrict setLiquidityManager / setStakingPool to a constructor-set deployer address for defence in depth.


M-3: Open recenter() Access Without Rate Limiting (Addressed)

Severity: Medium (griefing)Informational (resolved) File: src/LiquidityManager.sol:155-172

Original concern: When recenterAccess == address(0), anyone could call recenter() as long as the TWAP check passed, with no cooldown or rate limiting.

Resolution: The recenterAccess role and its associated setter/revoker functions (setRecenterAccess, revokeRecenterAccess) have been removed from the contract. recenter() is now unconditionally permissionless. In their place, a MIN_RECENTER_INTERVAL = 60 second cooldown is enforced on every call path with no bypass:

function recenter() external returns (bool isUp) {
    (, int24 currentTick,,,,,) = pool.slot0();

    // Always enforce cooldown and TWAP price stability — no bypass path
    require(block.timestamp >= lastRecenterTime + MIN_RECENTER_INTERVAL, "recenter cooldown");
    require(_isPriceStable(currentTick), "price deviated from oracle");
    lastRecenterTime = block.timestamp;

Assessment: The 60-second cooldown directly addresses the original gas-griefing and VWAP-frequency concerns. Combined with the amplitude check (>400 ticks from center) and TWAP oracle guard (5-min, 50-tick tolerance), the attack surface is materially reduced. No further action required.


L-1: Division by Zero Edge Case in Kraiken mint/burn

Severity: Low File: src/Kraiken.sol:109, 130

uint256 newStake = stakingPoolBalance * _amount / (totalSupply() - stakingPoolBalance);

Issue: If totalSupply() == stakingPoolBalance (staking pool holds 100% of tokens), the denominator is zero.

Impact: recenter() reverts when trying to mint KRK for new positions.

Mitigating factors: In practice, the LM always holds significant KRK in positions, making totalSupply() > stakingPoolBalance invariant. The only way to reach this state would be for the LM to burn all its tokens AND for all remaining supply to be in the staking pool — which would require zero active positions (impossible mid-operation since recenter burns then mints).


L-2: OptimizerV3 Integer Truncation at Bull/Bear Boundary

Severity: Low File: src/OptimizerV3.sol:152

uint256 stakedPct = percentageStaked * 100 / 1e18; // truncates, doesn't round

Issue: 91.9% staked truncates to 91, triggering bear mode even though staking is close to the 92% threshold.

Impact: The bull/bear boundary has ~1% hysteresis due to truncation. This is actually beneficial — it makes the boundary slightly harder to reach, adding a buffer against oscillation.


I-1: Missing Recentered Event (Addressed)

Severity: InformationalResolved File: src/LiquidityManager.sol:66, 224

recenter() now emits a Recentered(int24 indexed currentTick, bool indexed isUp) event declared at line 66 and emitted at line 224 on every successful recenter. Monitoring and indexing via Ponder or direct RPC log filtering is fully supported.


I-2: VWAP Directional Recording Is Sound But Has Known Limitations

Severity: Informational File: src/LiquidityManager.sol:177-191

The directional VWAP recording (only record on ETH outflow / sells — i.e. when price falls) is a deliberate design choice to prevent buy-side VWAP inflation. An attacker could theoretically buy to push the price up without VWAP recording, but cannot inflate VWAP through buy-recenter cycles because VWAP is frozen during price rises. However, a determined attacker could sell to force VWAP updates at lower prices. Mitigating factors:

  • Selling incurs real ETH cost (not free to manipulate)
  • VWAP is volume-weighted, so one-off manipulation is diluted by historical volume
  • The VWAP mirror defense naturally increases floor distance during sell pressure

This is acceptable behavior by design.


I-3: ThreePositionStrategy Floor Position at Zero Outstanding Supply

Severity: Informational File: src/abstracts/ThreePositionStrategy.sol:191-192

When outstandingSupply reaches 0 after subtracting pulledKraiken and discoveryAmount, the scarcity tick goes to MAX_TICK (extreme KRK-cheap). This is correct by design — when there's no outstanding supply to protect, the floor should be as far as possible from current price, locking ETH in a position that's virtually unreachable.


Access Control Summary

Function Contract Access Notes
recenter() LiquidityManager Anyone (TWAP-gated + 60s cooldown) recenterAccess role removed; cooldown enforced unconditionally
setFeeDestination() LiquidityManager Deployer only Restricted post-refactor
mint() / burn() Kraiken onlyLiquidityManager Secure
setLiquidityManager() Kraiken Anyone (set-once) Race condition risk
setStakingPool() Kraiken Anyone (set-once) Race condition risk
upgradeTo() OptimizerV3 (proxy) onlyAdmin (deployer) Secure
initialize() OptimizerV3 initializer guard (once) Secure
uniswapV3MintCallback() LiquidityManager CallbackValidation.verifyCallback Secure

Reentrancy Analysis

recenter(): No reentrancy risk. The function:

  1. Reads pool state (slot0)
  2. Burns all positions via pool.burn() (Uniswap V3 pools are not reentrant)
  3. Collects tokens via pool.collect()
  4. Transfers fees to feeDestination
  5. Mints new positions via pool.mint()

The uniswapV3MintCallback is validated via CallbackValidation.verifyCallback(factory, poolKey) which ensures only the canonical pool can trigger it. The callback mints KRK tokens and wraps ETH — neither of which creates reentrant paths back to recenter().

_scrapePositions(): Token transfers (IERC20.transfer) to feeDestination could theoretically trigger a callback if feeDestination is a contract. However, WETH and KRK transfers do not have callback hooks (no ERC-777 or similar), so this is safe.


Known Issues (Tracked Separately)

Floor Ratchet Extraction Attack

Branch: fix/floor-ratchet Severity: High (exploitable in 2000+ trade scenarios) Summary: Rapid recenters ratchet the floor position toward current price while packing ETH into it, enabling extraction through coordinated buy-crash-recenter-sell cycles. See MEMORY.md deep fuzzing results for full analysis.


Conclusion

The protocol is ready for mainnet deployment with the following pre-launch actions:

  1. Fix M-1 (PriceOracle fallback divisor) — simple one-line fix
  2. M-2 partially resolvedsetFeeDestination() now has deployer-only access; setLiquidityManager() / setStakingPool() remain open (mitigated by bundled deployment)
  3. Mitigate M-3Resolved: recenterAccess was removed; MIN_RECENTER_INTERVAL (60s) cooldown is now enforced unconditionally on all recenter() calls
  4. Continue tracking the Floor Ratchet vulnerability on its dedicated branch