fix: address review findings — CREATE2 guard, transition test, docs
- LiquidityManager.setFeeDestination: add CREATE2 bypass guard — also blocks re-assignment when the current feeDestination has since acquired bytecode (was a plain address when set, contract deployed to it later) - LiquidityManager.setFeeDestination: expand NatSpec to document the EOA-mutability trade-off and the CREATE2 guard explicitly - Test: add testSetFeeDestinationEOAToContract_Locks covering the realistic EOA→contract transition (the primary lock-activation path) - red-team.sh: add comment that DEPLOYER_PK is Anvil account-0 and must only be used against a local ephemeral Anvil instance - ARCHITECTURE.md: document feeDestination conditional-lock semantics and contrast with Kraiken's strictly set-once liquidityManager/stakingPool Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
9ff96ff137
commit
b902b89e3b
4 changed files with 35 additions and 4 deletions
|
|
@ -34,6 +34,7 @@ Optimizer.sol (UUPS Upgradeable Proxy)
|
||||||
- **Optimizer → Stake**: reads sentiment data (% staked, avg tax rate)
|
- **Optimizer → Stake**: reads sentiment data (% staked, avg tax rate)
|
||||||
- **Optimizer upgrades**: UUPS proxy, admin-only `_authorizeUpgrade()`
|
- **Optimizer upgrades**: UUPS proxy, admin-only `_authorizeUpgrade()`
|
||||||
- **feeDestination receives both WETH and KRK fees**: during `recenter()`, Uniswap V3 fee collection produces both tokens. WETH fees AND KRK fees are forwarded to `feeDestination` (see `LiquidityManager._scrapePositions()`).
|
- **feeDestination receives both WETH and KRK fees**: during `recenter()`, Uniswap V3 fee collection produces both tokens. WETH fees AND KRK fees are forwarded to `feeDestination` (see `LiquidityManager._scrapePositions()`).
|
||||||
|
- **feeDestination is a conditional-lock (not set-once)**: `setFeeDestination()` (deployer-only) allows repeated changes while the destination is an EOA, enabling staged deployment and testing. The moment a contract address is set, `feeDestinationLocked` is set to `true` and no further changes are allowed. A CREATE2 guard also blocks re-assignment if the current destination has since acquired bytecode. This differs from Kraiken's `liquidityManager`/`stakingPool` which are strictly set-once.
|
||||||
- **feeDestination KRK excluded from outstanding supply**: `_getOutstandingSupply()` subtracts `kraiken.balanceOf(feeDestination)` before computing scarcity, because protocol-held KRK cannot be sold into the floor and should not inflate the supply count.
|
- **feeDestination KRK excluded from outstanding supply**: `_getOutstandingSupply()` subtracts `kraiken.balanceOf(feeDestination)` before computing scarcity, because protocol-held KRK cannot be sold into the floor and should not inflate the supply count.
|
||||||
- **Staking pool KRK excluded from outstanding supply**: `_getOutstandingSupply()` also subtracts `kraiken.balanceOf(stakingPoolAddr)`, because staked KRK is locked and similarly cannot be sold into the floor.
|
- **Staking pool KRK excluded from outstanding supply**: `_getOutstandingSupply()` also subtracts `kraiken.balanceOf(stakingPoolAddr)`, because staked KRK is locked and similarly cannot be sold into the floor.
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -114,13 +114,24 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
|
||||||
if (amount1Owed > 0) IERC20(poolKey.token1).safeTransfer(msg.sender, amount1Owed);
|
if (amount1Owed > 0) IERC20(poolKey.token1).safeTransfer(msg.sender, amount1Owed);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// @notice Sets the fee destination address (deployer only)
|
/// @notice Sets the fee destination address (deployer only).
|
||||||
/// @dev Any EOA can be set repeatedly. Once a contract address is set, locks permanently.
|
/// @dev Conditional trapdoor: EOA addresses can be set repeatedly (allows setup/migration),
|
||||||
|
/// but once a contract address is assigned, feeDestinationLocked is set and no further
|
||||||
|
/// changes are allowed. Note: the deployer retains fee-redirect ability while feeDestination
|
||||||
|
/// is an EOA; this is an intentional trade-off that enables staged deployment.
|
||||||
|
///
|
||||||
|
/// CREATE2 guard: also blocks re-assignment if the current feeDestination has since
|
||||||
|
/// acquired bytecode (i.e. was a pre-computed EOA when set, later deployed to).
|
||||||
/// @param feeDestination_ The address that will receive trading fees
|
/// @param feeDestination_ The address that will receive trading fees
|
||||||
function setFeeDestination(address feeDestination_) external {
|
function setFeeDestination(address feeDestination_) external {
|
||||||
require(msg.sender == deployer, "only deployer");
|
require(msg.sender == deployer, "only deployer");
|
||||||
if (address(0) == feeDestination_) revert ZeroAddressInSetter();
|
if (address(0) == feeDestination_) revert ZeroAddressInSetter();
|
||||||
require(!feeDestinationLocked, "fee destination locked");
|
// Block if explicitly locked OR if the current destination has since become a contract
|
||||||
|
// (guards CREATE2 bypass: address looked like an EOA when set, bytecode deployed later)
|
||||||
|
require(
|
||||||
|
!feeDestinationLocked && (feeDestination == address(0) || feeDestination.code.length == 0),
|
||||||
|
"fee destination locked"
|
||||||
|
);
|
||||||
feeDestination = feeDestination_;
|
feeDestination = feeDestination_;
|
||||||
if (feeDestination_.code.length > 0) {
|
if (feeDestination_.code.length > 0) {
|
||||||
feeDestinationLocked = true;
|
feeDestinationLocked = true;
|
||||||
|
|
|
||||||
|
|
@ -1028,7 +1028,7 @@ contract LiquidityManagerTest is UniSwapHelper {
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @notice Setting fee destination to a contract locks it permanently
|
* @notice Setting fee destination to a contract locks it permanently (direct path)
|
||||||
*/
|
*/
|
||||||
function testSetFeeDestinationContract_Locks() public {
|
function testSetFeeDestinationContract_Locks() public {
|
||||||
LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer));
|
LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer));
|
||||||
|
|
@ -1038,6 +1038,23 @@ contract LiquidityManagerTest is UniSwapHelper {
|
||||||
assertEq(freshLm.feeDestination(), address(harberg));
|
assertEq(freshLm.feeDestination(), address(harberg));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @notice Realistic deployment path: set EOA first, then upgrade to a contract — lock triggers on transition
|
||||||
|
*/
|
||||||
|
function testSetFeeDestinationEOAToContract_Locks() public {
|
||||||
|
LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer));
|
||||||
|
// Step 1: set to an EOA during setup — allowed, not locked
|
||||||
|
freshLm.setFeeDestination(makeAddr("treasuryEOA"));
|
||||||
|
assertFalse(freshLm.feeDestinationLocked(), "not locked after EOA set");
|
||||||
|
// Step 2: upgrade to treasury contract once it is deployed — locks permanently
|
||||||
|
freshLm.setFeeDestination(address(harberg));
|
||||||
|
assertTrue(freshLm.feeDestinationLocked(), "locked after contract set");
|
||||||
|
assertEq(freshLm.feeDestination(), address(harberg));
|
||||||
|
// Step 3: any further attempt reverts
|
||||||
|
vm.expectRevert("fee destination locked");
|
||||||
|
freshLm.setFeeDestination(makeAddr("attacker"));
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @notice Once locked, setFeeDestination reverts
|
* @notice Once locked, setFeeDestination reverts
|
||||||
*/
|
*/
|
||||||
|
|
|
||||||
|
|
@ -133,6 +133,8 @@ log " recenterAccess granted"
|
||||||
# ── 3b. Set feeDestination to LM itself (fees accrue as liquidity) ─────────────
|
# ── 3b. Set feeDestination to LM itself (fees accrue as liquidity) ─────────────
|
||||||
# setFeeDestination allows repeated EOA sets; setting to a contract locks it permanently.
|
# setFeeDestination allows repeated EOA sets; setting to a contract locks it permanently.
|
||||||
# The deployer (Anvil account 0) deployed LiquidityManager and may call setFeeDestination again.
|
# The deployer (Anvil account 0) deployed LiquidityManager and may call setFeeDestination again.
|
||||||
|
# DEPLOYER_PK is Anvil's deterministic account-0 key — valid ONLY against a local ephemeral
|
||||||
|
# Anvil instance. Never run this script against a non-ephemeral or shared-state chain.
|
||||||
DEPLOYER_PK=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
|
DEPLOYER_PK=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
|
||||||
log "Setting feeDestination to LM ($LM) ..."
|
log "Setting feeDestination to LM ($LM) ..."
|
||||||
"$CAST" send --rpc-url "$RPC_URL" --private-key "$DEPLOYER_PK" \
|
"$CAST" send --rpc-url "$RPC_URL" --private-key "$DEPLOYER_PK" \
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue