Introduction

Welcome to the first post in our new monthly series, where we share five of the most intriguing findings from recent audits. In each article, you’ll discover common pitfalls, unique vulnerabilities, and practical fixes. Our goal is not only to help make protocols safer but also to educate more teams and auditors so we can all work together toward a safer ecosystem.

This article identifies key issues in different protocols interacting with Uniswap V3 liquidity positions and cross-chain swaps, including withdrawal limitations, reward distribution inefficiencies, and slippage vulnerabilities. We outline these problems and provide solutions to improve protocol security.

Issue 1: Lack of withdrawal mechanism for deposited Uni V3 NFT liquidity positions

Impact: Medium

Likelihood: High

Description

The protocol generates yield for stakers by locking Uniswap V3 liquidity position NFTs.There is a function that lets the NFT owner to add more liquidity to the position.

mapping(uint256 => address) public nftOwners;

...

function depositNFT(uint256 tokenId) external {
    uniV3PositionNft.safeTransferFrom(msg.sender, address(this), tokenId);
    nftOwners[tokenId] = msg.sender;
}

function addLiquidity(uint256 tokenId, uint256 amount0, uint256 amount1) external {
    require(nftOwners[tokenId] == msg.sender, "Not the NFT owner");
    // Logic to add liquidity
}

However, there is no function that lets the owner decrease position liquidity or to reclaim the NFT from the contract.

Without a withdrawal mechanism, once an NFT liquidity position is deposited, it becomes permanently locked within the contract, restricting liquidity management and preventing repositioning.

Recommendations

Introduce a withdrawal function that allows the owner to retrieve deposited liquidity positions. Additionally, consider adding functionality to partially decrease liquidity from existing positions.

function withdrawNFT(uint256 tokenId) external {
    require(ownerOf(tokenId) == msg.sender, "Not the NFT owner");
    nft.safeTransferFrom(address(this), msg.sender, tokenId);
}

Issue 2: Undistributed rewards may be stuck in the contract if no stakers exist in a cycle

Impact: Medium

Likelihood: Medium

Description

StakeVault is the contract responsible for receiving yield rewards from the RewardGenerator, where staked positions are held. These rewards are subsequently distributed equally among pools for the current cycle, ensuring that each staking pool receives a fair share of the generated yield.

The system operates in 30-day cycles, with a function that increments the cycle if the reward time has elapsed.

function incrementCycle() public returns (bool) {
    if (block.timestamp < cycleStartTime + CYCLE_DURATION) {
        // Cycle duration not met, so exit without reverting.
        return false;
    }

    currentCycleId += 1;
    cycleStartTime = block.timestamp;

    emit CycleIncremented(currentCycleId, cycleStartTime);
    return true;
}

This function is invoked during reward distribution, allowing each cycle to maintain its own independent reward pool. Additionally, each cycle tracks its own staked amount through shares, which are generated by stakers when they deposit their positions.

A crucial issue arises when no stakers participate in a particular cycle. In this scenario, the rewards allocated for that cycle become permanently stuck in the contract. This occurs because the mechanism for handling rewards does not account for cycles without stakers—the undistributed rewards are not carried over to the next cycle. In contrast, cycle shares are properly migrated forward when a new cycle is initiated.

As a result, any unclaimed rewards from empty cycles can accumulate, leading to a growing pool of inaccessible funds. This behavior not only causes reward leakage, but also prevents future cycles from utilizing or redistributing these locked rewards.

Recommendation

Implement a sweep() function to rescue stuck rewards from cycles with no stakers or transfer rewards to next cycle when incrementing.

function sweepUnclaimedRewards(uint256 cycleId) external onlyAdmin {
    if (cycleShares[cycleId] != 0) revert("Cycle has stakers");

    uint256 unclaimed = cycleRewards[cycleId];
    require(unclaimed > 0, "No unclaimed rewards");
    cycleRewards[currentCycleId] += unclaimed;
    delete cycleRewards[cycleId];
}

Issue 3: Addressing allocation point misconfiguration in liquidity manager

Impact: High

Likelihood: Medium

Description

The LiquidityFarmManager contract plays a crucial role within the protocol, specifically designed to manage Uni V3 liquidity farms with a focus on stable assets. By leveraging Uni V3’s concentrated liquidity model, it enables efficient liquidity provision within tight price ranges, such as maintaining a 1:1 peg for stablecoin pairs.

One of its primary functions is to allocate incentive tokens based on liquidity share and time within the farm. However, an issue has been identified in the registerFarm function, which may lead to incorrect reward distribution due to improper handling of allocation points.

The registerFarm function allows the creation and configuration of new farms within the protocol. It ensures that the first farm must have allocation points set correctly, as totalAllocPoints is initially zero. However, for subsequent farms, there is no restriction preventing the addition of a new farm with zero allocation points.

When a farm is registered with zero allocation points, the function incorrectly sets lastRewardTime to zero. As a result:

1. lastRewardTime remains at zero indefinitely.

2. Accumulates rewards incorrectly due to the misconfigured lastRewardTime.

3. When allocation points are finally assigned and liquidity is added, the updateFarm function incorporates past rewards, leading to incorrect distribution of incentives.

Code Snippet of the Issue

  function registerFarm(AddFarmParams calldata params) external restricted {
    ...
    // Ensure valid allocation points when registering a farm
    if (totalAllocPoints + params.allocPoints <= 0) revert InvalidAllocPoints();
    ...
    lastRewardTime = Math.mulDiv(params.allocPoints, _globalAccIncentiveTokenPerShare, Constants.SCALE_FACTOR_1E18),
  }

Potential Impact

This misconfiguration can have severe implications for the protocol:

• Farms created with zero allocation points will not accumulate rewards correctly.

• Once allocation points are assigned later, historical rewards will be included incorrectly, disrupting fair reward distribution.

• It could lead to unexpectedly large incentive token emissions to certain farms, potentially harming overall protocol efficiency and fairness.

Recommendation

To prevent farms from being created with zero allocation points, the registerFarm function should be updated to enforce a non-zero allocation requirement in every case.

  function registerFarm(AddFarmParams calldata params) external restricted {
  ...
+    require(params.allocPoints > 0, "Allocation points must be greater than zero");
    ...
  }

Issue 4: Missing slippage on add liquidity function can lead to stuck funds

Impact: Medium

Likelihood: Medium

Description

Uniswap V3 NFT positions can be deposited in the protocol and depositors can get extra rewards on top of their Uni Liquidity Provider rewards. The protocol has a function that lets users add additional value to their position.

If the minted amount is too small, the transaction reverts in this check in Uniswap position manager when addling liquidity.

(amount0, amount1) = pool.mint(
    params.recipient,
    params.tickLower,
    params.tickUpper,
    liquidity,
    abi.encode(MintCallbackData({poolKey: poolKey, payer: msg.sender}))
);

require(amount0 >= params.amount0Min && amount1 >= params.amount1Min, 'Price slippage check');

However when adding liquidity trough the protocol the slippage parameters are set to 0

When adding liquidity the amounts in the protocol are set to 0.

INonfungiblePositionManager.IncreaseLiquidityParams memory params = INonfungiblePositionManager
        .IncreaseLiquidityParams({
        tokenId: tokenId,
        amount0Desired: amountAdd0,
        amount1Desired: amountAdd1,
        amount0Min: 0, // <=
        amount1Min: 0, // <=
        deadline: block.timestamp
    });

As stated in the Uni v3 docs:

In production, amount0Min and amount1Min should be adjusted to create slippage protections.

We set amount0Min and amount1Min to zero for the example - but this would be a vulnerability in production. A function calling mint with no slippage protection would be vulnerable to a frontrunning attack designed to execute the mint call at an inaccurate price.

Tokens could be lost to MEV bots due to slippage. Also since the protocol would be deployed on a chain where frontrunning is possible this makes it even more likely.

Recommendation

Do not hardcode slippage protection parameter amount0Min and amount1Min to 0 when increasing liquidity, but instead let the user pass min amounts out.

Issue 5: Swap doesn’t correctly check for slippage

Impact: Low

Likelihood: High

Description

The protocol let’s users make cross-chain swaps that are similar to Uni V3 swaps, but are cross-chain.

The process being:

1. Swap asset A for asset B on chan X

2. Bridge asset B on chain Y

3. Optionally swap asset B for the desired end asset on chain Y

The final amount out on the destination chain would be checked against a min amount passed by the user on the source chain.

The protocol makes profits by taking a small fee of the tokens* on the destination chain.

In pseudo code the core of the logic looks like:

function crossChainSwap(
    address to,
    uint256 amountInSrc,
    address tokenInSrc,
    address tokenOutSrc,
    uint256 minAmountOutSrc,
    address tokenInDst,
    address tokenOutDst,
    uint256 minAmountOutDst
    ChainData memory chainData
) external {
    // safety checks

    uint256 sourceAmountOut = amountInSrc;

    if (tokenOutSrc != address(0) && tokenOutSrc != tokenInSrc) {
        sourceAmountOut = performSourceChainSwap(address tokenInSrc, address tokenOutSrc, uint256 minAmountOutSrc);
    }

    sourceAmountOut -= sourceAmountOut * feeBps / 10000;

    IERC20(tokenOutSrc).increaseAllowance(crossChainSwapManager, amount);

    // perform cross chain swap
}

The performSourceChainSwap function internally checks for slippage, which is as expected. However before the amount out is swapped cross chains a fee is applied. Note that the fee is applied after the slippage check.

Since the sourceAmountOut would be used as destination chain’s amountIn deducting the fee after the slippage check, might result in the cross swap amount being less than required by the user.

Recommendation

A solution would be to move the slippage check that verifies the sourceAmountOut is greater than the minAmountOutSrc after the fee is subtracted.

Outro

We highlighted five critical issues affecting Uniswap V3 integration and cross-chain swaps. Implementing the recommended fixes, withdrawal mechanisms, slippage protection, and better allocation point validation will enhance protocol security and user experience, ensuring long-term stability in the DeFi world.

Stay tuned for next month’s article, where we’ll share our latest discoveries and continue our journey toward a safer web3 ecosystem. Until then, stay secure and keep building!