fix: SECURITY_REVIEW.md references obsolete recenterAccess pattern (#838)

- Update M-3 finding: recenterAccess was removed; MIN_RECENTER_INTERVAL
  (60s) cooldown now enforced unconditionally — downgrade severity to
  Informational (resolved)
- Update Access Control Summary: remove recenterAccess rows, reflect
  permissionless recenter() with cooldown
- Update Conclusion: mark M-3 as resolved
- Fix stale M-1 impact note that mentioned recenterAccess as a workaround
- deployment.md: remove Section 3.2 "Set Recenter Access" (setRecenterAccess
  no longer exists); update 3.3 first-recenter comment
- deployment.md: replace recenterAccess() verification call with
  lastRecenterTime() check
- deployment.md §6.1: rewrite Pause Recentering note — no access-control
  switch exists, cooldown is the only rate limiter
- deployment.md §6.5: remove stale setRecenterAccess(0xdEaD) instruction

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
openhands 2026-03-16 14:43:37 +00:00
parent ed15d23746
commit 9b75817300
2 changed files with 25 additions and 51 deletions

View file

@ -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** assess severity — `recenter()` is permissionless (no access-control switch exists); 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

View file

@ -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.
@ -80,33 +80,26 @@ function setFeeDestination(address feeDestination_) external {
---
### 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.
---
@ -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 |
@ -221,5 +212,5 @@ The protocol is ready for mainnet deployment with the following pre-launch actio
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
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