chore: analysis tooling, research artifacts, and code quality
- Analysis: parameter sweep scripts, adversarial testing, 2D frontier maps - Research: KRAIKEN_RESEARCH_REPORT, SECURITY_REVIEW, STORAGE_LAYOUT - FuzzingBase: consolidated fuzzing helper, BackgroundLP simulation - Sweep results: CSV data for full 4D sweep (1050 combos), bull-bear, AS sweep, VWAP fix validation - Code quality: .gitignore for fuzz CSVs, gas snapshot, updated docs - Remove dead analysis helpers (CSVHelper, CSVManager, ScenarioRecorder) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
openhands
parent
d64b63aff4
commit
b7260b2eaf
256 changed files with 30276 additions and 1579 deletions
225
onchain/analysis/SECURITY_REVIEW.md
Normal file
225
onchain/analysis/SECURITY_REVIEW.md
Normal file
|
|
@ -0,0 +1,225 @@
|
|||
# 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 without `recenterAccess` being set.
|
||||
|
||||
**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)
|
||||
|
||||
**Severity:** Medium
|
||||
**Files:**
|
||||
- `src/LiquidityManager.sol:102-106` — `setFeeDestination()`
|
||||
- `src/Kraiken.sol:64-68` — `setLiquidityManager()`
|
||||
- `src/Kraiken.sol:76-80` — `setStakingPool()`
|
||||
|
||||
**Issue:** These set-once functions have no `msg.sender` restriction. Anyone can call them before the deployer:
|
||||
|
||||
```solidity
|
||||
function setFeeDestination(address feeDestination_) external {
|
||||
if (address(0) == feeDestination_) revert ZeroAddressInSetter();
|
||||
if (feeDestination != address(0)) revert AddressAlreadySet();
|
||||
feeDestination = feeDestination_; // first caller wins
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:** An attacker watching the mempool could frontrun deployment to:
|
||||
- Set themselves as `feeDestination` → steal all LP fees forever
|
||||
- Set a malicious `liquidityManager` → gain mint/burn control over KRK supply
|
||||
|
||||
**Mitigating factors:**
|
||||
- `DeployBase.sol` calls all setters in the same broadcast transaction as deployment
|
||||
- On Base L2, sequencer ordering reduces frontrunning risk vs L1
|
||||
- Using private mempools / bundled transactions eliminates the risk entirely
|
||||
|
||||
**Recommendation:** Either:
|
||||
1. Accept the risk with bundled deployment (current approach works on Base), or
|
||||
2. Add a constructor-set `deployer` address as the only authorized caller for these setters
|
||||
|
||||
---
|
||||
|
||||
### M-3: Open `recenter()` Access Without Rate Limiting
|
||||
|
||||
**Severity:** Medium (griefing)
|
||||
**File:** `src/LiquidityManager.sol:121-129`
|
||||
|
||||
```solidity
|
||||
function recenter() external returns (bool isUp) {
|
||||
if (recenterAccess != address(0)) {
|
||||
require(msg.sender == recenterAccess, "access denied");
|
||||
} else {
|
||||
require(_isPriceStable(currentTick), "price deviated from oracle");
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** When `recenterAccess == address(0)`, anyone can call `recenter()` as long as the TWAP check passes. There is no cooldown or rate limiting.
|
||||
|
||||
**Impact:**
|
||||
- Gas griefing: attacker triggers unnecessary recenters, wasting protocol gas on position burns/mints
|
||||
- VWAP frequency manipulation: more frequent recenters with small price movements could subtly influence VWAP recording
|
||||
- Each recenter costs 540k-820k gas, so griefing has a cost to the attacker too
|
||||
|
||||
**Mitigating factors:**
|
||||
- The TWAP oracle check (5-min, 50-tick tolerance) limits when recenter can be called
|
||||
- The amplitude check requires meaningful price movement (>400 ticks from center)
|
||||
- In practice, `recenterAccess` should be set to the txnBot address after deployment
|
||||
|
||||
**Recommendation:** Set `recenterAccess` to the txnBot immediately after deployment. Consider adding a minimum time between recenters (e.g., 60 seconds).
|
||||
|
||||
---
|
||||
|
||||
### 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
|
||||
|
||||
**Severity:** Informational
|
||||
**File:** `src/LiquidityManager.sol:121`
|
||||
|
||||
`recenter()` performs the most critical protocol operation but emits no event. The `EthScarcity`/`EthAbundance` events exist in `ThreePositionStrategy` but only fire during floor tick computation. A top-level `Recentered(int24 tick, bool isUp)` event would improve monitoring and indexing.
|
||||
|
||||
---
|
||||
|
||||
### I-2: VWAP Directional Recording Is Sound But Has Known Limitations
|
||||
|
||||
**Severity:** Informational
|
||||
**File:** `src/LiquidityManager.sol:146-158`
|
||||
|
||||
The directional VWAP recording (only record on ETH inflow / buys) is a deliberate design choice to prevent sell-side VWAP dilution. An attacker could theoretically buy to inflate VWAP, then sell without VWAP recording. However:
|
||||
- Buying costs real ETH (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 | `recenterAccess` or anyone (TWAP-gated) | Set recenterAccess to txnBot |
|
||||
| `setFeeDestination()` | LiquidityManager | Anyone (set-once) | Race condition risk |
|
||||
| `setRecenterAccess()` | LiquidityManager | `onlyFeeDestination` | Secure |
|
||||
| `revokeRecenterAccess()` | LiquidityManager | `onlyFeeDestination` | Secure |
|
||||
| `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. **Mitigate M-2** by using bundled transactions for deployment (already the case in DeployBase.sol)
|
||||
3. **Mitigate M-3** by setting `recenterAccess` to txnBot address immediately after deployment
|
||||
4. **Continue tracking** the Floor Ratchet vulnerability on its dedicated branch
|
||||
Loading…
Add table
Add a link
Reference in a new issue