Skip to content

PROTOCOL_KNOWLEDGE.md

Latest commit

 

History

History
324 lines (227 loc) · 12.5 KB

PROTOCOL_KNOWLEDGE.md

File metadata and controls

324 lines (227 loc) · 12.5 KB

Protocol Knowledge Base

This file contains institutional knowledge about protocol integrations, non-obvious behaviors, and lessons learned from development. AI agents should reference this when writing or reviewing protocol code, and should add new entries when discovering nuances.

How to Use This File

When writing protocol code: Review relevant sections before implementation. When fixing bugs: If the fix involves a non-obvious protocol behavior, add an entry. When reviewing code: Check if the code respects known nuances in this file.

Uniswap V4

Pool Manager Sync Before Settle

The Issue: Pool manager must be synced before settling currencies to ensure accurate balance tracking and prevent DoS attacks.

Wrong:

// For native ETH - missing sync
poolManager.settle{value: amount}();

// For ERC20 - missing sync
currency.transfer(address(poolManager), amount);
poolManager.settle();

Correct:

// For native ETH - sync first
poolManager.sync(currency);
poolManager.settle{value: amount}();

// For ERC20 - sync first
poolManager.sync(currency);
currency.transfer(address(poolManager), amount);
poolManager.settle();

Why: Without syncing, the pool manager's internal accounting may not reflect the actual balance, leading to settlement failures or DoS conditions. This is especially critical for native ETH where the value is transferred as part of the settle call. The sync() call updates the pool manager's cached balance to match the actual contract balance before settlement occurs.

Reference: Zora Audit Issue #9 - Unsynced ETH settlement can result in DoS

ModifyLiquidity Return Values

The Issue: modifyLiquidity() returns callerDelta that already includes accrued fees; feesAccrued is informational only.

The Interface:

/// @notice Modify the liquidity for the given pool
/// @dev Poke by calling with a zero liquidityDelta
/// @param key The pool to modify liquidity in
/// @param params The parameters for modifying the liquidity
/// @param hookData The data to pass through to the add/removeLiquidity hooks
/// @return callerDelta The balance delta of the caller of modifyLiquidity. This is the total of both principal delta and feesAccrued
/// @return feesAccrued The balance delta of the fees generated in the liquidity range. Returned for informational purposes
function modifyLiquidity(
  PoolKey memory key,
  IPoolManager.ModifyLiquidityParams memory params,
  bytes calldata hookData
) external returns (BalanceDelta callerDelta, BalanceDelta feesAccrued);

Wrong:

(BalanceDelta liquidityDelta, BalanceDelta feesAccrued) = poolManager.modifyLiquidity(...);
uint128 totalReceived = uint128(liquidityDelta.amount0() + feesAccrued.amount0()); // Double-counting!

Correct:

(BalanceDelta callerDelta, ) = poolManager.modifyLiquidity(...);
uint128 totalReceived = uint128(callerDelta.amount0()); // callerDelta already includes fees

Reference: Uniswap V4 IPoolManager Interface

Tick Boundary Semantics for Limit Order Crossing

The Issue: Order crossing checks must account for Uniswap V4's asymmetric tick boundary handling where currentTick == tickLower does not guarantee complete order execution.

Context: In Uniswap V4, liquidity at tick T is available for prices in the range [T, T+1) (right-exclusive). This means:

  • Upper tick boundaries: currentTick >= tickUpper reliably indicates crossing
  • Lower tick boundaries: currentTick == tickLower is AT the boundary but may not be fully crossed

Wrong:

// Inconsistent boundary handling - can cause premature fills
bool crossed = order.isCurrency0
    ? currentTick >= order.tickUpper    // Correct for upper bounds
    : currentTick <= order.tickLower;   // WRONG: <= for lower bounds

Correct:

// Consistent strict comparison respects tick boundary semantics
bool crossed = order.isCurrency0
    ? currentTick >= order.tickUpper    // Upper bound: inclusive
    : currentTick < order.tickLower;    // Lower bound: strictly less

Why This Matters:

  • At currentTick == tickLower, the order sits exactly at the tick boundary but may not be fully executed due to Uniswap V4's internal tick handling
  • Using <= allows premature fills, causing users to receive less output tokens than intended and potentially some unexpected input tokens
  • This issue is amplified in pools with large tickSpacing where partial fills can leave significant amounts unfilled
  • The asymmetry exists because Uniswap V4's liquidity ranges are right-exclusive: [tickLower, tickUpper)

Impact on Limit Orders:

  • False positive crossing: Orders incorrectly identified as fillable when they haven't fully crossed
  • Blocked withdrawals: Users cannot withdraw orders that are legitimately at the boundary but unfilled
  • Economic loss: Premature fills result in worse execution prices for users

Reference: Uniswap V4 Pool.sol tick handling, Zora Audit Issue #12

Swaps May Only Partially Execute Due to Price Limits

The Issue: When swapping large amounts, swaps can hit the sqrtPriceLimit and only partially execute, leaving unsettled currency deltas. All positive deltas must be taken to avoid CurrencyNotSettled() errors.

Context: In Uniswap V4, swaps are bounded by price limits (sqrtPriceLimit). When a swap would move the price beyond this limit, only a portion of the input is consumed. The remaining input amount stays as an unsettled positive delta that must be explicitly taken from the pool manager.

Wrong:

// Only taking the final output currency - partial swap remainders left unsettled
(Currency receivedCurrency, uint128 receivedAmount) = executeSwapPath(poolManager, fees0, fees1, path);
if (receivedAmount > 0) {
    poolManager.take(receivedCurrency, address(this), receivedAmount);
    distributeRewards(receivedCurrency, receivedAmount);
}
// If swap was partial, some input currency remains as positive delta - causes CurrencyNotSettled()

Correct:

// Execute swap path, then check ALL currencies for positive deltas
executeSwapPath(poolManager, fees0, fees1, path);

// Check input currency for partial swap remainder
int256 deltaIn = TransientStateLibrary.currencyDelta(poolManager, address(this), currencyIn);
if (deltaIn > 0) {
    poolManager.take(currencyIn, address(this), uint128(uint256(deltaIn)));
    distributeRewards(currencyIn, uint128(uint256(deltaIn)));
}

// Check all intermediate currencies for positive deltas
for (uint256 i = 0; i < path.length; i++) {
    Currency intermediateCurrency = path[i].intermediateCurrency;
    int256 delta = TransientStateLibrary.currencyDelta(poolManager, address(this), intermediateCurrency);
    if (delta > 0) {
        poolManager.take(intermediateCurrency, address(this), uint128(uint256(delta)));
        distributeRewards(intermediateCurrency, uint128(uint256(delta)));
    }
}

Why This Matters:

  • Large swaps frequently hit price limits, especially in pools with concentrated liquidity
  • Partial execution leaves positive deltas that must be settled before the unlock ends
  • Example: Attempting to swap 466 quadrillion may only consume 5.3 trillion (1.1%), leaving 461 quadrillion unsettled
  • Unsettled deltas cause CurrencyNotSettled() reverts when the pool manager's unlock callback completes
  • This is particularly common in multi-hop swaps where intermediate pools have limited liquidity

Impact:

  • Transaction reverts: Swaps fail with CurrencyNotSettled() when large fee amounts can't be fully swapped
  • Lost rewards: Partial swap remainders not distributed to reward recipients
  • Hook reliability: Hooks that convert currencies must handle partial execution to avoid bricking pools

Reference: Linear MKT-107

Solidity Patterns

Hash Function Security with Dynamic Data

The Issue: abi.encodePacked can create hash collisions when concatenating dynamic-length data or multiple parameters without padding.

Wrong:

bytes32 id = keccak256(abi.encodePacked(poolKey, coin, tick, maker, nonce));

Correct:

bytes32 id = keccak256(abi.encode(poolKey, coin, tick, maker, nonce));

Why: abi.encodePacked concatenates values without padding, which can lead to ambiguous encodings. For example, abi.encodePacked("aa", "b") produces the same result as abi.encodePacked("a", "ab"). abi.encode provides proper 32-byte padding between parameters, eliminating this collision risk.

Performance Impact: abi.encode has slightly higher gas cost (~50-100 gas), but the security benefit far outweighs the minimal cost difference.

Reference: Solidity Documentation on ABI Encoding

Zora-Specific Patterns

Safe Typecasting from Uniswap V4 BalanceDelta

The Issue: Uniswap V4's BalanceDelta returns int128 values that need to be cast to unsigned types, but the casting must respect the sign semantics to avoid overflow vulnerabilities.

Wrong:

(BalanceDelta delta, ) = poolManager.modifyLiquidity(...);
int128 amount0 = delta.amount0();
uint128 payout = uint128(amount0); // Dangerous! Could overflow if amount0 is negative

Correct:

(BalanceDelta delta, ) = poolManager.modifyLiquidity(...);
int128 amount0 = delta.amount0();
uint128 payout;

if (amount0 > 0) {
    // Safe cast when value is known to be positive
    payout = uint128(amount0);
} else if (amount0 < 0) {
    // Safe cast when converting negative to positive amount owed
    payout = uint128(uint256(int256(-amount0)));
}

Why: BalanceDelta amounts can be negative (owed to pool) or positive (owed from pool). Direct casting from int128 to uint128 when the value is negative will cause an overflow. The double cast through int256 and uint256 when negating ensures the value fits within uint128 bounds.

Common Patterns:

  • Positive amounts from burning liquidity: uint128(positiveAmount)
  • Negative amounts when minting liquidity: uint128(uint256(int256(-negativeAmount)))
  • Settlement deltas: uint256(-negativeAmount) for amounts owed to pool

Reference: packages/limit-orders/src/libs/ - LimitOrderCreate.sol:238-244, LimitOrderLiquidity.sol:99-110, SwapLimitOrders.sol:134-137

Launch Fee Affects Test Fee Calculations

The Issue: Tests that swap immediately after coin deployment will have 99% fees (launch fee) instead of the expected 1% LP fee, causing reward calculations and fee assertions to fail.

Wrong:

function test_rewardDistribution() public {
  _deployV4Coin(currency);
  // Swap immediately - launch fee is 99%!
  router.execute(commands, inputs, deadline);
  // Assertions about 1% fee distribution will fail
}

Correct:

function test_rewardDistribution() public {
  _deployV4Coin(currency);
  // Skip past launch fee period (default 10 seconds)
  vm.warp(block.timestamp + 1 days);
  // Now swap with normal 1% LP fee
  router.execute(commands, inputs, deadline);
}

Test Isolation: When tests run in parallel or sequence, timestamp state can leak between tests. Add isolation annotation to affected test contracts or functions:

/// forge-config: default.isolate = true
contract MyTest is BaseTest {
  // Tests run in isolated EVM state
}

// Or on individual functions:
/// forge-config: default.isolate = true
function test_specificTest() public {
  // This test runs in isolated state
}

Why: The launch fee applies a 99% fee for the first 10 seconds after coin creation to deter sniping. Tests expecting normal LP fee behavior must either warp past this period or use test isolation to ensure consistent timestamp state.

Reference: packages/coins/src/hooks/ZoraV4CoinHook.sol, packages/coins/src/libs/CoinConstants.sol (LAUNCH_FEE_DURATION = 10 seconds)

(Add entries as discovered)

Entry Template

When adding new entries, use this format:

### Brief Title

**The Issue:** One-sentence description of the non-obvious behavior.

**Wrong:**

\`\`\`solidity
// Code that demonstrates the mistake
\`\`\`

**Correct:**

\`\`\`solidity
// Code that demonstrates the fix
\`\`\`

**Reference:** Link to documentation or source code (if applicable)