Merge pull request 'fix: SECURITY_REVIEW.md references obsolete recenterAccess pattern (#838)' (#880) from fix/issue-838 into master
This commit is contained in:
johba
commit
8733d17062
2 changed files with 53 additions and 80 deletions
|
|
@ -100,22 +100,13 @@ cast send $LIQUIDITY_MANAGER --value 10ether \
|
|||
--mnemonic "$(cat .secret)"
|
||||
```
|
||||
|
||||
### 3.2 Set Recenter Access
|
||||
### 3.2 Trigger First Recenter
|
||||
|
||||
Restrict `recenter()` to the txnBot address:
|
||||
|
||||
```bash
|
||||
# Must be called by feeDestination (multisig)
|
||||
cast send $LIQUIDITY_MANAGER "setRecenterAccess(address)" $TXNBOT_ADDRESS \
|
||||
--rpc-url $BASE_RPC \
|
||||
--mnemonic "$(cat .secret)" # or via multisig
|
||||
```
|
||||
|
||||
### 3.3 Trigger First Recenter
|
||||
`recenter()` is permissionless — any address may call it. The 60-second cooldown (`MIN_RECENTER_INTERVAL`) and TWAP oracle check are always enforced.
|
||||
|
||||
```bash
|
||||
# Wait for pool to accumulate some TWAP history (~5 minutes of trades)
|
||||
# Then trigger first recenter (must be called by recenterAccess)
|
||||
# Anyone can trigger the first recenter; txnBot will take over ongoing calls
|
||||
cast send $LIQUIDITY_MANAGER "recenter()" \
|
||||
--rpc-url $BASE_RPC \
|
||||
--from $TXNBOT_ADDRESS
|
||||
|
|
@ -187,7 +178,7 @@ cast call $KRAIKEN "peripheryContracts()" --rpc-url $BASE_RPC # LM + Stake addr
|
|||
|
||||
# 2. LiquidityManager
|
||||
cast call $LM "feeDestination()" --rpc-url $BASE_RPC # Should be multisig
|
||||
cast call $LM "recenterAccess()" --rpc-url $BASE_RPC # Should be txnBot
|
||||
cast call $LM "lastRecenterTime()" --rpc-url $BASE_RPC # Should be non-zero after first recenter
|
||||
cast call $LM "positions(0)" --rpc-url $BASE_RPC # Floor position (after recenter)
|
||||
cast call $LM "positions(1)" --rpc-url $BASE_RPC # Anchor position
|
||||
cast call $LM "positions(2)" --rpc-url $BASE_RPC # Discovery position
|
||||
|
|
@ -212,17 +203,9 @@ cast balance $LM --rpc-url $BASE_RPC # Should show funded
|
|||
|
||||
### 6.1 Pause Recentering
|
||||
|
||||
**WARNING:** `revokeRecenterAccess()` does NOT pause recentering. It makes `recenter()` permissionless (anyone can call it with 60-second cooldown + TWAP check). In an attack scenario, this would make things worse.
|
||||
**NOTE:** `recenter()` is permissionless — there is no access-control switch to block it. The only mechanism that prevents a recenter is the 60-second `MIN_RECENTER_INTERVAL` cooldown and the TWAP oracle check. There is no admin function to revoke or grant access.
|
||||
|
||||
To truly lock out recenters, set `recenterAccess` to a burn address that no one controls:
|
||||
|
||||
```bash
|
||||
# Called by feeDestination (multisig) — sets access to a dead address
|
||||
cast send $LM "setRecenterAccess(address)" 0x000000000000000000000000000000000000dEaD \
|
||||
--rpc-url $BASE_RPC
|
||||
```
|
||||
|
||||
This leaves existing positions in place but prevents any new recenters. LP positions continue earning fees. To resume, call `setRecenterAccess()` with the txnBot address again.
|
||||
In an attack scenario the most effective response is to upgrade or replace the contract (see §6.3 / §6.4). Existing positions remain in place and continue earning fees regardless of recenter activity.
|
||||
|
||||
### 6.2 Upgrade Optimizer to Safe Defaults
|
||||
|
||||
|
|
@ -252,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** set recenter access to burn address (`0xdEaD`) — do NOT use `revokeRecenterAccess()` as it makes recenter permissionless
|
||||
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
|
||||
|
|
@ -297,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 |
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@ averageTick = int24(tickCumulativeDiff / int56(int32(PRICE_STABILITY_INTERVAL)))
|
|||
|
||||
**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.
|
||||
**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.
|
||||
|
||||
|
|
@ -47,66 +47,59 @@ 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.
|
||||
|
||||
---
|
||||
|
||||
### M-3: Open `recenter()` Access Without Rate Limiting
|
||||
### M-3: Open `recenter()` Access Without Rate Limiting *(Addressed)*
|
||||
|
||||
**Severity:** Medium (griefing)
|
||||
**File:** `src/LiquidityManager.sol:121-129`
|
||||
**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) {
|
||||
if (recenterAccess != address(0)) {
|
||||
require(msg.sender == recenterAccess, "access denied");
|
||||
} else {
|
||||
require(_isPriceStable(currentTick), "price deviated from oracle");
|
||||
}
|
||||
(, 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;
|
||||
```
|
||||
|
||||
**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).
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -142,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
|
||||
|
||||
|
|
@ -178,10 +171,8 @@ When `outstandingSupply` reaches 0 after subtracting `pulledKraiken` and `discov
|
|||
|
||||
| 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 |
|
||||
| `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 |
|
||||
|
|
@ -220,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)
|
||||
3. **Mitigate M-3** by setting `recenterAccess` to txnBot address immediately after deployment
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue