harb/onchain/analysis/SECURITY_REVIEW.md

# 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`

```solidity
// 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:**
```solidity
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:** ~~Medium~~ → **Low (partially resolved)**
**Files:**
- `src/LiquidityManager.sol:123-136` — `setFeeDestination()` — **resolved**
- `src/Kraiken.sol:64-68` — `setLiquidityManager()` — open
- `src/Kraiken.sol:76-80` — `setStakingPool()` — 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:

```solidity
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:

```solidity
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`

```solidity
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`

```solidity
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:** ~~Informational~~ → **Resolved**
**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 resolved** — `setFeeDestination()` now has deployer-only access; `setLiquidityManager()` / `setStakingPool()` remain open (mitigated by bundled deployment)
3. ~~**Mitigate M-3**~~ — **Resolved:** `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