Skate

AMM

6.1 Insecure validation in PeripheryEventEmitter allows off-chain backend to be deceived

The Skate AMM architecture relies on a cross-chain communication model where actions on periphery chains are relayed to the Skate chain for execution. The PeripheryPool contract initiates these actions by creating an entry in the permissionless ActionBox and then triggers a corresponding event via the PeripheryEventEmitter contract. The security model assumes that the PeripheryEventEmitter is a permissioned contract, meaning only legitimate PeripheryPool contracts can call it to emit events. These events are used by off-chain executors to validate and process the actions created in the ActionBox.

However, the validation mechanism within the PeripheryEventEmitter contract is flawed. The internal _isValidCaller(...) function checks if the peripheryPool address passed as an argument is a valid pool, instead of verifying that msg.sender is the one that is a valid pool.

function mint(
    address peripheryPool,
    address kernelPool,
    address user,
    int24 tickLower,
    int24 tickUpper,
    uint256 amount0,
    uint256 amount1
) external {
    // @audit The check validates the argument, not the actual caller (msg.sender).
    _isValidCaller(peripheryPool);
    emit Minted(peripheryPool, kernelPool, user, tickLower, tickUpper, amount0, amount1);
}

The _isValidCaller(...) function checks against the poolExists mapping in the PeripheryManager contract.

function _isValidCaller(address peripheryPool) private view {
    // @audit-issue The function confirms that `peripheryPool` is a valid pool,
    // but does not check if `msg.sender` is this pool.
    require(manager.poolExists(peripheryPool));
}

This vulnerability allows an attacker to call any function in the PeripheryEventEmitter from a malicious contract. By providing the address of a legitimate PeripheryPool as the peripheryPool argument, the attacker can successfully pass the _isValidCaller(...) check. This enables the attacker to emit fraudulent events (e.g., Minted, Swapped) with arbitrary parameters, such as minting a large number of tokens to their address without providing any underlying assets. These spoofed events appear to originate from a legitimate pool, breaking the backend's core security assumption that only valid pools can emit events from this contract. The off-chain executor, which trusts events from the PeripheryEventEmitter, will be deceived by these fraudulent events, leading to the execution of unauthorized actions on the Skate chain and resulting in a direct loss of funds from the AMM pools.

Recommendation

Consider redesigning the validation mechanism within the PeripheryEventEmitter contract to authenticate the caller ( msg.sender) instead of an input parameter.

Resolution

The remediation is applied. commit

6.2 Missing authorization check in update_pool_config(...) allows full drain of funds from periphery pools

The update_pool_config(...) function is intended to be a privileged function that allows the pool's manager to update critical configuration parameters. These parameters include the gateways array, which contains a list of public keys authorized to call sensitive settlement functions.

However, the account validation for this function is insufficient. While the UpdatePoolConfig struct correctly specifies that the manager account must be a Signer, it fails to verify that this signer is the _actual manager_ of the periphery_pool. The check to ensure that ctx.accounts.manager.key() is the same as ctx.accounts.periphery_pool.manager is missing.

pub fn update_pool_config(
    ctx: Context<UpdatePoolConfig>,
    chain_id: Option<u32>,
    src_vm_type: Option<u8>,
    description: Option<String>,
    gateways: Option<Vec<Pubkey>>,
    kernel_factory: Option<String>,
    kernel_pool: Option<String>,
) -> Result<()> {
    // @audit-issue The function lacks a check to ensure that the signer
    // is the legitimate manager of the pool.
    let pool = &mut ctx.accounts.periphery_pool;
    // ...
    if let Some(gateways) = gateways {
        pool.gateways = gateways;
    }
    // ... other updates
    Ok(())
}

The corresponding UpdatePoolConfig struct is defined as follows:

#[derive(Accounts)]
pub struct UpdatePoolConfig<'info> {
    #[account(mut)]
    pub periphery_pool: Box<Account<'info, PeripheryPool>>,

    // @audit-issue The `has_one = manager` constraint is missing on the
    // `periphery_pool` account.
    #[account(mut)]
    pub manager: Signer<'info>,
}

This vulnerability allows any malicious actor to call update_pool_config(...). An attacker can add their own public key to the gateways list. As an authorized gateway, the attacker can then invoke privileged functions such as settle_mint(...), settle_burn(...), and settle_swap(...). These functions execute token transfers from the pool's token accounts. By providing crafted data to these settlement functions, the attacker can drain all token0 and token1 funds held by the PeripheryPool contract.

Recommendation

Consider enforcing that the manager signer account matches the manager address stored in the periphery_pool account. This can be achieved by adding the has_one = manager constraint to the periphery_pool account in the UpdatePoolConfig validation struct.

6.3 Race condition in removeStagedAssets allows for theft of any staged assets

The Skate AMM architecture separates user interactions on periphery chains from core logic on the Skate chain. When a user calls functions like mint(...), increaseLiquidity(...), or swap(...) on a PeripheryPool contract, their tokens are transferred to the pool, and their staged balance is recorded in the usersData mapping. An off-chain executor then picks up this action and attempts to process it on the KernelManager contract on the Skate chain.

If the kernel transaction succeeds, an appropriate settlement function is called on the PeripheryPool to finalize the state. However, if the kernel transaction fails for any reason, an executor is expected to call removeStagedAssets(...) to refund the user their staged tokens.

The vulnerability lies in the removeStagedAssets(...) function. This function refunds the user's _entire_ staged balance, as recorded in usersData, rather than the specific amounts associated with the single failed action. This creates a critical race condition that a malicious user can exploit across any function that stages assets.

An attacker can initiate a transaction (e.g., a mint or swap) that is designed to fail on the kernel chain. When they observe an executor submitting a transaction to call removeStagedAssets(...) to refund them, they can front-run the executor's transaction with a second, valid transaction (e.g., another mint, increaseLiquidity, or swap). This second call adds more funds to their staged balance in usersData. The subsequent execution of removeStagedAssets(...) will then read the now-inflated balance and transfer the assets from _both_ actions back to the attacker.

The impact is that the attacker receives a full refund for their second (valid) action, while that action is still processed successfully on the kernel chain, granting them a free liquidity position or swap. This drains funds from the PeripheryPool, as the assets that should have backed the user's action have been incorrectly returned. This critical issue is compounded by the _settle(...) function, which silently handles accounting underflows, masking the state inconsistency when the second action is eventually settled.

All functions that add to a user's usersData balance are vectors for this attack, including mint(...):

function mint(...) external override {
    // ...
    // @audit The user's staged balance is increased, making it vulnerable.
    UserData storage data = usersData[msg.sender];
    data.amount0 += amount0;
    data.amount1 += amount1;
    // ...
}

The increaseLiquidity(...) function follows the same vulnerable pattern:

function increaseLiquidity(...) external {
    // ...
    // @audit This function also adds to the user's staged balance.
    UserData storage data = usersData[msg.sender];
    data.amount0 += amount0;
    data.amount1 += amount1;
    // ...
}

Similarly, the swap(...) function stages the input token amount, making it a viable entry point for the attack:

function swap(...) external override {
    // ...
    // @audit The input token for the swap is staged here.
    if (zeroForOne) {
        tokenIn = token0;
        usersData[msg.sender].amount0 += amountIn;
    } else {
        tokenIn = token1;
        usersData[msg.sender].amount1 += amountIn;
    }
    // ...
}

Regardless of which function is used to stage the assets, the generic removeStagedAssets(...) function refunds the entire balance, enabling the exploit.

function removeStagedAssets(address usr) external override onlyExecutor {
    // @audit-issue Reads the entire staged balance for the user,
    // not just the balance from a specific failed action.
    UserData memory userData = usersData[usr];
    uint256 amount0 = userData.amount0;
    uint256 amount1 = userData.amount1;
    // @audit This zeroes out the user's balance.
    _settle(usr, amount0, amount1);
    // @audit The user is refunded the entire staged amount.
    if (amount0 != 0) IERC20(token0).transfer(usr, amount0);
    if (amount1 != 0) IERC20(token1).transfer(usr, amount1);
    // ...
}

The _settle(...) function's silent handling of underflows prevents a revert that would otherwise signal the accounting error when the attacker's valid transaction is settled after the refund has already occurred.

function _settle(address user, uint256 amount0, uint256 amount1) private {
    // to avoid under flow due to possible precision mismatch
    UserData storage userData = usersData[user];
    // @audit-issue After `removeStagedAssets` has run, `userData.amount0` will be 0.
    // When the settlement function for the second valid action calls this,
    // it should underflow but instead silently evaluates to 0, hiding the exploit.
    userData.amount0 = userData.amount0 > amount0 ? userData.amount0 - amount0 : 0;
    userData.amount1 = userData.amount1 > amount1 ? userData.amount1 - amount1 : 0;

    manager.eventEmitter().amountSettled(address(this), kernelPool, user, amount0, amount1);
}

Recommendation

Consider modifying the removeStagedAssets(...) function to accept the specific amount0 and amount1 to be removed as parameters, likely derived from the failed action data. This will ensure that only the funds from a single failed action are refunded, preventing the described race condition.

It is also recommended to review the settlement logic in the _settle(...) function. The current approach to prevent underflow-related reverts inadvertently conceals the theft of funds.

Resolution

The remediation is applied. commit

Additonal remediation is applied. commit

Update from Nethermind Security

The applied fix is incorrect as it still uses the amount from the user data storage struct to transfer the tokens to the user. The attack described in the issue is still possible.

6.4 Inconsistent extraData normalization leads to potential loss of funds

In the Skate AMM, user interactions occur on PeripheryPool contracts deployed on various chains, while the core AMM logic is executed on the central KernelManager contract. When a user wishes to withdraw liquidity via the burn(...) or decreaseLiquidity(...) functions in the PeripheryPool, they can provide slippage protection parameters ( amount0Min, amount1Min) within the extraData field.

The PeripheryPool contract first uses these minimum amounts to check against its own token balances. These balances are in their native, un-normalized decimal format.

// src/amm/periphery/PeripheryPool.sol
function decreaseLiquidity(
    uint256 tokenId,
    uint128 liquidityAmount,
    bytes memory extraData // must contain amount0Min, amount1Min
)
    external
{
    // @audit extraData is decoded and used to check against un-normalized balances.
    (uint256 amount0Min, uint256 amount1Min) = abi.decode(extraData, (uint256, uint256));
    (uint256 amount0Available, uint256 amount1Available) = balancesAvailable();
    require(
        amount0Min <= amount0Available && amount1Min <= amount1Available,
        "PeripheryPool::decreaseLiquidity: not enough balance in periphery pool"
    );
    // ...
    // @audit The same extraData is passed to the kernel.
    action.kernelAppCalldata = abi.encodeWithSignature(
        "decreaseLiquidity(bytes)",
        abi.encode(
            // ...
            liquidityAmount,
            extraData
        )
    );
    // ...
}

The same extraData is then forwarded to the KernelManager contract. Inside the KernelManagerLib, the decreaseLiquidity(...) function performs its own slippage check. However, this check compares the amount_Min values from extraData against the token amounts returned by the KernelPool, which are normalized to 18 decimals.

// src/amm/kernel/libraries/KernelManagerLib.sol
function decreaseLiquidity(
    DataTypes.State storage state,
    bytes calldata decreaseLiquidityData
)
    external
    returns (IMessageBox.Task[] memory tasks)
{
    (
        // ...
        uint128 liquidityAmount,
        bytes memory extraData
    ) = abi.decode(
        decreaseLiquidityData,
        (bytes32, uint256, uint256, bytes32, address, uint256, uint128, bytes)
    );
    // ...
    (vars.amount0, vars.amount1) = vars.pool.burn(
        address(this), position.tickLower, position.tickUpper, liquidityAmount, false
    );
    // @audit-issue extraData is decoded again and compared against normalized amounts.
    (vars.amount0Min, vars.amount1Min) = abi.decode(extraData, (uint256, uint256));
    require(
        vars.amount0 >= vars.amount0Min && vars.amount1 >= vars.amount1Min,
        "price slippage check"
    );
    // ...
}

This creates a fundamental conflict. It is impossible for a user to provide a single extraData value that is valid for both checks simultaneously for tokens that do not have 18 decimals.

• If the user provides un-normalized amount_Min values (correct for the periphery check), the slippage check on the kernel side becomes ineffective. An attacker could exploit this by front-running the transaction, manipulating the price, and causing the user to receive far fewer tokens than expected, leading to significant financial loss;
• If the user provides normalized amount_Min values (correct for the kernel check), the initial check on the periphery side will fail for any token with fewer than 18 decimals. This is because the check compares a numerically large, normalized value (e.g., 1 * 10^18 for 1 token) against the pool's total balance in its native, smaller decimal format (e.g., 1000 * 10^6 for 1000 USDC). The check will incorrectly determine that the minimum amount requested exceeds the available balance, causing legitimate transactions to revert and preventing users from withdrawing liquidity with proper slippage protection;

Recommendation

Consider revisiting and changing the slippage check mechanism in such a way that appropriate checks, using the correct decimal precision, are performed on each chain. It is also recommended to review all other functions that accept extraData to ensure consistent handling of slippage parameters across the entire protocol.

Resolution

The remediation is applied. commit

6.5 Incorrect decimal normalization in swap leads to bypassed slippage protection

The Skate AMM architecture separates user interactions on periphery chains from the core logic on the Skate chain. The PeripheryPool contract, deployed on various EVM chains, is responsible for normalizing all token amounts to a standard 18-decimal precision before relaying actions to the KernelManager contract on the Skate chain. This ensures consistent mathematical operations within the core protocol.

Users initiate swaps by calling the swap(...) function in the PeripheryPool contract. This function accepts amountSpecified and an extraData parameter, which contains the minAmountOutOrMaxAmountIn value for slippage protection.

The swap(...) function correctly normalizes the amountSpecified (the input amount) to 18 decimals. However, it fails to normalize the minAmountOutOrMaxAmountIn value decoded from extraData. This unnormalized value is then passed along with the normalized input amount to the KernelManager.

// src/amm/periphery/PeripheryPool.sol
function swap(
    bytes32 recipient,
    bool zeroForOne,
    int256 amountSpecified,
    uint160 sqrtPriceLimitX96,
    bytes memory extraData
) external override {
    // ...
    // @audit minAmountOutOrMaxAmountIn is decoded but not normalized.
    (uint256 minAmountOutOrMaxAmountIn, uint256 destChainId, uint256 destVmType) =
        abi.decode(extraData, (uint256, uint256, uint256));

    // ...
    (uint256 amountIn, uint256 amountOut) = exactIn
        ? (uint256(amountSpecified), minAmountOutOrMaxAmountIn)
        : (minAmountOutOrMaxAmountIn, uint256(-amountSpecified));

    // ...
    // @audit `amountIn` is correctly normalized.
    uint256 amountInNormalized = _normalizeTokenAmount(amountIn, tokenIn);

    // ...
    action.kernelAppCalldata = abi.encodeWithSignature(
        "swap(bytes)",
        abi.encode(
            // ...
            int256(amountInNormalized),
            sqrtPriceLimitX96,
            // @audit `extraData` containing the unnormalized value is passed to the kernel.
            extraData
        )
    );
    // ...
}

The KernelManagerLib contract on the Skate chain decodes this data and performs its swap logic, including the slippage check. The check compares the calculated output amount (which is normalized to 18 decimals) against the unnormalized minAmountOutOrMaxAmountIn provided by the user.

// src/amm/kernel/libraries/KernelManagerLib.sol
function swap(
    DataTypes.State storage state,
    bytes calldata swapData
) external returns (IMessageBox.Task[] memory tasks) {
    // ...
    // @audit `extraData` is decoded again here. `minAmountOutOrMaxAmountIn` is not normalized.
    (uint256 minAmountOutOrMaxAmountIn,,) = abi.decode(extraData, (uint256, uint256, uint256));
    vars.exactIn = amountSpecified > 0;
    if (zeroForOne) {
        vars.amount0ToSettle =
            vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
        // @audit `vars.amount1ToTransfer` is a normalized value.
        vars.amount1ToTransfer = uint256(-vars.amount1Signed);
        // @audit-issue The following line may underflow.
        vars.amount0ToTransfer = vars.amount0ToSettle - uint256(vars.amount0Signed);
    } else {
        // @audit `vars.amount0ToTransfer` is a normalized value.
        vars.amount0ToTransfer = uint256(-vars.amount0Signed);
        vars.amount1ToSettle =
            vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
        // @audit-issue The following line may underflow.
        vars.amount1ToTransfer = vars.amount1ToSettle - uint256(vars.amount1Signed);
    }

    // @audit-issue Slippage check compares a normalized amount with an unnormalized amount.
    require(
        (zeroForOne ? vars.amount1ToTransfer : vars.amount0ToTransfer)
            >= (vars.exactIn ? minAmountOutOrMaxAmountIn : uint256(-amountSpecified)),
        "PeripheryPool::swap: slippage check"
    );
    // ...
}

This discrepancy has a severe impact when swapping tokens with decimals less than 18. For an exact-input swap, the calculated output amount ( amount1ToTransfer or amount0ToTransfer) will be an 18-decimal normalized value, while minAmountOutOrMaxAmountIn will be a native, non-normalized value. The slippage check becomes normalized_output >= unnormalized_minimum_output, which will almost always pass, even if the user receives far fewer tokens than their minimum expectation. This allows a malicious actor or unfavorable market conditions to cause significant financial loss to the user.

Furthermore, for exact-output swaps, the logic uses the unnormalized minAmountOutOrMaxAmountIn (representing maxAmountIn) in calculations with normalized values. This can lead to arithmetic underflows and cause the transaction to revert.

Recommendation

Consider normalizing the minAmountOutOrMaxAmountIn parameter.

Resolution

The remediation is applied. commit

6.6 Incorrect liquidity recalculation leads to loss of funds

The mint(...) and increaseLiquidity(...) function in the KernelManagerLib contract is used to add liquidity to a pool. It internally calls the _calcAmountsUsedInMintAndValidate(...) helper function to determine the amount of liquidity to be minted and to perform slippage checks.

The _calcAmountsUsedInMintAndValidate(...) function first correctly calculates the liquidityAmount based on the user's desired deposit amounts. It then calls pool.mint(...), which mints the liquidity and returns the actual token amounts used, amount0Used and amount1Used. However, the function then incorrectly recalculates the liquidityAmount using these returned amounts. This logic deviates from the standard Uniswap V3 implementation.

function _calcAmountsUsedInMintAndValidate(
    IKernelPool pool,
    uint160 sqrtPriceX96,
    int24 tickLower,
    int24 tickUpper,
    uint256 amount0,
    uint256 amount1,
    bytes memory extraData
)
    private
    returns (uint256 amount0Used, uint256 amount1Used, uint128 liquidityAmount)
{
    // @audit First, the correct liquidity is calculated from the desired amounts.
    liquidityAmount = LiquidityAmounts.getLiquidityForAmounts(
        sqrtPriceX96,
        tickLower.getSqrtRatioAtTick(),
        tickUpper.getSqrtRatioAtTick(),
        amount0,
        amount1
    );
    // @audit The pool mints the correct amount of liquidity.
    (amount0Used, amount1Used) =
        pool.mint(address(this), tickLower, tickUpper, liquidityAmount, "", false);

    // @audit-issue The liquidity amount is incorrectly recalculated here using the actual amounts used.
    // This will likely result in a smaller liquidity value than what was actually minted.
    liquidityAmount = LiquidityAmounts.getLiquidityForAmounts(
        sqrtPriceX96,
        tickLower.getSqrtRatioAtTick(),
        tickUpper.getSqrtRatioAtTick(),
        amount0Used,
        amount1Used
    );
    (uint256 amount0Min, uint256 amount1Min) = abi.decode(extraData, (uint256, uint256));
    require(amount0Used >= amount0Min && amount1Used >= amount1Min, "price slippage check");
}

The mint(...) and increaseLiquidity(...) function in KernelManagerLib then uses this incorrect, smaller liquidityAmount to create the user's NFT position.

function mint(
    DataTypes.State storage state,
    bytes calldata mintData
)
    external
    returns (IMessageBox.Task[] memory tasks)
{
    // ...
    (vars.amount0Used, vars.amount1Used, vars.liquidityAmount) =
    _calcAmountsUsedInMintAndValidate(
        vars.pool, vars.sqrtPriceX96, tickLower, tickUpper, amount0, amount1, extraData
    );
    // ...
    // @audit The incorrect, smaller liquidityAmount is stored in the user's NFT position.
    state.nftPositions[vars.tokenId] = DataTypes.NFTPosition({
        pool: kernelPool,
        tickLower: tickLower,
        tickUpper: tickUpper,
        liquidity: vars.liquidityAmount,
        // ...
    });
    // ...
}

Due to the internal logic of LiquidityAmounts.getLiquidityForAmounts(...), the recalculated liquidityAmount will often be less than the amount of liquidity that was actually added to the pool. When this smaller value is stored with the user's NFT, their position is underrepresented. This will prevent the user from withdrawing their full share of the liquidity and collecting all the fees they are entitled to, leading to a direct and permanent loss of funds.

Recommendation

Consider removing the recalculation of liquidityAmount in the _calcAmountsUsedInMintAndValidate(...) function. The liquidity amount returned by the first call to pool.mint(...) is the correct amount that was minted and should be associated with the user's NFT position. The logic should be aligned with the reference Uniswap V3 implementation.

Resolution

The remediation is applied. commit

6.7 Ineffective Slippage Protection and Denial of Service for Exact-Output Swaps

The swap(...) function in the periphery_pool program on Solana allows users to initiate token swaps. It accepts a min_amount_out_or_max_amount_in parameter, which is crucial for protecting users from slippage. For exact-input swaps, this parameter specifies the minimum amount of output tokens the user is willing to accept. For exact-output swaps, it defines the maximum amount of input tokens the user is willing to spend. After a user calls swap(...), the function constructs a KernelAction::Swap payload and sends it to the action_box program. An off-chain executor then relays this action to the Kernel on the Skate chain for final processing and settlement.

However, the KernelAction::Swap struct defined in kernel_action.rs does not include a field for the min_amount_out_or_max_amount_in parameter. This means this critical slippage protection value is not passed to the Kernel. The off-chain executor processing the action is forced to use a default value, which is 0, when executing the swap logic on the Skate chain.

This leads to two significant issues:

1. No Slippage Protection: For exact-input swaps, the slippage check on the Kernel side becomes ineffective because it compares the actual output amount against a minimum of 0. This exposes users to substantial financial loss, as their swaps can be executed at extremely unfavorable prices during volatile market conditions or by a malicious actor front-running their transaction;
2. Denial of Service for Exact-Output Swaps: For exact-output swaps, the Kernel logic uses the max_amount_in value (which it receives as 0) in its calculations. This leads to an arithmetic underflow, causing the transaction on the Skate chain to revert. Consequently, all exact-output swaps initiated from the Solana periphery are non-functional, representing a denial of service for a core feature of the AMM;

// File: solana/periphery_pool/src/lib.rs

pub fn swap(
    ctx: Context<SwapCtx>,
    recipient: String,
    zero_for_one: bool,
    amount_specified: i64,
    sqrt_price_limit_x96: String,
    // @audit This parameter is used for local checks but is not included in the
    // kernel_action payload, leaving the kernel without slippage protection information.
    min_amount_out_or_max_amount_in: u64,
    dest_chain_id: u32,
    dest_vm_type: u8,
    _extra_data: Vec<u8>,
    action_box_seed: u64,
) -> Result<()> {
    // ...
    let (amount_in, amount_out) = if exact_in {
        (amount_specified as u64, min_amount_out_or_max_amount_in)
    } else {
        let amount_out_val = amount_specified.checked_neg().ok_or(PeripheryError::InvalidAmounts)?;
        (min_amount_out_or_max_amount_in, amount_out_val as u64)
    };
    // ...
    // @audit-issue The `KernelAction` does not include `min_amount_out_or_max_amount_in`.
    let kernel_action = KernelAction::Swap {
        action_id: action_id.clone(),
        chain_id: pool.chain_id,
        src_vm_type: pool.src_vm_type,
        dest_chain_id,
        dest_vm_type,
        recipient: recipient.clone(),
        user: user_bytes32.clone(),
        kernel_pool: pool.kernel_pool.clone(),
        zero_for_one,
        amount_specified,
        sqrt_price_limit_x96: sqrt_price_limit_x96.clone(),
        extra_data: _extra_data.clone(),
    };
    // ...
}

// File: src/amm/kernel/libraries/KernelManagerLib.sol

    function swap(
        DataTypes.State storage state,
        bytes calldata swapData
    )
        external
        returns (IMessageBox.Task[] memory tasks)
    {
        // ...
        SwapLocalVars memory vars;
        (vars.amount0Signed, vars.amount1Signed, vars.sqrtPriceX96, vars.liquidity, vars.tick) =
        IKernelPool(kernelPool).swap(
            _recipient, zeroForOne, amountSpecified, sqrtPriceLimitX96, extraData, false
        );

        (uint256 minAmountOutOrMaxAmountIn,,) = abi.decode(extraData, (uint256, uint256, uint256));
        vars.exactIn = amountSpecified > 0;
        if (zeroForOne) {
            // @audit It will be 0 if exactIn is false
            vars.amount0ToSettle =
                vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
            vars.amount1ToTransfer = uint256(-vars.amount1Signed);
            // @audit-issue It will revert due to underflow as vars.amount0ToSettle will be 0 if exactIn is false
            vars.amount0ToTransfer = vars.amount0ToSettle - uint256(vars.amount0Signed);
        } else {
            vars.amount0ToTransfer = uint256(-vars.amount0Signed);
            // @audit It will be 0 if exactIn is false
            vars.amount1ToSettle =
                vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
            // @audit-issue It will revert due to underflow as vars.amount1ToSettle will be 0 if exactIn is false
            vars.amount1ToTransfer = vars.amount1ToSettle - uint256(vars.amount1Signed);
        }

        // ...
    }

Recommendation

Consider revisiting the cross-chain communication mechanism for swap operations. The current implementation fails to propagate the user's slippage preferences from the Solana periphery to the Kernel. A revised design should ensure that all critical parameters, including min_amount_out_or_max_amount_in, are reliably transmitted and enforced during execution on the Skate chain. This will restore the functionality of exact-output swaps and re-enable essential slippage protection for users performing exact-input swaps.

6.8 Rounding in favor of the user in normalize_amount_for_transfer leads to periphery pool insolvency

When a user interacts with periphery pool operations such as mint(...) or increase_liquidity(...), they provide tokens to the contract. The amounts for these operations are standardized to a 6-decimal precision from the front-end. The normalize_amount_for_transfer(...) helper function is responsible for converting these standardized amounts to the token's native decimal precision before the actual transfer_checked(...) call is made to pull funds from the user.

However, for tokens with fewer than 6 decimals, the normalization logic involves division, which truncates the result. This effectively rounds the amount down in favor of the user. As a result, the actual number of tokens transferred from the user's wallet is less than the amount used for the pool's internal accounting and for the AMM calculations performed by the KernelManager on the Skate chain.

This discrepancy between the accounted amount and the actually transferred amount leads to a state where the protocol's internal ledger is inconsistent with its real token holdings. Over time, even during normal usage, these small rounding errors will accumulate across numerous transactions, causing the PeripheryPool's accounted value to diverge from its actual reserves, eventually leading to insolvency. In a more severe scenario, a malicious actor could repeatedly exploit this vulnerability. By making a series of deposits and withdrawals, the attacker can consistently extract slightly more assets than they provided, systematically draining the PeripheryPool of funds.

pub fn normalize_amount_for_transfer(amount: u64, token_decimals: u8) -> Result<u64> {
    // ...
    else {
        // Need to divide the amount (e.g., token with 3 decimals)
        let decimal_diff = NORMALIZED_DECIMALS - token_decimals;
        let divisor = 10u64.pow(decimal_diff as u32);

        // @audit-issue Division truncates the result, rounding down the amount
        // transferred from the user, while accounting uses the original, larger amount.
        let adjusted_amount: u64 = amount.checked_div(divisor)
            .ok_or(PeripheryError::DivisionByZero)?;

        return Ok(adjusted_amount);
    }
}

Recommendation

Consider adjusting the rounding direction in the normalize_amount_for_transfer(...) function. When converting amounts for tokens with fewer decimals than the normalized standard, the calculation should round up to ensure the protocol always receives at least the amount it accounts for. This prevents discrepancies that could lead to the drainage of funds.

6.9 Swap logic incorrectly handles exact output swaps leading to failed slippage protection

The swap functionality in the Skate AMM is designed to support both exact input and exact output swaps, similar to Uniswap V3. A user specifies an exact input swap with a positive amountSpecified and an exact output swap with a negative amountSpecified. However, the implementation incorrectly processes exact output swaps, leading to failed slippage protection and users swapping more tokens than intended.

The issue begins in the PeripheryPool.swap(...) function. When a user provides a negative amountSpecified for an exact output swap, the exactIn flag is correctly identified as false. The amountIn variable is set to minAmountOutOrMaxAmountIn, which represents the maximum input the user is willing to spend.

// src/amm/periphery/PeripheryPool.sol
function swap(
    // ...
)
    external
    override
{
    // ...
    bool exactIn = amountSpecified > 0;
    (uint256 amountIn, uint256 amountOut) = exactIn
        ? (uint256(amountSpecified), minAmountOutOrMaxAmountIn)
        : (minAmountOutOrMaxAmountIn, uint256(-amountSpecified));
    // ...
}

However, when creating the action to be sent to the kernel, the amountSpecified field in the kernelAppCalldata is encoded as int256(amountInNormalized). Since amountIn is always a positive value, the amountSpecified sent to the kernel will always be positive.

// src/amm/periphery/PeripheryPool.sol
// ...
action.kernelAppCalldata = abi.encodeWithSignature(
    "swap(bytes)",
    abi.encode(
        // ...
        // @audit-issue The `amountSpecified` sent to the kernel is derived from `amountIn`,
        // which is always positive. This erases the distinction between the
        // exact-input and exact-output swaps.
        int256(amountInNormalized),
        sqrtPriceLimitX96,
        extraData
    )
);
// ...

Consequently, the KernelManagerLib.swap(...) function always receives a positive amountSpecified, causing it to interpret every swap as an exact input swap. This breaks the logic for handling exact output swaps.

The primary impact is the failure of the slippage protection mechanism for what should be exact output swaps. In KernelManagerLib.swap(...), the vars.exactIn flag is always true. The slippage check is as follows:

// src/amm/kernel/libraries/KernelManagerLib.sol
// ...
require(
    // @audit `vars.exactIn` is always true, so this check is performed with minAmountOutOrMaxAmountIn.
    (zeroForOne ? vars.amount1ToTransfer : vars.amount0ToTransfer)
        >= (vars.exactIn ? minAmountOutOrMaxAmountIn : uint256(-amountSpecified)),
    "PeripheryPool::swap: slippage check"
);
// ...

For an intended exact output swap, minAmountOutOrMaxAmountIn represents the user's _maximum input amount_. The check incorrectly compares the actual output amount ( vars.amount1ToTransfer or vars.amount0ToTransfer) against this maximum input amount ( amountOut >= maxAmountIn). This comparison is misleading and does not offer effective slippage protection. The correct check should ensure the actual input amount does not exceed the user's specified maximum.

This flaw can cause transactions to revert incorrectly or, more critically, allow swaps to execute with significant negative slippage, causing financial loss for the user. Furthermore, since the swap is always treated as exact input using the user's maxAmountIn, the user may swap a larger amount of tokens than necessary to achieve their desired output amount.

Recommendation

Consider revisiting the approach for how swap parameters are passed from the periphery to the kernel. One potential path is to forward the original, user-provided amountSpecified to the kernel layer, ensuring its sign is preserved after normalization, rather than calculating a new positive value from the amountIn. Following this or any alternative change, it is advisable to perform a holistic review of the entire swap-related logic across all contracts to ensure the necessary adjustments are made for end-to-end consistency and correctness.

Resolution

The remediation is applied. commit

6.10 The initialize_tokens instruction lacks access control and validation

The PeripheryPool contract is designed to be initialized in two steps. First, the initialize_pool(...) instruction creates the PeripheryPool state account, setting its configuration, including the manager address. Second, the initialize_tokens(...) instruction is called to create the token accounts ( pool_token0 and pool_token1) that will hold the pool's assets. This second instruction is intended to be called by the manager.

However, the InitializeTokens accounts context is missing a crucial access control check. It does not verify that the manager signer is the same one stored in the periphery_pool account. This allows anyone to call initialize_tokens(...) for any existing pool.

Furthermore, the instruction can be called multiple times for the same pool with different token mints. Because the pool's token accounts are created as PDAs using the token mint's address as a seed ( seeds = [b"pool-token", periphery_pool.key().as_ref(), token0.key().as_ref()]), an attacker can provide a different mint address to create a new, distinct token account PDA, bypassing the init constraint that would otherwise prevent re-initialization.

This can lead to a denial-of-service condition. An attacker can call initialize_tokens(...) on a legitimate, existing pool and provide malicious or incorrect token mints. This will re-initialize the token0 and token1 pubkeys stored in the PeripheryPool state. When off-chain services later attempt to settle actions (e.g., settle_mint(...)), their transactions will fail because the token accounts they derive will not match the new, incorrect ones on-chain. This effectively bricks the periphery pool, halting all operations. The protocol cannot fix the legitimate pools by calling initialize_tokens(...) again with the correct values which were previously set, because the token account PDAs were already created, causing any subsequent init attempt to revert.

#[derive(Accounts)]
pub struct InitializeTokens<'info> {
    // @audit-issue Access control is missing. Anyone can call this instruction.
    #[account(mut)]
    pub periphery_pool: Box<Account<'info, PeripheryPool>>,
    #[account(
        seeds = [b"pool-authority", periphery_pool.key().as_ref()],
        bump,
    )]
    /// CHECK: This is a derived PDA used as token account authority.
    pub pool_authority: UncheckedAccount<'info>,
    // @audit Using a different token mint will result in a new PDA,
    // allowing re-initialization of the pool's token accounts.
   #[account(
        init,
        payer = manager,
        token::mint = token0,
        token::authority = pool_authority,
        token::token_program = token_program0,
        seeds = [b"pool-token", periphery_pool.key().as_ref(), token0.key().as_ref()],
        bump,
    )]
    pub pool_token0: Box<InterfaceAccount<'info, TokenAccount>>,
    #[account(
        init,
        payer = manager,
        token::mint = token1,
        token::authority = pool_authority,
        token::token_program = token_program1,
        seeds = [b"pool-token", periphery_pool.key().as_ref(), token1.key().as_ref()],
        bump,
    )]
    pub pool_token1: Box<InterfaceAccount<'info, TokenAccount>>,
    // ...
}

Recommendation

Consider implementing the following mitigations:

• Enforce strict access control on the initialize_tokens(...) instruction. The instruction should only be executable by the manager address that was set during the pool's creation and is stored within the PeripheryPool state;
• Introduce a state check to prevent re-initialization. The logic should ensure that the token accounts for a PeripheryPool can only be set once, preventing any party from overwriting the correct token addresses;
• Validate the PeripheryPool account by deriving its address from its creation seeds within the initialize_tokens(...) instruction. This ensures that the token0 and token1 mints used for token account creation are the same ones used when the pool itself was created, maintaining data integrity;

6.11 Unsafe usage of ERC-20 token transfer functions

The PeripheryPool contract facilitates Automated Market Maker (AMM) operations, which involve frequent transfers of ERC-20 tokens to and from users. These transfers are performed using the standard transfer(...) and transferFrom(...) functions.

However, the contract does not check the boolean return value of these ERC-20 token transfer calls. According to the ERC-20 standard, these functions can return false to signal a failed transfer without reverting the transaction. Certain tokens, such as those with fee-on-transfer mechanisms, pausable contracts, or blacklisting features, may utilize this return pattern.

By not verifying the return value, the contract will continue its execution as if the transfer was successful, even when it failed. This creates a discrepancy between the contract's internal state (e.g., updating a user's deposit amounts in usersData) and the actual token balances. This can lead to incorrect accounting and potential loss of funds for either the user or the protocol.

The issue is present in multiple functions, as seen in the mint(...) function:

function mint(...) external override {
    // ...
    UserData storage data = usersData[msg.sender];
    data.amount0 += amount0;
    data.amount1 += amount1;
    // ...
    if (amount0 != 0) {
        // @audit-issue The return value of `transferFrom` is not checked.
        IERC20(token0).transferFrom(msg.sender, address(this), amount0);
    }
    if (amount1 != 0) {
        // @audit-issue The return value of `transferFrom` is not checked.
        IERC20(token1).transferFrom(msg.sender, address(this), amount1);
    }
    // ...
}

This vulnerability pattern is repeated across several functions in the PeripheryPool contract that handle token movements. The following is a list of affected functions:

• mint(...);
• increaseLiquidity(...);
• removeStagedAssets(...);
• swap(...);
• _transferTo(...), which is used by all settlement functions;

A silent failure in any of these transfers would cause the protocol's logic to diverge from the reality of token ownership, potentially leading to accounting errors and value loss.

Recommendation

Consider using OpenZeppelin's SafeERC20 library for all ERC-20 token operations to ensure that token transfers are always executed successfully or the entire transaction is rolled back, maintaining state consistency.

Resolution

The remediation is applied. commit

6.12 Dust amounts will get stuck in PeripheryPool contracts

The Skate AMM architecture involves KernelPool contracts on the Skate chain that handle all core logic, and PeripheryPool contracts on various chains that hold user funds and interact with the kernel. A key design choice is that the KernelPool operates with token amounts that are normalized to 18 decimals for its internal calculations, regardless of the token's actual decimals.

The PeripheryPool contract is responsible for converting token amounts between their native decimal representation and the normalized 18-decimal representation used by the kernel. This is done via the _normalizeTokenAmount(...) and _denormalizeTokenAmount(...) functions.

The _denormalizeTokenAmount(...) function converts a normalized 18-decimal amount back to the token's native decimal amount by dividing by 10 ** (18 - tokenDecimals).

function _denormalizeTokenAmount(
    uint256 amount,
    address token
)
    private
    view
    returns (uint256)
{
    // @audit-issue Precision is lost here due to integer division.
    return amount / 10 ** (NORMALIZED_DECIMAL - IERC20Metadata(token).decimals());
}

This function is used in _transferTo(...) before transferring funds out to users.

function _transferTo(address user, uint256 amount0, uint256 amount1) internal {
    uint256 amount0Denormalized = _denormalizeTokenAmount(amount0, token0);
    uint256 amount1Denormalized = _denormalizeTokenAmount(amount1, token1);
    (uint256 amount0Available, uint256 amount1Available) = balancesAvailable();
    // ...
    if (amount0Denormalized != 0) IERC20(token0).transfer(user, amount0Denormalized);
    if (amount1Denormalized != 0) IERC20(token1).transfer(user, amount1Denormalized);
    // ...
}

The problem is that the integer division in _denormalizeTokenAmount(...) truncates any remainder. For tokens with fewer than 18 decimals, any normalized amount calculated by the KernelPool that is less than 10 ** (18 - tokenDecimals) will be rounded down to zero when denormalized. The KernelPool performs high-precision calculations for swap outputs, fees, and liquidity provider shares. When these precise, normalized amounts are sent to the PeripheryPool to be paid out, the truncated remainder (dust) is not transferred. However, this dust amount was accounted for in the KernelPool's state. As a result, these small amounts of tokens will accumulate in the PeripheryPool contract over time.

Since there is no function to withdraw these residual dust balances, they will be permanently locked in the contract, leading to a gradual but irreversible loss of funds for the protocol's users.

The same issue is present in the Solana's Periphery Pool program.

Recommendation

Consider implementing a mechanism to track the accumulated dust amounts. A privileged function could be added to allow a manager or owner role to periodically sweep these dust funds to a designated treasury or DAO address. This would prevent the funds from being permanently locked and allow for their recovery.

Resolution

The remediation is applied. commit

Fixed in bb8f07f

Update from Nethermind Security

The issue is still present in the Solana's Periphery Pool program.

6.13 Exact output swaps may revert unexpectedly when a price limit is reached

The Skate AMM is designed to mirror the core mathematical logic of Uniswap V3. In an "exact output" swap, a user specifies the precise amount of tokens they wish to receive ( amountOut) and a maximum amount of input tokens they are willing to pay. Users can also set a price limit ( sqrtPriceLimitX96) to protect against unfavorable price movements.

In the standard Uniswap V3 implementation, if an exact output swap hits the specified price limit before the full amountOut has been acquired, the swap is not reverted. Instead, it completes with the amount of output tokens that could be obtained before the price limit was reached. The actual amount received will be less than the amount requested.

However, the swap(...) function in the KernelManagerLib contract enforces a stricter requirement. After the underlying KernelPool.swap(...) call is completed, it checks if the actual amount of output tokens is greater than or equal to the amount the user originally requested.

// src/amm/kernel/libraries/KernelManagerLib.sol
function swap(
    DataTypes.State storage state,
    bytes calldata swapData
)
    external
    returns (IMessageBox.Task[] memory tasks)
{
    // ...
    (vars.amount0Signed, vars.amount1Signed, vars.sqrtPriceX96, vars.liquidity, vars.tick) =
    IKernelPool(kernelPool).swap(
        _recipient, zeroForOne, amountSpecified, sqrtPriceLimitX96, extraData, false
    );

    (uint256 minAmountOutOrMaxAmountIn,,) = abi.decode(extraData, (uint256, uint256, uint256));
    vars.exactIn = amountSpecified > 0;
    if (zeroForOne) {
        vars.amount0ToSettle =
            vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
        vars.amount1ToTransfer = uint256(-vars.amount1Signed);
        vars.amount0ToTransfer = vars.amount0ToSettle - uint256(vars.amount0Signed);
    } else {
        vars.amount0ToTransfer = uint256(-vars.amount0Signed);
        vars.amount1ToSettle =
            vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
        vars.amount1ToTransfer = vars.amount1ToSettle - uint256(vars.amount1Signed);
    }

    // @audit-issue This check is stricter than Uniswap V3's.
    require(
        (zeroForOne ? vars.amount1ToTransfer : vars.amount0ToTransfer)
            >= (vars.exactIn ? minAmountOutOrMaxAmountIn : uint256(-amountSpecified)),
        "PeripheryPool::swap: slippage check"
    );
    // ...
}

Because the underlying KernelPool inherits Uniswap V3's behavior, it can return an actual output amount ( uint256(-vars.amount1Signed) or uint256(-vars.amount0Signed)) that is less than the requested amount ( uint256(-amountSpecified)) when a price limit is hit. In such cases, the require statement in KernelManagerLib will fail, causing the entire transaction to revert.

This behavior is inconsistent with the established Uniswap V3 pattern, where such a swap would partially succeed. This can lead to failed transactions and a degraded user experience, particularly during periods of high market volatility when price limits are more likely to be triggered.

Recommendation

Consider revisiting the logic that handles exact output swaps. The current implementation enforces that the entire requested output amount must be fulfilled, which causes swaps to revert if a price limit is hit prematurely. This behavior is inconsistent with the underlying Uniswap V3 protocol, which would allow for a partial fulfillment in such scenarios. Re-evaluating this strict requirement could prevent unexpected transaction failures and provide a more consistent and user-friendly experience, especially in volatile market conditions.

Resolution

The check was removed as part of the finding "Swap behavior deviates from Uniswap V3 regarding sqrtPriceLimitX96" and the correct check against "maxInputAmount" is introduced.

6.14 Fee Collection Mechanism Lacks Granularity

In the Uniswap V3 protocol, the collect(...) function allows users to specify amount0Max and amount1Max, providing granular control over fee withdrawals. This enables users to perform partial fee collections, such as withdrawing only one of the two token fees or claiming a specific amount.

The Skate protocol consolidates fee collection into the decreaseLiquidity(...) function, which is triggered with a liquidityAmount of 0. When this function is used for fee collection, the underlying burn(...) call returns zero for both token amounts. This design requires the user to pass amount0Min = 0 and amount1Min = 0 to satisfy the slippage check.

Following this, the logic in KernelManagerLib.decreaseLiquidity(...) proceeds to collect the _entirety_ of the fees owed to the position.

// src/amm/kernel/libraries/KernelManagerLib.sol
function decreaseLiquidity(
    DataTypes.State storage state,
    bytes calldata decreaseLiquidityData
)
    // ...
{
    // ...
    // @audit When liquidityAmount is 0, vars.amount0 and vars.amount1 will be 0.
    (vars.amount0, vars.amount1) = vars.pool.burn(
        address(this), position.tickLower, position.tickUpper, liquidityAmount, false
    );
    (vars.amount0Min, vars.amount1Min) = abi.decode(extraData, (uint256, uint256));

    // @audit This check forces amount{0,1}Min to be 0 when collecting fees.
    require(
        vars.amount0 >= vars.amount0Min && vars.amount1 >= vars.amount1Min,
        "price slippage check"
    );

    // @audit-issue The subsequent collect call withdraws all available fees, offering no user control.
    (vars.amount0Collected, vars.amount1Collected) = vars.pool.collect(
        address(this),
        position.tickLower,
        position.tickUpper,
        position.tokensOwed0,
        position.tokensOwed1
    );
    // ...
}

This implementation results in an "all-or-nothing" fee withdrawal mechanism. Users do not have the ability to perform partial fee collections. Every call to collect fees withdraws the full amount of both tokens owed. This is a functional difference from the Uniswap V3 protocol, which offers more flexibility in managing accrued fees.

The same issue is present in the Solana's Periphery Pool program.

Recommendation

Consider revisiting the fee collection design to provide users with more granular control over withdrawing their accrued fees. This would better align with the flexibility offered by the original Uniswap V3 protocol and enhance the user experience for position managers.

Resolution

To keep the withdrawal simpler in cross-chain environment we will keep the current approach and consider incorporting granularity in the next versions.

6.15 Incorrect accounting of global staged assets in swap(...)

The PeripheryPool contract maintains two levels of accounting for assets staged by users for various actions: a per-user accounting within the UserDataAccount PDA, and a global accounting within the PeripheryPool account state itself ( staged_token0_amount and staged_token1_amount).

Functions like mint(...) and increase_liquidity(...) correctly increment both the per-user and the global staged asset counters.

However, the swap(...) function only updates the per-user UserDataAccount and fails to increment the global staged_token0_amount or staged_token1_amount counters in the PeripheryPool.

pub fn swap(
    // ...
) -> Result<()> {
    // ...
    let user_data: &mut Account<'_, UserDataAccount> = &mut ctx.accounts.user_data;
    // @audit-issue Global staged asset counters are NOT incremented here.
    if zero_for_one {
        // @audit Only the per-user amount is incremented.
        user_data.amount0 = user_data
            .amount0
            .checked_add(amount_in)
            .ok_or(PeripheryError::Overflow)?;
    } else {
        // @audit Only the per-user amount is incremented.
        user_data.amount1 = user_data
            .amount1
            .checked_add(amount_in)
            .ok_or(PeripheryError::Overflow)?;
    }
    // ...
}

The remove_staged_assets(...) function, which is intended to be called when an action fails one the kernel side, decrements both the per-user and global counters. If a swap action fails and this function is called, it will attempt to decrement the global counters by an amount they were never incremented by. This will cause an underflow. The issue is masked because the code uses .unwrap_or(0), which prevents a panic but sets the global counter to 0, leading to an incorrect state.

pub fn remove_staged_assets(ctx: Context<RemoveStagedAssetsCtx>) -> Result<()> {
    // ...
    let amount0 = user_data.amount0;
    let amount1 = user_data.amount1;
    // ...
    let pool = &mut ctx.accounts.periphery_pool;
    // @audit-issue This will underflow if a swap fails, but `unwrap_or(0)` masks the error.
    pool.staged_token0_amount = pool.staged_token0_amount.checked_sub(amount0).unwrap_or(0);
    // @audit-issue This will underflow if a swap fails, but `unwrap_or(0)` masks the error.
    pool.staged_token1_amount = pool.staged_token1_amount.checked_sub(amount1).unwrap_or(0);
    // ...
}

While this logic discrepancy does not cause a direct loss of funds or a denial-of-service due to the unwrap_or(0) pattern, it results in inconsistent state within the contract. Any off-chain services or monitoring tools that rely on the staged_token0_amount and staged_token1_amount values from the PeripheryPool account will read incorrect data.

Recommendation

Consider either removing the global staged asset counters ( staged_token0_amount and staged_token1_amount) if they are redundant, or ensure they are consistently updated in the swap(...) function to maintain correct and reliable accounting across the protocol.

6.16 Missing Uniswap V3 standard parameters like deadline and recipient

The PeripheryPool contract is the primary entry point for users to interact with the Skate AMM on periphery chains. It exposes functions for managing liquidity ( mint(...), increaseLiquidity(...), burn(...), decreaseLiquidity(...)) and performing swaps ( swap(...)). These functions deviate from the standard Uniswap V3 implementation by omitting key parameters that provide user protection and composability.

Crucially, all of these price-sensitive functions lack a deadline parameter. In Uniswap V3, this parameter is a standard safety feature to protect users from their transactions being withheld by miners or MEV bots and executed much later at a significantly worse price. This omission exposes users to potential financial losses due to slippage from delayed execution.

Furthermore, the mint(...) function also omits the recipient parameter, which is present in Uniswap V3's implementation. This forces the newly minted position NFT to be sent to msg.sender, reducing the protocol's composability by preventing contracts or users from minting positions on behalf of others.

The same problem is present in the Solana's Periphery Pool program.

Recommendation

Consider aligning the function signatures in PeripheryPool with the Uniswap V3 standards to enhance user security and protocol composability.

Resolution

The remediation is applied. commit and solana commit

Update from Nethermind Security

The burn(...) and burn_legacy(...) instructions in the Solana's Periphery Pool program is missing the deadline parameter.

6.17 Only NFT position owner can increase liquidity

The KernelManager contract facilitates the management of liquidity positions, which are represented as NFTs. The increaseLiquidity(...) function allows users to add more funds to their existing positions.

However, the current implementation in the KernelManagerLib contract restricts this action exclusively to the owner of the position NFT. The function verifies that the address initiating the transaction is the owner of the tokenId corresponding to the liquidity position.

function increaseLiquidity(
    DataTypes.State storage state,
    bytes calldata increaseLiquidityData
)
    external
    returns (IMessageBox.Task[] memory tasks)
{
    // ...
    (
        // ...
        bytes32 user,
        // ...
        uint256 tokenId,
        // ...
    ) = abi.decode(
        increaseLiquidityData,
        (bytes32, uint256, uint256, bytes32, address, uint256, uint256, uint256, bytes)
    );
    IncreaseLiquidityLocalVars memory vars;
    vars.user = _fromBytes32ToAddress(user);
    // @audit-issue This check requires the caller to be the owner of the NFT position.
    require(
        IKernelManager(address(this)).ownerOf(tokenId) == vars.user,
        "KernelManagerLib::increaseLiquidity: not owner of tokenId"
    );
    // ...
}

This design choice differs from other similar AMM protocols where anyone can "donate" or add liquidity to any existing position. This restriction can be limiting in several scenarios. For instance, a user who holds their position NFT in a secure cold wallet cannot add liquidity using a more convenient hot wallet. Furthermore, it prevents the development of third-party services or integrations that might want to top up user positions on their behalf.

Recommendation

Consider re-evaluating whether this ownership check is a strict product requirement or a precautionary measure. Allowing any address to increase the liquidity of a position could provide greater flexibility for both users and external integrators, enhancing the composability of the protocol.

Resolution

The remediation is applied. commit

6.18 Potential actionId collision in collectProtocol(...)

The collectProtocol(...) function in the KernelManagerLib library is used by the KernelManager contract owner to withdraw accrued protocol fees. This process generates a task with a corresponding actionId that is intended to be unique for off-chain processing.

The actionId is calculated by hashing a combination of a fixed string ("collectProtocol"), the recipient, the kernelPool address, and the current block.number.

// src/amm/kernel/libraries/KernelManagerLib.sol
// ...
    tasks[0] = IMessageBox.Task({
        // ...
        actionId: keccak256(
            // @audit-issue The hash only depends on recipient, kernelPool, and block.number.
            abi.encodePacked("collectProtocol", recipient, kernelPool, block.number)
        )
    });
}

However, this hashing mechanism does not guarantee uniqueness if the function is called multiple times within the same block with the same recipient and kernelPool parameters. In such a scenario, all generated tasks will have an identical actionId.

This hash collision could lead to unexpected behavior in off-chain systems that rely on the uniqueness of actionId for task tracking and execution. For instance, an off-chain service might process the first task and ignore subsequent ones with the same ID, causing the owner's later fee collection attempts within the same block to be missed.

Recommendation

Consider incorporating a nonce or another source of entropy into the actionId calculation to ensure its uniqueness across multiple calls within the same block.

Resolution

The remediation is applied. commit

6.19 Refunded tokens from swaps are sent to the recipient address

In the Skate AMM, users can initiate a swap by calling the swap(...) function on a PeripheryPool contract. This function allows specifying a recipient address that will receive the output tokens from the trade.

Unlike typical Uniswap V3-style swaps where only the exact required amount of input tokens is pulled from the user, the Skate AMM pulls the maximum specified input amount (the amountSpecified for exact-in swaps, or the maximum input amount from extraData for exact-out swaps) and stages it. After the swap is processed by the KernelManager on the Skate chain, any unused portion of the input token is refunded.

The issue is that these refunded input tokens are sent to the recipient of the swap, not to the original user who initiated the transaction and provided the funds. The calculation of the refunded amount and the swapped amount happens within the swap(...) function in the KernelManagerLib contract.

// src/amm/kernel/libraries/KernelManagerLib.sol
function swap(...) external returns (IMessageBox.Task[] memory tasks) {
    // ...
    if (zeroForOne) {
        // @audit The total input amount from the user.
        vars.amount0ToSettle =
            vars.exactIn ? uint256(amountSpecified) : minAmountOutOrMaxAmountIn;
        // @audit The amount of token1 to transfer to the recipient (swapped amount).
        vars.amount1ToTransfer = uint256(-vars.amount1Signed);
        // @audit The amount of token0 to transfer back (refunded amount).
        vars.amount0ToTransfer = vars.amount0ToSettle - uint256(vars.amount0Signed);
    } // ... else block for oneForZero swap has similar logic.

    // ...
    tasks = new IMessageBox.Task[](1);
    tasks[0] = IMessageBox.Task({
        appAddress: appAddress,
        taskCalldata: abi.encodeWithSignature(
            "settleSwap(bytes32,bytes32,uint256,uint256,uint256,uint256)",
            recipient,
            user,
            vars.amount0ToTransfer, // @audit This includes the refunded amount.
            vars.amount1ToTransfer, // @audit This includes the swapped amount.
            vars.amount0ToSettle,
            vars.amount1ToSettle
        ),
        // ...
    });
}

The settleSwap(...) function in the PeripheryPool contract then calls _transferTo(...), which sends both the swapped tokens and the refunded input tokens to the recipient.

// src/amm/periphery/PeripheryPool.sol
function settleSwap(
    bytes32 recipient,
    bytes32 user, // @audit-issue Unused in the transfer logic.
    uint256 amount0ToTransfer,
    uint256 amount1ToTransfer,
    // ...
)
    external
    override
    onlyGateway
{
    // ...
    // @audit-issue The `recipient` receives both the swapped tokens and any refund.
    _transferTo(_fromBytes32ToAddress(recipient), amount0ToTransfer, amount1ToTransfer);
}

This behavior is counterintuitive. A user might specify a recipient contract for a swap, expecting it to only receive the output token. However, the contract will also receive the refunded input tokens. If the recipient contract is not designed to handle these unexpected tokens, the funds could become stuck, leading to a loss for the user.

The same issue is present in the Solana's Periphery Pool program.

Recommendation

Consider documenting this behavior so that users and developers are aware that the recipient address will receive both the output of the swap and any refunded input tokens. Alternatively, consider modifying the logic to send the refunded tokens back to the account that originally provided the tokens for the swap. If cross-chain swaps are implemented in the future, this behavior might need to be revisited depending on the chosen implementation.

Resolution

The remediation is applied. commit and solana commit

6.20 Swap behavior deviates from Uniswap V3 regarding sqrtPriceLimitX96

Users can initiate a token swap by calling the swap(...) function on the PeripheryPool contract. This action is then relayed to the KernelManager, which processes the intent via the KernelManagerLib.swap(...) library function. Finally, KernelManagerLib calls the swap(...) function on the corresponding KernelPool to execute the swap on the Skate chain.

The KernelPool.swap(...) function, which contains the core swap logic, requires a sqrtPriceLimitX96 parameter to protect against slippage. This parameter must be a valid, non-zero value that is on the correct side of the current price, otherwise the transaction will revert.

The issue is that the periphery layer of the Skate AMM ( PeripheryPool and KernelManagerLib) does not handle the sqrtPriceLimitX96 parameter in the same user-friendly manner as Uniswap V3's SwapRouter. Specifically, it's passed directly to the KernelPool.

// src/amm/kernel/libraries/KernelManagerLib.sol
function swap(
    DataTypes.State storage state,
    bytes calldata swapData
) external returns (IMessageBox.Task[] memory tasks) {
    // ...
    (vars.amount0Signed, vars.amount1Signed, vars.sqrtPriceX96, vars.liquidity, vars.tick) =
        // @audit-issue The `sqrtPriceLimitX96` is passed directly to the KernelPool.
        IKernelPool(kernelPool).swap(
            _recipient, zeroForOne, amountSpecified, sqrtPriceLimitX96, extraData, false
        );
    // ...
}

This implementation differs from Uniswap V3's SwapRouter in two key ways:

1. Handling of Zero Price Limit: In Uniswap V3's SwapRouter, if a user provides a sqrtPriceLimitX96 of 0, the contract automatically substitutes it with the safest possible price limits ( TickMath.MIN_SQRT_RATIO + 1 or TickMath.MAX_SQRT_RATIO - 1). This allows users to perform swaps without needing to calculate a precise limit, making the interface more robust and easier to use. The Skate AMM lacks this feature, meaning a sqrtPriceLimitX96 of 0 will cause the KernelPool.swap(...) call to revert;

2. Exact Output Guarantee: For exact output swaps, Uniswap V3's SwapRouter includes an on-chain check that verifies the user receives the exact requested amount of tokens if no price limit ( sqrtPriceLimitX96 == 0) is specified. This provides an important safety guarantee against slippage. The Skate AMM's KernelManagerLib does not perform this check.

// src/UniswapV3/periphery/SwapRouter.sol
function exactOutputInternal(/*...*/) private returns (uint256 amountIn) {
    // ...
    (int256 amount0Delta, int256 amount1Delta) =
        getPool(tokenIn, tokenOut, fee).swap(
            // ...
            // @audit The router provides default price limits if none are specified.
            sqrtPriceLimitX96 == 0
                ? (zeroForOne ? TickMath.MIN_SQRT_RATIO + 1 : TickMath.MAX_SQRT_RATIO - 1)
                : sqrtPriceLimitX96,
            // ...
        );
    // ...
    // @audit-issue This check ensures the user receives the exact output amount.
    if (sqrtPriceLimitX96 == 0) require(amountOutReceived == amountOut);
}

By not implementing these features from the Uniswap V3 SwapRouter, the Skate AMM delegates the responsibility of calculating and providing a valid sqrtPriceLimitX96 to the off-chain clients or users for every swap. It also removes the on-chain guarantee for exact output swaps when no price limit is provided.

Recommendation

Consider aligning the KernelManagerLib.swap(...) function's behavior with that of Uniswap V3's SwapRouter. This would involve:

1. Checking if the sqrtPriceLimitX96 parameter is 0. If so, replace it with the appropriate default value ( TickMath.MIN_SQRT_RATIO + 1 or TickMath.MAX_SQRT_RATIO - 1) based on the swap direction before calling KernelPool.swap(...);
2. For exact output swaps where sqrtPriceLimitX96 is 0, add a requirement to ensure that the amount of tokens sent to the user matches the requested output amount;

Resolution

The remediation is applied. commit

6.21 The _normalizeTokenAmount(...) function does not handle decimals larger than 18

The PeripheryPool contract is responsible for handling user interactions on periphery chains. A core part of its logic is to normalize all token amounts to a standard 18 decimal precision before relaying them to the main KernelManager on the Skate chain. This is handled by the internal _normalizeTokenAmount(...) function.

The current implementation of this function does not support tokens with more than 18 decimals. When a token with, for example, 20 decimals is used, the calculation NORMALIZED_DECIMAL - IERC20Metadata(token).decimals() will attempt to compute 18 - 20, which results in an arithmetic underflow, causing the transaction to revert.

function _normalizeTokenAmount(
    uint256 amount,
    address token
)
    private
    view
    returns (uint256)
{
    // @audit-issue The normalization will revert for tokens with
    // more than 18 decimals due to an underflow.
    return amount
        * 10 ** (NORMALIZED_DECIMAL - IERC20Metadata(token).decimals());
}

This issue affects all functions that rely on this normalization logic, such as mint(...), increaseLiquidity(...), and swap(...), effectively preventing the use of any token with more than 18 decimals within the protocol.

Recommendation

Although tokens with more than 18 decimals are uncommon, if they are expected to be used within the protocol, consider adjusting the _normalizeTokenAmount(...) function to handle this case. Note that the _denormalizeTokenAmount(...) function must be adjusted as well to ensure amounts are converted back correctly.

Resolution

The remediation is applied. commit

6.22 The burn(...) function lacks a check for available liquidity

The burn(...) function allows a user to initiate the burning of their liquidity position. It accepts amount0min and amount1min as parameters, which act as slippage protection by defining the minimum amount of each token the user is willing to receive. The function then constructs a KernelAction::Burn and dispatches it via the action_box program for off-chain processing.

However, the burn(...) function does not validate whether the pool has sufficient token balances to satisfy the amount0min and amount1min requirements before creating the action.

pub fn burn(
    ctx: Context<BurnCtx>,
    tick_lower: i32,
    tick_upper: i32,
    liquidity_amount: u128,
    action_box_seed: u64,
    amount0min: u64,
    amount1min: u64,
    token_id: String
) -> Result<()> {
    // @audit-issue There is no check to ensure that the pool has enough liquidity
    // to satisfy `amount0min` and `amount1min` before proceeding.

    let user_bytes32 = pubkey_to_bytes32(ctx.accounts.user.key());
    let action_id = generate_action_id(user_bytes32.clone(), ...)?;
    let kernel_action = KernelAction::Burn {
        action_id: action_id.clone(),
        chain_id: ctx.accounts.periphery_pool.chain_id,
        // ...
        amount0min,
        amount1min,
        token_id: token_id.clone(),
    };
    // ...
    // @audit An action is created even if the pool cannot fulfill the minimum amounts.
    action_box::cpi::create_action_v2(
        cpi_ctx,
        // ...
    )?;

    // ...
    Ok(())
}

This is inconsistent with the decrease_liquidity(...) function, which performs an equivalent check using the balances_available(...).

pub fn decrease_liquidity(
    ctx: Context<DecreaseLiquidityCtx>,
    // ...
    amount0min: u64,
    amount1min: u64,
    // ...
) -> Result<()> {
    // @audit This function correctly checks for available balances.
    let (amount0_available, amount1_available) = balances_available(
        &ctx.accounts.pool_token0,
        &ctx.accounts.pool_token1,
    )?;
    require!(
        amount0min <= amount0_available && amount1min <= amount1_available,
        PeripheryError::InsufficientLiquidity
    );
    // ...
}

This omission allows a transaction to succeed and an action to be created, even when the pool's liquidity is insufficient to meet the user's minimum requirements. An off-chain executor will identify that the pool's balances cannot fulfill the amount0min and amount1min for settlement. Instead of executing the action in kernel and then calling settle_burn(...), which would fail, the executor's likely recourse is to call remove_staged_assets(...) to refund the user. This results in a failed user flow where the initial burn transaction succeeded but did not result in a successful liquidity removal. It leads to a poor user experience, wastes the user's transaction fees, and relies on a more complex and potentially delayed recovery path.

Recommendation

Consider adding a balance check at the beginning of the burn(...) function to verify that the pool has sufficient liquidity to meet the amount0min and amount1min requirements, similar to the logic in the decrease_liquidity(...) function.

6.23 The changePeripheryPool(...) function can add new periphery pools

The KernelManager contract distinguishes between two roles for managing periphery pools: the poolCreator and the owner. The poolCreator can add a new periphery pool for a specific kernelPool and chainId by calling the addPeripheryPool(...) function. This function correctly ensures that a periphery pool does not already exist for the given chainId.

// src/skate/kernel/libraries/KernelManagerLib.sol
function addPeripheryPool(/*...*/)
    external
{
    // @audit Ensures that a pool does not already exist for this chainId.
    require(
        state.dataByPool[kernelPool].peripheryPoolByChainId[chainId].pool
            == bytes32(0x0),
        "periphery pool already added"
    );
    // ...
    state.dataByPool[kernelPool].peripheryPoolByChainId[chainId] = poolData;
    state.eventEmitter.peripheryPoolAdded(
        kernelPool, chainId, pool, token0, token1
    );
}

The owner, however, can call the changePeripheryPool(...) function. This function is intended to modify an existing periphery pool. However, it lacks a check to verify that a pool for the given chainId already exists. This allows the owner to use this function to add a new periphery pool, effectively giving it the same functionality as addPeripheryPool(...) but with a different access level.

// src/skate/kernel/libraries/KernelManagerLib.sol
function changePeripheryPool(/*...*/)
    external
{
    // @audit-issue No check to ensure a periphery pool already exists.
    DataTypes.PeripheryPool memory poolData =
        DataTypes.PeripheryPool(pool, token0, token1);

    state.dataByPool[kernelPool].peripheryPoolByChainId[chainId] = poolData;

    // @audit-issue Emits a `peripheryPoolAdded` event, not a "changed" event.
    state.eventEmitter.peripheryPoolAdded(
        kernelPool, chainId, pool, token0, token1
    );
}

This dual functionality is not documented and can be misleading. The function's name implies modification, not creation. Furthermore, the function emits a peripheryPoolAdded event, which is inaccurate when the function is used to change an existing pool's data. This ambiguity can make the codebase harder to understand and maintain.

Recommendation

Consider enforcing a clear separation of concerns between adding and changing periphery pools.

Resolution

The remediation is applied. commit

6.24 The createPool function does not prevent creating a pool with identical tokens

The createPool(...) function in the PeripheryManager contract is used by the poolCreator to deploy new periphery AMM pools. However, the function lacks a check to ensure that the two token addresses provided, token0 and token1, are not the same.

This allows for the creation of a pool where both underlying assets are identical. Such a configuration serves no practical purpose in an AMM and represents an invalid state. Standard implementations, such as Uniswap V3's factory, explicitly prevent this scenario. While the poolCreator is a trusted role, incorporating this validation would harden the contract against accidental misconfigurations and align it with established best practices, thereby reducing the potential for errors.

Recommendation

Consider adding a validation check at the beginning of the createPool(...) function to ensure that token0 is not equal to token1.

Resolution

The remediation is applied. commit

6.25 The settle_swap(...) function does not utilize the recipient for settlement

The swap(...) function allows a user to specify a recipient for the swapped tokens, which can be different from the user initiating the transaction. Both the initiator's and the recipient's addresses are encoded and sent to the kernel chain for processing.

After the kernel processes the swap, an off-chain executor calls the settle_swap(...) function. The context for this function, SettleSwapCtx, derives the user_data account to be settled based on the user account provided in the instruction's accounts.

#[derive(Accounts)]
pub struct SettleSwapCtx<'info> {
    // ...
    // @audit The user_data account is derived from the `user` account,
    // which is described as the one receiving tokens in the comment below.
    #[account(
        mut,
        seeds = [b"user-data", user.key().as_ref(), periphery_pool.key().as_ref()],
        bump,
    )]
    pub user_data: Account<'info, UserDataAccount>,
    // ...
    // @audit-issue The user could be the recipient, not necessarily the swap initiator.
    /// CHECK: user who is receiving tokens / being settled
    pub user: UncheckedAccount<'info>,
}

As confirmed by the client during a sync call, the off-chain executor service always passes the swap initiator's address and token accounts to the settle_swap instruction, regardless of the recipient value specified in the initial swap call. As a result, the swap output is always transferred to the initiator, and the initiator's staged assets are the ones settled.

This effectively renders the recipient parameter in the swap function non-functional, as the specified recipient will never receive the funds. The comment in SettleSwapCtx is also misleading, as it suggests the user is the one receiving tokens, which is only true because the off-chain logic ignores the intended recipient. This behavior appears to be a workaround, as passing a recipient who has never interacted with the pool would cause the instruction to revert because their user_data account may not exist.

Recommendation

Consider either implementing the full logic to correctly handle the recipient address during the settle_swap process (which would involve passing the recipient's token accounts) or removing the recipient parameter from the swap function to avoid confusion.

6.26 The swap(...) function does not validate the destination VM type

The swap(...) function in the periphery_pool contract allows a user to initiate a token swap. This action is then relayed to the Kernel on the Skate chain. The function accepts dest_chain_id and dest_vm_type to specify the target for the subsequent action.

The function correctly validates that the dest_chain_id matches the current chain ID. However, it fails to validate the dest_vm_type. An incorrect dest_vm_type can be provided by the user and will be passed along in the KernelAction sent to the Kernel chain. This could lead to processing errors for backend services that rely on the vm_type to correctly handle action data.

For consistency, the Solana implementation should align with the EVM implementation, which validates both the chain ID and the VM type.

The EVM PeripheryPool contract includes the following check:

// src/amm/periphery/PeripheryPool.sol
// ...
require(
    destChainId == block.chainid && destVmType == 1,
    "PeripheryPool::swap: cross-chain swap is not supported"
);
// ...

The Solana implementation is missing the check for dest_vm_type:

// solana/src/lib.rs
// ...
/* @audit-issue The destination VM type is not validated, unlike in the EVM implementation. */
require!(
    dest_chain_id == pool.chain_id,
    PeripheryError::InvalidChainId
);
// ...

Recommendation

Consider adding a validation check in the swap(...) function to ensure that the dest_vm_type parameter corresponds to the expected value for the Solana environment. This will improve data integrity and maintain consistency with the EVM implementation.

6.27 Tokens without a decimals() function are not supported

The PeripheryPool contract includes helper functions _normalizeTokenAmount(...) and _denormalizeTokenAmount(...) to adjust token amounts to and from a normalized 18-decimal precision.

The issue is that both helper functions make a direct external call to the decimals() function on the token contract to get its decimal precision.

function _normalizeTokenAmount(uint256 amount, address token) private view returns (uint256) {
    // @audit-issue The decimals() function is optional in the ERC20 standard.
    return amount * 10 ** (NORMALIZED_DECIMAL - IERC20Metadata(token).decimals());
}

function _denormalizeTokenAmount(
    uint256 amount,
    address token
)
    private
    view
    returns (uint256)
{
    // @audit-issue The decimals() function is optional in the ERC20 standard.
    return amount / 10 ** (NORMALIZED_DECIMAL - IERC20Metadata(token).decimals());
}

However, the decimals() function is optional according to the ERC20 standard. Some valid ERC20 tokens might not implement this function. If such a token is used to create a pool, any interaction that relies on amount normalization or denormalization will revert.

Recommendation

Consider revisiting the logic to handle ERC20 tokens that do not implement the optional decimals() function. This would improve the protocol's compatibility with a wider range of tokens.

Resolution

The remediation is applied. commit

6.28 Unsafe cast to int256 in the swap(...) function

The swap(...) function in the PeripheryPool contract allows users to swap tokens. As part of this process, the input amount, amountIn, is normalized based on the token's decimals to get amountInNormalized.

The problem is that this amountInNormalized value, which is a uint256, is then unsafely cast to an int256 when constructing the kernelAppCalldata. If the amountInNormalized value exceeds the maximum value for a positive int256 (i.e., 2^255 - 1), the cast will not revert but will instead wrap around and be interpreted as a negative number due to two's complement representation.

This could lead to the kernelManager contract receiving incorrect swap data, potentially resulting in an unexpected swap outcome for the user. While the likelihood of such a large input amount is low, this behavior can become problematic, especially if combined with other vulnerabilities outlined in this report.

function swap(...) external override {
    // ...
    uint256 amountInNormalized = _normalizeTokenAmount(amountIn, tokenIn);
    // ...
    action.kernelAppCalldata = abi.encodeWithSignature(
        "swap(bytes)",
        abi.encode(
            // ...
            // @audit-issue The `amountInNormalized` (uint256) is unsafely cast to `int256`.
            // If it's larger than `type(int256).max`, it will become a negative value.
            int256(amountInNormalized),
            sqrtPriceLimitX96,
            extraData
        )
    );
    // ...
}

Recommendation

Consider implementing a safe casting mechanism. Before casting the amountInNormalized from uint256 to int256, verify that its value is within the valid range for int256.

Resolution

The casting issue was resolved as part of fix relating to normalization of amount specified.

6.29 generate_action_id(...) may return the same hash for different actions

The periphery_pool program includes several functions such as mint(...), increase_liquidity(...), burn(...), swap(...), and decrease_liquidity(...) that allow users to interact with the AMM. Each of these functions calls the internal helper function generate_action_id(...) to create a unique identifier for the specific action being executed.

The generate_action_id(...) function calculates an action_id by producing a Keccak256 hash from a combination of the user, chain_id, a user-provided seed, the signer, and the current blockchain slot.

pub fn generate_action_id(
    user: String,
    chain_id: u32,
    seed: u64,
    signer: Pubkey
) -> Result<String> {
    let clock: Clock = Clock::get()?;
    let slot = clock.slot;

    let mut hasher = Keccak256::new();
    hasher.update(user.as_bytes());

    hasher.update(&slot.to_be_bytes());
    let chain_id_u64 = chain_id as u64;
    hasher.update(&chain_id_u64.to_be_bytes());
    // @audit The seed is a user-controlled parameter.
    hasher.update(&seed.to_be_bytes());
    // @audit The signer is the deterministic pool authority PDA.
    hasher.update(&signer.to_bytes());

    let hash_result = hasher.finalize();
    Ok(hex::encode(hash_result))
}

The signer parameter in this context is the pool_authority PDA, which is deterministic for any given pool. For a specific user interacting with the same pool, the user and chain_id will also be constant. If a user's transactions are processed within the same slot, the slot value will be identical as well. The primary variable for ensuring uniqueness is the user-controlled seed.

However, if a user calls two different functions (e.g., mint(...) and increase_liquidity(...)) for the same pool within the same slot and provides the same action_box_seed for both calls, the generate_action_id(...) function will produce an identical hash. This results in two distinct user actions being assigned the same action_id. This could cause ambiguity and potential issues for off-chain systems that rely on unique action_id values for tracking and processing user actions.

Recommendation

Consider revisiting the logic for action_id generation to ensure that each distinct user action results in a unique identifier.

Resolution

Resolved in solana commit

6.30 Missing extraData validation in increaseLiquidity(...) and mint(...)

The mint(...) and increaseLiquidity(...) functions in the PeripheryPool contract are used by liquidity providers to add liquidity to a pool. Both functions accept an extraData parameter, which is intended to contain two uint256 values, amount0Min and amount1Min, for slippage protection.

The current implementation checks if extraData has a length of zero and, if so, defaults it to abi.encode(0, 0). However, it does not validate that if extraData is provided (i.e., its length is greater than zero), it has the correct size of 64 bytes to be decoded into two uint256 values.

// src/amm/periphery/PeripheryPool.sol
function mint(
    // ...
    bytes memory extraData // must contain amount0Min, amount1Min
)
    external
    override
{
    // ...
    // @audit-issue If extraData has a non-zero length but is not 64 bytes,
    // this check passes.
    if (extraData.length == 0) extraData = abi.encode(0, 0);
    // ...
    action.kernelAppCalldata = abi.encodeWithSignature(
        "mint(bytes)",
        abi.encode(
            // ...
            // @audit The potentially malformed extraData is passed to the KernelManager.
            extraData
        )
    );
    // ...
}

The logic for increaseLiquidity(...) is identical.

This malformed extraData is then passed to the KernelManager contract on the Skate chain. When the corresponding mint(...) or increaseLiquidity(...) function in KernelManagerLib is executed, it attempts to decode extraData. This abi.decode call will revert if the data does not have the expected length.

// src/amm/kernel/libraries/KernelManagerLib.sol
function _calcAmountsUsedInMintAndValidate(
    // ...
    bytes memory extraData
)
    private returns (/*...*/)
{
    // ...
    // @audit-issue This call will revert if extraData.length is not of the correct length.
    (uint256 amount0Min, uint256 amount1Min) =
        abi.decode(extraData, (uint256, uint256));
    // ...
}

This leads to a scenario where a user's transaction succeeds on the periphery chain, consuming gas and locking their tokens in the PeripheryPool contract, only to have the cross-chain operation fail later on the Skate chain. This creates a poor user experience and can cause confusion.

Recommendation

Consider adding a validation check in the mint(...) and increaseLiquidity(...) functions within the PeripheryPool contract to ensure that if extraData is not empty, its length is exactly 64 bytes. This would make the handling of extraData consistent and prevent failed transactions on the Skate chain due to malformed input.

Resolution

The remediation is applied. commit

Additional remediation is applied. commit

Update from Nethermind Security

There are two points of concern regarding the applied fix:

• The newly added check in the increaseLiquidity(...) function is insufficient as it is missing the deadline parameter. It checks for two uint256 values, while the extraData contains three;
• The sanity check that uses abi.decode in the expression statement might be removed by the Solidity compiler while optimizing the code;

6.31 The createPool(...) function does not validate if tickSpacing is configured

The poolCreator can create new kernel pools by calling the createPool(...) function in the KernelManager contract. This function takes a fee level as a parameter, which is then used to retrieve the corresponding tickSpacing for the new pool from the feeAmountTickSpacing mapping.

However, the createPool(...) function in the KernelManagerLib contract does not verify that a tickSpacing has been configured for the provided fee. If the poolCreator attempts to create a pool with a fee for which a tickSpacing has not been set, the lookup will default to 0.

This will cause the deployment of the new KernelPool proxy to fail. The pool's initialize(...) function, which is called during creation, calculates maxLiquidityPerTick by calling Tick.tickSpacingToMaxLiquidityPerTick(_tickSpacing). This function will revert with a division-by-zero error if _tickSpacing is 0, leading to a failed deployment.

function createPool(
    DataTypes.State storage state,
    string calldata description,
    uint24 fee,
    uint160 initialPrice,
    address kernelPoolImpl
)
    external
    returns (address)
{
    // ...
    address kernelPool = address(
        new ERC1967Proxy(
            address(kernelPoolImpl),
            abi.encodeWithSignature(
                "initialize(uint160,address,address,uint24,int24)",
                initialPrice,
                token0,
                token1,
                fee,
                // @audit-issue No check to ensure tickSpacing is not 0.
                state.feeAmountTickSpacing[fee]
            )
        )
    );
    // ...
}

This can lead to potential operational issues if an unconfigured fee tier is used. While this does not pose a direct threat to user funds, it goes against best practices and can cause confusion.

Recommendation

Consider adding a validation check in the createPool(...) function to ensure that the tickSpacing for the specified fee is greater than zero before attempting to create the pool. This aligns with the implementation in Uniswap V3's factory contract and prevents the creation of broken pools.

Resolution

The remediation is applied. commit

6.32 The setFeeAmountTickSpacing(...) function lacks input validation

The owner of the KernelManager contract can configure new fee tiers by calling the setFeeAmountTickSpacing(...) function. This function establishes a link between a swap fee and its corresponding tickSpacing.

The current implementation lacks any validation on the fee and spacing inputs. This allows the owner to set a tickSpacing of 0 or an excessively large value, which can be configured for any given fee.

function setFeeAmountTickSpacing(
    DataTypes.State storage state,
    uint24 fee,
    int24 spacing
)
    external
{
    // @audit-issue No validation on fee or spacing parameters.
    state.feeAmountTickSpacing[fee] = spacing;
}

The KernelPool contract relies on Uniswap V3's math libraries for its core operations. These libraries, particularly those related to tick bitmaps and calculations, operate under the assumption that tickSpacing is a positive, reasonably-sized number. For instance, Uniswap V3's implementation caps the tickSpacing to prevent potential overflows in functions like TickBitmap.nextInitializedTickWithinOneWord. Allowing arbitrary values could lead to unexpected reverts or incorrect calculations during swaps and liquidity management operations.

Recommendation

Consider introducing input validation for the fee and spacing parameters in the setFeeAmountTickSpacing(...) function. It is recommended to constrain the tickSpacing to a safe, positive range that is compatible with the underlying Uniswap V3 math libraries used by the KernelPool, for example by ensuring spacing is greater than 0 and less than a reasonable upper bound.

Resolution

The remediation is applied. commit

6.33 Usage of tx.origin for authorization is unsafe

The submitTasks(...) function in the MessageBox contract is responsible for processing user intents submitted by off-chain executors. Authorization is performed by checking if tx.origin is a whitelisted executor.

Relying on tx.origin for authorization can make contracts vulnerable to phishing attacks. If a whitelisted executor is lured into calling a malicious contract, that contract could then call submitTasks(...) on the executor's behalf. Since tx.origin would still be the executor's address, the malicious call would pass the authorization check, potentially leading to the submission of unauthorized tasks.

function submitTasks(
    IMessageBox.Task[] calldata tasks,
    IMessageBox.Intent calldata intent
)
    external
    override
{
    // @audit-issue The check relies on tx.origin, which can be manipulated in phishing attacks.
    require(_executorRegistry.isExecutor(tx.origin), NotAnExecutor(tx.origin));
    require(msg.sender == intent.intentData.appAddress, IntentIsNotSignedForTheApp());
    // ...
}

Recommendation

Consider replacing the tx.origin check with a msg.sender check. Since submitTasks(...) is intended to be called by SkateApp contracts, the authorization logic could be moved to the processIntent(...) function within the SkateApp contract, verifying that msg.sender is a whitelisted executor.

Resolution

We won't be making changes to the current architecture in this version. For the current version, we are running the executors and the executor interaction would be only be through a set of applications. Additionally, the off-chain process also validates the authenticity of applications emitting the tasks. The design-wise, we did not want to give this authorization control to the applications since they can make modification to avoid it e.g. if this check is moved to "processIntent" function, the application logic can override the function and circumvent this authorization check.