10c90e4c50
- 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>
216 lines
11 KiB
Markdown
216 lines
11 KiB
Markdown
# 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
|