PancakeSwap Infinity Periphery Contracts Security Review Report

The _addLiquidity function of the BinPositionManager should properly check for slippage when taking the user's token to increase liquidity.

The handle slippage, the function uses the validateMaxIn function, however it passes the incorrect parameter values for token 0 and token 1, and instead passes the value for token 0 twice:

delta.validateMaxIn(params.amount0Max, params.amount0Max);

This should be corrected to:

delta.validateMaxIn(params.amount0Max, params.amount1Max);

In the documentation string of the validateMaxIn function we also see that it expects both token 0 and token 1, as it uses these values to check the amount 0 and amount 1 from the delta.

The slippage check for token 1 is now incorrect and can result in direct asset loss through pool manipulation and sandwich attacks.

function _addLiquidity(IBinPositionManager.BinAddLiquidityParams calldata params) internal {
 
    -- SNIP --
 
    /// 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.amount0Max);
}

/// @notice Revert if one or both deltas exceeds a maximum input
/// @param delta The principal amount of tokens to be added, does not include any fees accrued (which is possible on increase)
/// @param amount0Max The maximum amount of token0 to spend
/// @param amount1Max The maximum amount of token1 to spend
/// @dev This should be called when adding liquidity (mint or increase)
function validateMaxIn(BalanceDelta delta, uint128 amount0Max, uint128 amount1Max) internal pure {
    // Called on mint or increase, where we expect the returned delta to be negative.
    // However, on pools where hooks can return deltas on modify liquidity, it is possible for a returned delta to be positive (even after discounting fees accrued).
    // Thus, we only cast the delta if it is guaranteed to be negative.
    // And we do NOT revert in the positive delta case. Since a positive delta means the hook is crediting tokens to the user for minting/increasing liquidity, we do not check slippage.
    // This means this contract will NOT support _positive_ slippage checks (minAmountOut checks) on pools where the hook returns a positive delta on mint/increase.
    int256 amount0 = delta.amount0();
    int256 amount1 = delta.amount1();
    if (amount0 < 0 && amount0Max < uint128(uint256(-amount0))) {
        revert MaximumAmountExceeded(amount0Max, uint128(uint256(-amount0)));
    }
    if (amount1 < 0 && amount1Max < uint128(uint256(-amount1))) {
        revert MaximumAmountExceeded(amount1Max, uint128(uint256(-amount1)));
    }
}

It is recommended:

function _addLiquidity(IBinPositionManager.BinAddLiquidityParams calldata params) internal {
 
    -- SNIP --
 
    /// 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);
--  delta.validateMaxIn(params.amount0Max, params.amount0Max);
}

Note: This finding has been included for completeness, as it was identified prior to the relevant fixes being merged during the audit window by the Pancake team independently.

CLPositionManager and BinPositionManager currently have no way of preventing callers from transferring positions regardless of Vault's SettlementGuard state, and positions may be transferred while the Vault is locked. In the case of CLPositionManager, there are similarly no restrictions on when a position may be subscribed or unsubscribed from.

This may result in unintended effects during modifyLiquidities calls, as it cannot be assumed that positions will maintain the same owner and subscription settings during Vault locks. It may also allow hooks to trigger notifications which may be inaccurate by the time after the liquidity has been modified. Malicious hooks may also re-enter Vault functions during locked states before liquidity has been modified, by triggering the execution of the notifySubscribe callback in CLNotifier::subscribe.

...
function batchTransferFrom(address from, address to, uint256[] calldata ids, uint256[] calldata amounts)
    public
    virtual
    override
...

...
function transferFrom(address from, address to, uint256 id) public virtual override
...

...
function subscribe(uint256 tokenId, address newSubscriber, bytes calldata data)
    external
    payable
    onlyIfApproved(msg.sender, tokenId)
...

...
function unsubscribe(uint256 tokenId) external payable onlyIfApproved(msg.sender, tokenId) {
...

Consider implementing an onlyIfVaultUnlocked modifier to CLPositionManager and BinPositionManager, similar to the current Vault::isLocked modifier, which prevents execution if vault::getLocker() returns a nonzero address.

// to be available for both CLPositionManager and BinPositionManager
...
/// @notice reverts if vault is locked
modifier onlyIfVaultUnlocked() {
    if (vault.getLocker() != address(0)) revert VaultMustBeUnlocked();
    _;
}
...

// v4-periphery/interfaces/IPositionManager.sol
...
/// @notice Thrown when Vault must be unlocked
error VaultMustBeUnlocked();
...

Consider using the trustlessPermit() function, or ensure the contract does not revert when the permit() call fails.

STATUS: Fixed

DESCRIPTION:

The Permit2Forwarder and SelfPermitERC721 contracts integrate the EIP-2612 Permit function, It transfers the burden of holding native (gas) tokens away from users, by allowing them to sign an approval off-chain and send it to a trusted service, which could use the funds as if the user called approve(). Because transactions are in the mempool and accessible to anyone, an attacker can extract the _signature parameters from the current call and front-run it with a direct permit() transaction. In this case, the result is harmful, as the user loses the functionality that follows the permit().

For example, all functions that use the permit functionality for gasless transactions can be blocked, preventing users from using this feature within the project.

function selfPermitERC721(address token, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
    public
    payable
    override
{
    IERC721Permit(token).permit(address(this), tokenId, deadline, v, r, s);
}

/// @notice allows forwarding batch permits to permit2
/// @dev this function is payable to allow multicall with NATIVE based actions
function permitBatch(address owner, IAllowanceTransfer.PermitBatch calldata _permitBatch, bytes calldata signature)
    external
    payable
{
    permit2.permit(owner, _permitBatch, signature);
}

/// @notice allows forwarding a single permit to permit2
/// @dev this function is payable to allow multicall with NATIVE based actions
function permit(address owner, IAllowanceTransfer.PermitSingle calldata permitSingle, bytes calldata signature)
    external
    payable
{
    permit2.permit(owner, permitSingle, signature);
}

Consider using the trustlessPermit() function, or ensure the contract does not revert when the permit() call fails.

The V4Router.sol contract imports ./libraries/SafeCast.sol, but does not use it anywhere, moreover, the SafeCastTemp is already imported in the BinRouterBase.sol and CLRouterBase.sol contracts, from which the V4Router.sol inherits from.

There are also a couple of not-used imports, which are mentioned in the path.

import {SafeCastTemp} from "./libraries/SafeCast.sol";

import {BalanceDelta} from "pancake-v4-core/src/types/BalanceDelta.sol";
import {PoolKey} from "pancake-v4-core/src/types/PoolKey.sol";

import {ActionConstants} from "./libraries/ActionConstants.sol";

import {SafeTransferLib} from "solmate/src/utils/SafeTransferLib.sol";

import {SafeTransferLib} from "solmate/src/utils/SafeTransferLib.sol";
import {Currency} from "pancake-v4-core/src/types/Currency.sol";

import {PoolKey} from "pancake-v4-core/src/types/PoolKey.sol";

Remove unused imports.

The CurrencyLibrary library is assigned to the Currency type in multiple contracts, but in the Currency.sol contract, this library is already assigned globally to the Currency type.

using CurrencyLibrary for Currency global;

Remove redundant assignments of CurrencyLibrary wherever they occur.

BinPositionManager::_addLiquidity() reverts with the custom error IdDesiredOverflows (line 196-197 in v4-periphery/pool-bin/BinPositionManager.sol). A better custom error would be as in TraderJoe V2's function _addLiquidity() to revert with something like IdSlippageCaught. This may be more correct to signify that this revert happened due to the id slippage was caught (activeId is not within slippage).

Also the custom error may be better if it includes the params.idSlippage and activeId similar to TraderJoe's implementation.

(uint24 activeId,,) = binPoolManager.getSlot0(params.poolKey.toId());
 
if (params.activeIdDesired + params.idSlippage < activeId) revert IdDesiredOverflows(activeId);
 
if (params.activeIdDesired - params.idSlippage > activeId) revert IdDesiredOverflows(activeId);

Consider adjusting the custom error so that it signifies that the revert happened because the activeId is not within slippage inside BinPositionManager::_addLiquidity():

if (params.activeIdDesired + params.idSlippage < activeId) revert IdSlippageCaught(params.activeIdDesired, params.idSlippage, activeId);
 
if (params.activeIdDesired - params.idSlippage > activeId) revert IdSlippageCaught(params.activeIdDesired, params.idSlippage, activeId);

1. infinity-periphery/src/MixedQuoter.sol
   • In the function name convertWETHToV4NativeCurency(); the word "Currency" is misspelled as "Curency".

function convertWETHToV4NativeCurency(PoolKey memory poolKey, address tokenIn, address tokenOut)

2. infinity-periphery/src/pool-bin/interfaces/IBinPositionManager.sol
   • In the error message AddLiquidityInputActiveIdMismath(), the word "Mismatch" is misspelled as "Mismath".

error AddLiquidityInputActiveIdMismath();

• In the comment above BinRemoveLiquidityParams struct; the word "receive" is misspelled as "recieve".

/// @notice BinRemoveLiquidityParams
/// - amount0Min: Min amount to recieve for token0
/// - amount1Min: Min amount to recieve for token1

3. infinity-periphery/src/Planner.sol
   • The comment below finalizeSwap(); the word "isn't" is misspelled as "isnt".

blindly settling and taking all, without slippage checks, isnt recommended in prod

4. infinity-periphery/src/base/BaseActionsRouter.sol
   • In the comment above msgSender() function; the word "shouldn't" is misspelled as "shouldnt".

`msg.sender` shouldnt be used, as this will be the v4 vault contract that calls `lockAcquired`

5. infinity-periphery/src/pool-cl/CLMigrator.sol
   • In the comment under the migrateFromV2() and migrateFromV3() functions; the word "manually" is misspelled as "mannually".

/// @notice if user mannually specify the price range, they might need to send extra token

Consider fixing all typos before deployment.