PancakeSwap Infinity Core Contracts Security Review Report

When a user wants to mint liquidity into the bin of a pool, they can choose the proportions of the amounts freely for the current active bin. Only if the ratio is different from the current ratio, the user would be paying composition fees.

Inside of the _updateBin function, the user's effective shares and amounts are calculated using getSharesAndEffectiveAmountsIn. This function takes the amount of effective liquidity into account as well, by recalculating from the shares back to liquidity and subtracting any surplus from the user's amounts. This alone would protect against rounded down shares and thus inflation attacks.

However, the shares get recalculated again in _updateBin when adding the composition fees. More specifically, the composition fees aren't taken from the amounts, but rather the amount of shares the user will receive. The amounts are not recalculated and so here there is no protection against potential rounding down. A small of amount of fees could turn 2 wei shares into 1 wei, halving the user's share of the bin's total reserves.

Furthermore, this edge case is not protected against in the pancake-v4-periphery/src/pool-bin/BinPositionManager.sol#L222-230 :

(BalanceDelta delta, BinPool.MintArrays memory mintArray) = binPoolManager.mint(
    params.poolKey,
    IBinPoolManager.MintParams({liquidityConfigs: liquidityConfigs, amountIn: amountIn, salt: bytes32(0)}),
    params.hookData
);
 
/// Slippage checks, similar to CL type. However, this is different from TJ, in PCS v4,
/// as hooks can impact delta (take extra token), user need to be protected with amountMax instead
delta.validateMaxIn(params.amount0Max, params.amount1Max);

Only the amount in is checked to not exceed the max, not the amount of shares that was minted.

Inflation of the reserves against a total supply of 1 wei is possible using the same method of rounding down the shares due to composition fees in a loop. The amount would exponentially increase and the reverse can be doubled each time this way.

As a result, the victim user would be paying the full amount for only half the shares. The rest of the value would be distributed to the attacker that holds the first shares and after burning that, they would've stolen half of the assets of the victim user.

function _updateBin(State storage self, MintParams memory params, uint24 id, bytes32 maxAmountsInToBin)
    internal
    returns (
        uint256 shares,
        bytes32 amountsIn,
        bytes32 amountsInToBin,
        bytes32 feeAmountToProtocol,
        bytes32 compositionFeeAmount
    )
{
    BinSlot0 slot0Cache = self.slot0;
    uint24 activeId = slot0Cache.activeId();
    bytes32 binReserves = self.reserveOfBin[id];
 
    uint256 price = id.getPriceFromId(params.binStep);
    uint256 supply = self.shareOfBin[id];
 
    (shares, amountsIn) = binReserves.getSharesAndEffectiveAmountsIn(maxAmountsInToBin, price, supply);
    amountsInToBin = amountsIn;
 
    if (id == activeId) {
        // Fees happens when user try to add liquidity in active bin but with different ratio of (x, y)
        /// eg. current bin is 40/60 (a,b) but user tries to add liquidity with 50/50 ratio
        uint24 lpFee = params.lpFeeOverride.isOverride()
            ? params.lpFeeOverride.removeOverrideAndValidate(LPFeeLibrary.TEN_PERCENT_FEE)
            : slot0Cache.lpFee();
 
        bytes32 feesAmount;
        (feesAmount, feeAmountToProtocol) =
            binReserves.getCompositionFeesAmount(slot0Cache.protocolFee(), lpFee, amountsIn, supply, shares);
        compositionFeeAmount = feesAmount;
 
        if (feesAmount != 0) {
            {
                uint256 userLiquidity = amountsIn.sub(feesAmount).getLiquidity(price);
                /// @dev Ensure fee accrued only to existing lp, before calculating new share for minter
                uint256 binLiquidity = binReserves.add(feesAmount.sub(feeAmountToProtocol)).getLiquidity(price);
                shares = userLiquidity.mulDivRoundDown(supply, binLiquidity);
            }
 
            if (feeAmountToProtocol != 0) {
                amountsInToBin = amountsInToBin.sub(feeAmountToProtocol);
            }
        }
    } else {
        amountsIn.verifyAmounts(activeId, id);
    }
 
    if (shares == 0 || amountsInToBin == 0) revert BinPool__ZeroShares(id);
    if (supply == 0) _addBinIdToTree(self, id);
 
    self.reserveOfBin[id] = binReserves.add(amountsInToBin);
}

We recommend to enforce a minimum amount of total supply either after the first mint or during creation, such that this rate cannot be manipulated.

On certain EVM-based chains, the native asset may have an ERC20-like interface and a designated contract address. For example, in the CELO chain, the native asset can be transferred either through msg.value or through the ERC20-like methods of the GoldToken contract, as documented here: Celo for Ethereum Developers | Celo Documentation This dual transfer method can create ambiguity and lead to double spending of native asset, if PancakeSwap protocol is deployed on such chains.

An attack exploiting this vulnerability could unfold as follows:

1. The attacker possesses an amount X of native assets, transferable either through msg.value or the ERC20-like contract at address Addr.
2. The attacker initiates Vault.sync() for Addr. There is no need to call sync() for the second time when the native asset is transferred through msg.value, or currency = address(0).
3. The attacker performs two settle operations: Vault.settle(0) with msg.value, and subsequent Vault.settle(Addr).
4. By executing two take operations, the attacker can retrieve back 2X amount of native asset: Vault.take(Addr) and Vault.take(0).

/// @inheritdoc IVault
function sync(Currency currency) public returns (uint256 balance) {
    balance = currency.balanceOfSelf();
    currency.setVaultReserves(balance);
}
 
function settle(Currency currency) external payable override isLocked returns (uint256 paid) {
    if (!currency.isNative()) {
        if (msg.value > 0) revert SettleNonNativeCurrencyWithValue();
        uint256 reservesBefore = currency.getVaultReserves();
        uint256 reservesNow = sync(currency);
        paid = reservesNow - reservesBefore;
    } else {
        paid = msg.value;
    }
 
    SettlementGuard.accountDelta(msg.sender, currency, paid.toInt128());
}

Cache the currency parameter within the Vault.sync() function to use it later in Vault.settle(), ensuring that users cannot specify the currency for Vault.settle().

If no currency is cached, it indicates that Vault.sync() has not been invoked, and the native asset (address(0)) is transferred via msg.value.

PancakeSwap Infinity heavily depends on the TSTORE and TLOAD opcodes to implement critical features such as reentrancy protection, flash accounting, and slippage control. The issue arises because zkSync Era currently lacks support for these opcodes, as confirmed by rollup.codes. While zkSync has indicated plans to add support in the future, it's not available at this time.

Without support for these opcodes, any contract execution in PancakeSwap Infinity that relies on them will fail, effectively making the protocol incompatible with zkSync Era for the moment.

function setLocker(address newLocker) internal {
    address currentLocker = getLocker();
 
    // either set from non-zero to zero (set) or from zero to non-zero (reset)
    if (currentLocker != address(0) && newLocker != address(0)) revert IVault.LockerAlreadySet(currentLocker);
 
    assembly ("memory-safe") {
        tstore(LOCKER_SLOT, and(newLocker, 0xffffffffffffffffffffffffffffffffffffffff))
    }
}
 
function getLocker() internal view returns (address locker) {
    assembly ("memory-safe") {
        locker := tload(LOCKER_SLOT)
    }
}
 
function getUnsettledDeltasCount() internal view returns (uint256 count) {
    assembly ("memory-safe") {
        count := tload(UNSETTLED_DELTAS_COUNT)
    }
}
 
function getCurrencyDelta(address settler, Currency currency) internal view returns (int256 delta) {
    uint256 elementSlot = uint256(keccak256(abi.encode(settler, currency, CURRENCY_DELTA)));
    assembly ("memory-safe") {
        delta := tload(elementSlot)
    }
}

function accountDelta(address settler, Currency currency, int256 newlyAddedDelta) internal {
    if (newlyAddedDelta == 0) return;
 
    /// @dev update the count of non-zero deltas if necessary
    int256 currentDelta = getCurrencyDelta(settler, currency);
    int256 nextDelta = currentDelta + newlyAddedDelta;
    unchecked {
        if (nextDelta == 0) {
            assembly ("memory-safe") {
                tstore(UNSETTLED_DELTAS_COUNT, sub(tload(UNSETTLED_DELTAS_COUNT), 1))
            }
        } else if (currentDelta == 0) {
            assembly ("memory-safe") {
                tstore(UNSETTLED_DELTAS_COUNT, add(tload(UNSETTLED_DELTAS_COUNT), 1))
            }
        }
    }
 
    /// @dev ref: https://docs.soliditylang.org/en/v0.8.24/internals/layout_in_storage.html#mappings-and-dynamic-arrays
    /// simulating mapping index but with a single hash
    /// save one keccak256 hash compared to built-in nested mapping
    uint256 elementSlot = uint256(keccak256(abi.encode(settler, currency, CURRENCY_DELTA)));
    assembly ("memory-safe") {
        tstore(elementSlot, nextDelta)
    }
}

We recommend implementing a version of SettlementGuard.sol and VaultReserve.sol that uses normal storage instead of transient storage, which can then be used on chains that do not supports transient storage.

Commentary from the client:

We'll wait for zkSync to support these 2 opcode before deploying.

The current implementation in getSharesAndEffectiveAmountsIn doesn't prevent bin liquidity from exceeding the maximum Liquidity limit when new liquidity or swap fees are added. It only checks the bin's liquidity and incoming amount before adding them, allowing potential overflow due to swap fee accumulation or additional liquidity.

The same issue also applies to:

• BinPool::donate() line 352
• BinPool::swap() line 171, if amountsIn + fees exceeds amountsOutOfBin
• BinHelper::getAmountsIn()
• BinHelper::getAmountsOut()

function getSharesAndEffectiveAmountsIn(bytes32 binReserves, bytes32 amountsIn, uint256 price, uint256 totalSupply)
    internal
    pure
    returns (uint256 shares, bytes32 effectiveAmountsIn)
{
    (uint256 x, uint256 y) = amountsIn.decode();
 
    uint256 userLiquidity = getLiquidity(x, y, price);
    if (userLiquidity == 0) return (0, 0);
 
    uint256 binLiquidity = getLiquidity(binReserves, price);
    if (binLiquidity == 0 || totalSupply == 0) return (userLiquidity.sqrt(), amountsIn);
 
    shares = userLiquidity.mulDivRoundDown(totalSupply, binLiquidity);
    uint256 effectiveLiquidity = shares.mulDivRoundUp(binLiquidity, totalSupply);
 
    if (userLiquidity > effectiveLiquidity) {
        uint256 deltaLiquidity = userLiquidity - effectiveLiquidity;
 
        // The other way might be more efficient, but as y is the quote asset, it is more valuable
        if (deltaLiquidity >= Constants.SCALE) {
            uint256 deltaY = deltaLiquidity >> Constants.SCALE_OFFSET;
            deltaY = deltaY > y ? y : deltaY;
 
            y -= deltaY;
            deltaLiquidity -= deltaY << Constants.SCALE_OFFSET;
        }
 
        if (deltaLiquidity >= price) {
            uint256 deltaX = deltaLiquidity / price;
            deltaX = deltaX > x ? x : deltaX;
 
            x -= deltaX;
        }
 
        amountsIn = uint128(x).encode(uint128(y));
    }
 
    return (shares, amountsIn);
}

// BinPool.sol
 
function swap(State storage self, SwapParams memory params)
    internal
    returns (BalanceDelta result, SwapState memory swapState)
{
    -- SNIP --
    self.reserveOfBin[swapState.activeId] = binReserves.add(amountsInWithFees).sub(amountsOutOfBin); // @audit: same issue
    ...
}
 
function donate(State storage self, uint16 binStep, uint128 amount0, uint128 amount1)
    internal
    returns (BalanceDelta result, uint24 activeId)
{
    -- SNIP --
    self.reserveOfBin[activeId] = binReserves.add(amountIn); // @audit: same issue
    ...
}

Consider adding a limit to ensure that bin liquidity can't overflow.

// (2^256 - 1) / (2 * log(2**128) / log(1.0001))
++ uint256 internal constant MAX_LIQUIDITY_PER_BIN =
    65251743116719673010965625540244653191619923014385985379600384103134737;
 
function getSharesAndEffectiveAmountsIn(bytes32 binReserves, bytes32 amountsIn, uint256 price, uint256 totalSupply)
    internal
    pure
    returns (uint256 shares, bytes32 effectiveAmountsIn)
{
    (uint256 x, uint256 y) = amountsIn.decode();
 
    uint256 userLiquidity = getLiquidity(x, y, price);
    if (userLiquidity == 0) return (0, 0);
 
    uint256 binLiquidity = getLiquidity(binReserves, price);
    if (binLiquidity == 0 || totalSupply == 0) return (userLiquidity.sqrt(), amountsIn);
 
    shares = userLiquidity.mulDivRoundDown(totalSupply, binLiquidity);
    uint256 effectiveLiquidity = shares.mulDivRoundUp(binLiquidity, totalSupply);
 
    if (userLiquidity > effectiveLiquidity) {
        uint256 deltaLiquidity = userLiquidity - effectiveLiquidity;
 
        // The other way might be more efficient, but as y is the quote asset, it is more valuable
        if (deltaLiquidity >= Constants.SCALE) {
            uint256 deltaY = deltaLiquidity >> Constants.SCALE_OFFSET;
            deltaY = deltaY > y ? y : deltaY;
 
            y -= deltaY;
            deltaLiquidity -= deltaY << Constants.SCALE_OFFSET;
        }
 
        if (deltaLiquidity >= price) {
            uint256 deltaX = deltaLiquidity / price;
            deltaX = deltaX > x ? x : deltaX;
 
            x -= deltaX;
        }
 
        amountsIn = uint128(x).encode(uint128(y));
    }
 
++ if (getLiquidity(binReserves.add(amountsIn), price) > Constants.MAX_LIQUIDITY_PER_BIN) {
++     revert BinHelper__MaxLiquidityPerBinExceeded();
++ }
 
    return (shares, amountsIn);
}

BinHelper::getAmountsIn() and BinHelper::getAmountsOut() - Note: see TraderJoe V2's fix for their fn BinHelper::getAmounts():

function getAmountsOut(
    bytes32 binReserves,
    uint24 fee,
    uint16 binStep,
    bool swapForY, // swap `swapForY` and `activeId` to avoid stack too deep
    uint24 activeId,
    bytes32 amountsInLeft
) internal pure returns (bytes32 amountsInWithFees, bytes32 amountsOutOfBin, bytes32 totalFees) {
 
    -- SNIP --
    (amountsInWithFees, amountsOutOfBin, totalFees) = swapForY
        ? (amountIn128.encodeFirst(), amountOut128.encodeSecond(), fee128.encodeFirst())
        : (amountIn128.encodeSecond(), amountOut128.encodeFirst(), fee128.encodeSecond());
 
++ if (
++     getLiquidity(binReserves.add(amountsInWithFees).sub(amountsOutOfBin), price)
++     > Constants.MAX_LIQUIDITY_PER_BIN
++ ) {
++     revert BinHelper__MaxLiquidityPerBinExceeded();
++ }
 
function getAmountsIn(
    bytes32 binReserves,
    uint24 fee,
    uint16 binStep,
    bool swapForY,
    uint24 activeId,
    bytes32 amountsOutLeft
) internal pure returns (bytes32 amountsInWithFees, bytes32 amountsOutOfBin, bytes32 totalFees) {
 
    -- SNIP --
    (amountsInWithFees, amountsOutOfBin, totalFees) = swapForY
        ? (amountIn.encodeFirst(), amountOutOfBin.encodeSecond(), feeAmount.encodeFirst())
        : (amountIn.encodeSecond(), amountOutOfBin.encodeFirst(), feeAmount.encodeSecond());
 
++ if (
++     getLiquidity(binReserves.add(amountsInWithFees).sub(amountsOutOfBin), price)
++     > Constants.MAX_LIQUIDITY_PER_BIN
++ ) {
++     revert BinHelper__MaxLiquidityPerBinExceeded();
++ }

The _fetchProtocolFee() function in ProtocolFees incorporates gas-griefing protection by verifying if the size of the returned data exceeds 32 bytes following the external call to protocolFeeController:

(bool _success, bytes memory _data) =
    address(protocolFeeController).call{gas: controllerGasLimit}(
        abi.encodeCall(IProtocolFeeController.protocolFeeForPool, (key))
    );
// Ensure that the return data fits within a word
if (!_success || _data.length > 32) return (false, 0);

However, this protective measure doesn't function as intended and fails to prevent gas-griefing attacks when protocolFeeController returns a substantial amount of data. Since the Solidity code snippet provided copies the returned data into memory first. Once the data is in memory, it then checks if the length exceeds 32 bytes.

function _fetchProtocolFee(PoolKey memory key) internal returns (bool success, uint24 protocolFee) {
    if (address(protocolFeeController) != address(0)) {
        // note that EIP-150 mandates that calls requesting more than 63/64ths of remaining gas
        // will be allotted no more than this amount, so controllerGasLimit must be set with this
        // in mind.
        if (gasleft() < controllerGasLimit) revert ProtocolFeeCannotBeFetched();
 
        (bool _success, bytes memory _data) =
            address(protocolFeeController).call{gas: controllerGasLimit}(
                abi.encodeCall(IProtocolFeeController.protocolFeeForPool, (key))
            );
        // Ensure that the return data fits within a word
        if (!_success || _data.length > 32) return (false, 0);
 
        uint256 returnData;
        assembly ("memory-safe") {
            returnData := mload(add(_data, 0x20))
        }
 
        // Ensure return data does not overflow a uint24 and that the underlying fees are within bounds.
        (success, protocolFee) = (returnData == uint24(returnData)) && uint24(returnData).validate()
            ? (true, uint24(returnData))
            : (false, 0);
    }
}

To address this, the code within the _fetchProtocolFee() function should verify that the returndatasize does not surpass 32 bytes after executing an external call. It should avoid copying data into memory if the limit is exceeded.

In the swap() function of BinPoolManager, the beforeSwap hook has the ability to override the value of lpFee (lines 144 and 145).

(int128 amountToSwap, BeforeSwapDelta beforeSwapDelta, uint24 lpFeeOverride) =
    BinHooks.beforeSwap(key, swapForY, amountSpecified, hookData);

uint24 lpFee = params.lpFeeOverride.isOverride()
    ? params.lpFeeOverride.removeOverrideAndValidate(LPFeeLibrary.TEN_PERCENT_FEE)
    : slot0Cache.lpFee;

Later, the value of lpFeeOverride is utilized within the swap() function of BinPool (lines 126-128).

LP minting for the active Bin might entail implicit swapping if the liquidity is provided in a different ratio (lines 429-445 of BinPool).

// Fees happens when user try to add liquidity in active bin but with different ratio of (x, y)
/// eg. current bin is 40/60 (a,b) but user tries to add liquidity with 50/50 ratio
bytes32 fees;
(fees, feeForProtocol) =
    binReserves.getCompositionFees(slot0Cache.protocolFee, slot0Cache.lpFee, amountsIn, supply, shares);
compositionFee = fees;
if (fees != 0) {
    {
        uint256 userLiquidity = amountsIn.sub(fees).getLiquidity(price);
        uint256 binLiquidity = binReserves.getLiquidity(price);
        shares = userLiquidity.mulDivRoundDown(supply, binLiquidity);
    }
 
    if (feeForProtocol != 0) {
        amountsInToBin = amountsInToBin.sub(feeForProtocol);
    }
}

In this case, it's not possible to use overridden LP fees from a hook; it always defaults to using slot0Cache.lpFee instead.

function mint(PoolKey memory key, IBinPoolManager.MintParams calldata params, bytes calldata hookData)
    external
    override
    whenNotPaused
    returns (BalanceDelta delta, BinPool.MintArrays memory mintArray)
{
    PoolId id = key.toId();
    BinPool.State storage pool = pools[id];
    pool.checkPoolInitialized();
 
    BinHooks.beforeMint(key, params, hookData);
 
    bytes32 feeForProtocol;
    bytes32 compositionFee;
    (delta, feeForProtocol, mintArray, compositionFee) = pool.mint(
        BinPool.MintParams({
            to: msg.sender,
            liquidityConfigs: params.liquidityConfigs,
            amountIn: params.amountIn,
            binStep: key.parameters.getBinStep(),
            salt: params.salt
        })
    );
 
    unchecked {
        if (feeForProtocol > 0) {
            protocolFeesAccrued[key.currency0] += feeForProtocol.decodeX();
            protocolFeesAccrued[key.currency1] += feeForProtocol.decodeY();
        }
    }
 
    /// @notice Make sure the first event is noted, so that later events from afterHook won't get mixed up with this one
    emit Mint(id, msg.sender, mintArray.ids, params.salt, mintArray.amounts, compositionFee, feeForProtocol);
 
    BalanceDelta hookDelta;
    (delta, hookDelta) = BinHooks.afterMint(key, params, delta, hookData);
 
    if (hookDelta != BalanceDeltaLibrary.ZERO_DELTA) {
        vault.accountAppBalanceDelta(key, hookDelta, address(key.hooks));
    }
    vault.accountAppBalanceDelta(key, delta, msg.sender);
}

Add the ability to use overridden LP fees during minting when swaps are involved.

The Swap() function in the CLPoolManager and BinPoolManager smart contract emits an event containing information about delta. However, this event is emitted before the hook's afterswap function is called, which is designed to modify delta. If a hook with afterswap is implemented, the event will emit incorrect values for delta. The same issue exists with the Mint, Burn, and ModifyLiquidity events.

emit Swap(
    id,
    msg.sender,
    delta.amount0(),
    delta.amount1(),
    state.sqrtPriceX96,
    state.liquidity,
    state.tick,
    state.swapFee,
    state.protocolFee
);
 
BalanceDelta hookDelta;
(delta, hookDelta) = CLHooks.afterSwap(key, params, delta, hookData, beforeSwapDelta);
 
if (hookDelta != BalanceDeltaLibrary.ZERO_DELTA) {
    vault.accountAppBalanceDelta(key, hookDelta, address(key.hooks));
}
 
/// @dev delta already includes protocol fee
/// all tokens go into the vault
vault.accountAppBalanceDelta(key, delta, msg.sender);
}

Set event after hook call or add another event after hook call.

The VaultToken is susceptible to an ERC-20 race condition issue related to token approval.

Let's consider a scenario where Alice has approved Eve to spend n of her Vault tokens, but later she decides to change Eve's approval to m tokens. Alice submits a function call to approve with the value n for Eve. Eve runs an Ethereum node, so she knows that Alice is going to change her approval to m.

Eve then submits a transferFrom request, sending n of Alice's tokens to herself, but gives it a much higher gas price than Alice's transaction. The transferFrom executes first so gives Eve n tokens and sets Eve's approval to zero. Then Alice's transaction executes and sets Eve's approval to m. Eve then sends those m tokens to herself, resulting in Eve receiving n + m. tokens. Even though she should have received at most max(n,m).

function approve(address spender, Currency currency, uint256 amount) public virtual returns (bool) {
    allowance[msg.sender][spender][currency] = amount;
 
    emit Approval(msg.sender, spender, currency, amount);
 
    return true;
}

Consider adding increase/decrease allowance functions.

If params.liquidityDelta would be 0, the function CLHooks::beforeModifyLiquidity() (v4-core/pool-cl/libraries/CLHooks.sol line 78-80) would still call the ICLHooks::beforeRemoveLiquidity hook.

This may unnecessarily increase gas cost due to the call invoked in this case inside Hooks::callHook() (v4-core/libraries/Hooks.sol line 79).

The same issue exists in CLHooks::afterModifyLiquidity() where the ICLHooks.afterRemoveLiquidity hook may get called unnecessarily (line 114 in v4-core/libarires/Hooks.sol) via Hooks::callHookWithReturnDelta().

function beforeModifyLiquidity(
    PoolKey memory key,
    ICLPoolManager.ModifyLiquidityParams memory params,
    bytes calldata hookData
) internal {
    ICLHooks hooks = ICLHooks(address(key.hooks));
 
    if (params.liquidityDelta > 0 && key.parameters.shouldCall(HOOKS_BEFORE_ADD_LIQUIDITY_OFFSET, hooks)) {
        Hooks.callHook(hooks, abi.encodeCall(ICLHooks.beforeAddLiquidity, (msg.sender, key, params, hookData)));
    } else if (params.liquidityDelta <= 0 && key.parameters.shouldCall(HOOKS_BEFORE_REMOVE_LIQUIDITY_OFFSET, hooks))
    {
        Hooks.callHook(hooks, abi.encodeCall(ICLHooks.beforeRemoveLiquidity, (msg.sender, key, params, hookData)));
    }
}

function afterModifyLiquidity(
    PoolKey memory key,
    ICLPoolManager.ModifyLiquidityParams memory params,
    BalanceDelta delta,
    BalanceDelta feesAccrued,
    bytes calldata hookData
) internal returns (BalanceDelta callerDelta, BalanceDelta hookDelta) {
    ICLHooks hooks = ICLHooks(address(key.hooks));
    callerDelta = delta;
 
    if (params.liquidityDelta > 0) {
        if (key.parameters.shouldCall(HOOKS_AFTER_ADD_LIQUIDITY_OFFSET, hooks)) {
            hookDelta = BalanceDelta.wrap(
                Hooks.callHookWithReturnDelta(
                    hooks,
                    abi.encodeCall(
                        ICLHooks.afterAddLiquidity, (msg.sender, key, params, delta, feesAccrued, hookData)
                    ),
                    key.parameters.hasOffsetEnabled(HOOKS_AFTER_ADD_LIQUIDIY_RETURNS_DELTA_OFFSET)
                )
            );
 
            callerDelta = callerDelta - hookDelta;
        }
    } else {
        if (key.parameters.shouldCall(HOOKS_AFTER_REMOVE_LIQUIDITY_OFFSET, hooks)) {
            hookDelta = BalanceDelta.wrap(
                Hooks.callHookWithReturnDelta(
                    hooks,
                    abi.encodeCall(
                        ICLHooks.afterRemoveLiquidity, (msg.sender, key, params, delta, feesAccrued, hookData)
                    ),
                    key.parameters.hasOffsetEnabled(HOOKS_AFTER_REMOVE_LIQUIDIY_RETURNS_DELTA_OFFSET)
                )
            );
 
            callerDelta = callerDelta - hookDelta;
        }
    }
}

Consider adjusting the code to call the beforeRemoveLiquidity hook only when params.liquidityDelta < 0 (line 78 inside v4-core/pool-cl/libraries/CLHooks.sol):

} else if (params.liquidityDelta < 0 && key.parameters.shouldCall(HOOKS_BEFORE_REMOVE_LIQUIDITY_OFFSET, hooks))

Also consider adjusting the code to call the afterRemoveLiquidity hook only when liquidity was removed. To do so consider changing line 108 inside v4-core/pool-cl/libraries/CLHooks.sol):

} else if (params.liquidityDelta < 0) {

Commentary from the client:

This is expected behaviour.

If an app should be exploitable in any way, governance might want to have an option to unregister this app from the vault. However currently it's not possible to unregister an app.

function registerApp(address app) external override onlyOwner {
    isAppRegistered[app] = true;
 
    emit AppRegistered(app);
}

Commentary from the client:

This is expected behaviour, if we can register, it means we have the ability to prevent user from removing liquidity. If theres an exploit, we can pause the pool manager.

Consider implementing a mechanism to unregister an app in the Vault contract.

The Ownable.sol contract implemented in the project provides basic access control through single-step ownership transfers, where ownership is immediately transferred to a new address. While this is a common pattern, it introduces certain security risks. These risks arise because the new owner immediately assumes control without any validation, confirmation, or safeguard measures, potentially exposing the contract to accidental or malicious takeovers. If the wrong address is provided, the contract could become permanently inaccessible or controlled by an unintended party.

function transferOwnership(address newOwner) public virtual onlyOwner {
    if (newOwner == address(0)) {
        revert();
    }
    _transferOwnership(newOwner);
}
 
function _transferOwnership(address newOwner) internal virtual {
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
}

Implement a two-step process where the new owner must explicitly accept the ownership transfer, ensuring that only willing and intended parties gain control.

Inside BinPoolManager::donate() if currentBinShare is equal to MIN_BIN_SHARE_FOR_DONATE a revert is triggered incorrectly.

if (currentBinShare <= MIN_BIN_SHARE_FOR_DONATE) {
    revert InsufficientBinShareForDonate(currentBinShare);
}

Consider adjusting the comparison operator to <:

if (currentBinShare < MIN_BIN_SHARE_FOR_DONATE) {

In the BinPoolManager contract, MAX_BIN_STEP and MIN_BIN_SHARE_FOR_DONATE variables are written in uppercase, which typically signals that these variables are either constant or immutable. However, in this case, they are neither.

Using uppercase for non-constant variables goes against common Solidity coding standards and are confusing.

uint16 public override MAX_BIN_STEP = 100;
 
uint256 public override MIN_BIN_SHARE_FOR_DONATE = 2 ** 128;

Follow Solidity's best practices by using camelCase for variables that are neither constant nor immutable. For example:

• maxBinStep instead of MAX_BIN_STEP
• minBinShareForDonate instead of MIN_BIN_SHARE_FOR_DONATE

The code in the CLPool library has been changed, but the documentation has not been updated accordingly. As a result, the documentation is now incorrect and does not reflect the current functionality of the code.

/// @dev If amountSpecified is the output, also given amountSpecified cant be 0,
/// then the tx will always revert if the swap fee is 100%
+ if (state.swapFee >= LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE) {
+     if (!exactInput) {
+         revert InvalidFeeForExactOut();
+     }
- if (!exactInput && (state.swapFee == LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE)) {
-     revert InvalidFeeForExactOut();
}

Update the comments as well, like e.g.

/// @dev If amountSpecified is the output and the swap fee is 100% or greater,
/// then the transaction will revert. In the case of exact input, the transaction
/// can proceed even with a swap fee of 100% or more.

These contracts import the following contracts but do not use them:

• ICLHooks.sol
• Currency.sol
• IBinHooks.sol

import "./interfaces/IBinHooks.sol";

import {Currency} from "../types/Currency.sol";

import {Currency} from "../types/Currency.sol";

import "./interfaces/ICLHooks.sol";

Remove unused imports.

In Solidity (^0.8.0), adding the unchecked keyword around arithmetic operations can reduce gas usage where underflow or overflow is impossible due to preceding checks. In the getSharesAndEffectiveAmountsIn() function of the BinHelper library, can be optimized for better performance and clarity by using unchecked arithmetic for safe performance gains.

// The other way might be more efficient, but as y is the quote asset, it is more valuable
if (deltaLiquidity >= Constants.SCALE) {
    uint256 deltaY = deltaLiquidity >> Constants.SCALE_OFFSET;
    deltaY = deltaY > y ? y : deltaY;
 
    y -= deltaY;
 
    deltaLiquidity -= deltaY << Constants.SCALE_OFFSET;
}
 
if (deltaLiquidity >= price) {
    uint256 deltaX = deltaLiquidity / price;
    deltaX = deltaX > x ? x : deltaX;
 
    x -= deltaX;
}

Using unchecked in the arithmetic operation improves gas efficiency since the greater-than condition ensures that no underflow can occur.

// The other way might be more efficient, but as y is the quote asset, it is more valuable
if (deltaLiquidity >= Constants.SCALE) {
    uint256 deltaY = deltaLiquidity >> Constants.SCALE_OFFSET;
    deltaY = deltaY > y ? y : deltaY;
+   unchecked {
    y -= deltaY;
+   }
    deltaLiquidity -= deltaY << Constants.SCALE_OFFSET;
}
 
if (deltaLiquidity >= price) {
    uint256 deltaX = deltaLiquidity / price;
    deltaX = deltaX > x ? x : deltaX;
+   unchecked {
    x -= deltaX;
+   }
}

The library Hooks inside v4-core/libraries/Hooks.sol is missing its natspec.

import {CustomRevert} from "./CustomRevert.sol";
 
library Hooks {

Consider adding the missing natspec to the library:

+ /**
+  * @title Hooks library
+  * @notice This library provides functions to interact with hooks
+  */
library Hooks {

1. infinity-core/src/ProtocolFeeController.sol
   • In the error message InvalidProtocolFeeSplitRatio();, the word "Invalid" is misspelled as "Invliad".

   error InvliadProtocolFeeSplitRatio();

• In the natspec comment to the setProtocolFee() function, the word "protocol" is misspelled as "protcool".

   /// @notice Override the default protcool fee for the pool

2. infinity-core/src/pool-cl/libraries/CLPool.sol
   • In the comment above the PoolAlreadyInitialized() error; the word "initialize" is misspelled as "initalize".

   /// @notice Thrown when trying to initalize an already initialized pool

Consider fixing all typos before deployment.

In the following locations, there are constant variables that are declared public. However, setting these constants to private will save deployment gas. This is because the compiler won't have to create non-payable getter functions for deployment calldata, won't need to store the bytes of the value outside of where it's used, and won't add another entry to the method ID table. If necessary, the values can still be read from the verified contract source code:

uint256 public constant ONE_HUNDRED_PERCENT_RATIO = 1e6;

The mentioned variables should be marked as private instead of public.

The abstract contract ProtocolFees uses the ProtocolFeeController contract to calculate the fees for a given pool key. It does this using an external call.

Even though protocol fee controller address is configurable (although it is assumed to be ProtocolFeeController.sol in the repository), the function to-be-called is hardcoded to be IProtocolFeeController.protocolFeeForPool, which is a view function in both the interface and the contract.

However, a low-level call is used in assembly instead of staticcall.

function _fetchProtocolFee(PoolKey memory key) internal returns (uint24 protocolFee) {
    if (address(protocolFeeController) != address(0)) {
        address targetProtocolFeeController = address(protocolFeeController);
        bytes memory data = abi.encodeCall(IProtocolFeeController.protocolFeeForPool, (key));
 
        bool success;
        uint256 returnData;
        assembly ("memory-safe") {
            // only load the first 32 bytes of the return data to prevent gas griefing
            success := call(gas(), targetProtocolFeeController, 0, add(data, 0x20), mload(data), 0, 32)
 
            // load the return data
            returnData := mload(0)
 
            // success if return data size is also 32 bytes
            success := and(success, eq(returndatasize(), 32))
        }
 
        // revert if call fails or return size is not 32 bytes
        if (!success) {
            CustomRevert.bubbleUpAndRevertWith(
                targetProtocolFeeController, bytes4(data), ProtocolFeeCannotBeFetched.selector
            );
        }
 
        if (returnData == uint24(returnData) && uint24(returnData).validate()) {
            protocolFee = uint24(returnData);
        } else {
            // revert if return value overflow a uint24 or greater than max protocol fee
            revert ProtocolFeeTooLarge(uint24(returnData));
        }
    }
}

The low-level call should be done using staticcall to adhere to the interface and to ensure no state-changes can be made in custom implementations of the protocol fee controller.

The collectProtocolFee function is designed to collect protocol fees from the pool manager and can only be called by the contract owner:

function collectProtocolFee(address recipient, Currency currency, uint256 amount) external onlyOwner {
    IProtocolFees(poolManager).collectProtocolFees(recipient, currency, amount);
 
    emit ProtocolFeeCollected(currency, amount);
}

The function calls collectProtocolFees() in the pool manager contract, which in turn transfers tokens from the Vault and returns the amountCollected:

function collectProtocolFees(address recipient, Currency currency, uint256 amount)
    external
    override
    returns (uint256 amountCollected)
{
    if (msg.sender != address(protocolFeeController)) revert InvalidCaller();
 
    amountCollected = (amount == 0) ? protocolFeesAccrued[currency] : amount;
    protocolFeesAccrued[currency] -= amountCollected;
    vault.collectFee(currency, amountCollected, recipient);
}

function collectFee(Currency currency, uint256 amount, address recipient) external onlyRegisteredApp {
    // prevent transfer between the sync and settle balanceOfs (native settle uses msg.value)
    (Currency syncedCurrency,) = VaultReserve.getVaultReserve();
    if (!currency.isNative() && syncedCurrency == currency) revert FeeCurrencySynced();
    reservesOfApp[msg.sender][currency] -= amount;
    currency.transfer(recipient, amount);
}

As a result, the ProtocolFeeCollected event may display an amount larger than what was truly collected, leading to inaccurate event data.

Consider returning the difference in the balances before and after the transfer to get the exact fee amount.

According to natspec documentation and the source, the maximum value for newProtocolFee is 4000 which is equivalent to 0,4%.

/// @param newProtocolFee 1000 = 0.1%, and max at 4000 = 0.4%. If set at 0.1%, this means 0.1% of amountIn for each swap will go to protocol
function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external onlyOwner {
    if (address(key.poolManager) != poolManager) revert InvalidPoolManager();
 
    // no need to validate the protocol fee as it will be done in the pool manager
    IProtocolFees(address(key.poolManager)).setProtocolFee(key, newProtocolFee);
}

function setProtocolFee(PoolKey memory key, uint24 newProtocolFee) external virtual {
    if (msg.sender != address(protocolFeeController)) revert InvalidCaller();
    if (!newProtocolFee.validate()) revert ProtocolFeeTooLarge(newProtocolFee);
    PoolId id = key.toId();
    _setProtocolFee(id, newProtocolFee);
    emit ProtocolFeeUpdated(id, newProtocolFee);
}

uint24 internal constant FEE_0_THRESHOLD = 4001;
uint24 internal constant FEE_1_THRESHOLD = 4001 << 12;

function validate(uint24 self) internal pure returns (bool valid) {
    // Equivalent to: getZeroForOneFee(self) <= MAX_PROTOCOL_FEE && getOneForZeroFee(self) <= MAX_PROTOCOL_FEE
    assembly ("memory-safe") {
        let isZeroForOneFeeOk := lt(and(self, 0xfff), FEE_0_THRESHOLD)
        let isOneForZeroFeeOk := lt(and(self, 0xfff000), FEE_1_THRESHOLD)
        valid := and(isZeroForOneFeeOk, isOneForZeroFeeOk)
    }
}

However, in documentation, the maximum protocol fee is 1000 (0.1%) on token0 or token1, depending on swap direction. This discrepancy leads to an inconsistency between the documentation and the source code.

Consider updating the official documentation to align with the actual implementation.