diff --git a/docs/technical/deployment.md b/docs/technical/deployment.md index 0f2c316..1c1c012 100644 --- a/docs/technical/deployment.md +++ b/docs/technical/deployment.md @@ -235,7 +235,7 @@ If the optimizer needs temporary override, deploy a new implementation with hard ### 6.5 Known Attack Response: Floor Ratchet If floor ratchet extraction is detected (rapid recenters + floor tick creeping toward current price): -1. **Immediately** assess severity — `recenter()` is permissionless (no access-control switch exists); the 60s cooldown is the only rate limiter +1. **Immediately** upgrade the optimizer to safe bear-mode defaults (§6.2) — this maximises floor distance (AW=100 → 7000-tick clearance) and makes ratchet extraction significantly harder while a patched LiquidityManager is prepared. Note: there is no access-control switch on `recenter()`; the 60s cooldown is the only rate limiter 2. Assess floor position state via `positions(0)` 3. Deploy patched LiquidityManager if fix is ready 4. Current mitigation: bear-mode parameters (AW=100) create 7000-tick floor distance, making ratchet extraction significantly harder @@ -280,9 +280,8 @@ Track these metrics via Ponder or direct RPC polling: | Deploy contracts | ~2 min | Funded deployer wallet | | Verify on Basescan | ~5 min | Deployment complete | | Fund LiquidityManager | ~1 min | Deployment complete | -| Set recenter access | ~1 min | feeDestination set (multisig) | | Wait for TWAP history | ~5-10 min | Pool initialized | -| First recenter | ~1 min | TWAP history + recenter access | +| First recenter | ~1 min | TWAP history accumulated | | Deploy txnBot | ~5 min | Addresses configured | | Deploy Ponder | ~10 min | Addresses + kraiken-lib updated | | Deploy frontend | ~5 min | Ponder running | diff --git a/onchain/analysis/SECURITY_REVIEW.md b/onchain/analysis/SECURITY_REVIEW.md index 83ac851..1fa13db 100644 --- a/onchain/analysis/SECURITY_REVIEW.md +++ b/onchain/analysis/SECURITY_REVIEW.md @@ -47,36 +47,36 @@ averageTick = int24(tickCumulativeDiff / int56(int32(fallbackInterval))); --- -### M-2: One-Time Setters Lack Access Control (Deployment Race) +### M-2: One-Time Setters Lack Access Control (Deployment Race) *(Partially addressed)* -**Severity:** Medium +**Severity:** ~~Medium~~ → **Low (partially resolved)** **Files:** -- `src/LiquidityManager.sol:102-106` — `setFeeDestination()` -- `src/Kraiken.sol:64-68` — `setLiquidityManager()` -- `src/Kraiken.sol:76-80` — `setStakingPool()` +- `src/LiquidityManager.sol:123-136` — `setFeeDestination()` — **resolved** +- `src/Kraiken.sol:64-68` — `setLiquidityManager()` — open +- `src/Kraiken.sol:76-80` — `setStakingPool()` — open -**Issue:** These set-once functions have no `msg.sender` restriction. Anyone can call them before the deployer: +**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(); - if (feeDestination != address(0)) revert AddressAlreadySet(); - feeDestination = feeDestination_; // first caller wins + require( + !feeDestinationLocked && (feeDestination == address(0) || feeDestination.code.length == 0), + "fee destination locked" + ); + feeDestination = feeDestination_; + if (feeDestination_.code.length > 0) { + feeDestinationLocked = true; + } } ``` -**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 +**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. -**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 +**Recommendation:** No action required if deployment uses bundled transactions. Optionally restrict `setLiquidityManager` / `setStakingPool` to a constructor-set deployer address for defence in depth. --- @@ -135,22 +135,22 @@ uint256 stakedPct = percentageStaked * 100 / 1e18; // truncates, doesn't round --- -### I-1: Missing `Recentered` Event +### I-1: Missing `Recentered` Event *(Addressed)* -**Severity:** Informational -**File:** `src/LiquidityManager.sol:121` +**Severity:** ~~Informational~~ → **Resolved** +**File:** `src/LiquidityManager.sol:66, 224` -`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. +`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:146-158` +**File:** `src/LiquidityManager.sol:177-191` -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) +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 @@ -211,6 +211,6 @@ The `uniswapV3MintCallback` is validated via `CallbackValidation.verifyCallback( 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) +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