Skip to content

Latest commit

 

History

History
253 lines (201 loc) · 10.7 KB

033.md

File metadata and controls

253 lines (201 loc) · 10.7 KB

Expert Cedar Unicorn

High

Strategy collectFees uses incorrect position range in vestPosition fee collection

Summary

The Strategy::collectFees collects all outstanding position fees. In case there's ongoing vested position, it collects the already vested part of it. The issue arises in a parameter mismatch in the vesting position fees collection function (collectPositionFees(vestPosition.tickLower, mainPosition.tickUpper); that causes transaction reverts or collection of incorrect fees (partial or complete loss of vesting position fees) for vault depositors.

Link here: https://github.com/sherlock-audit/2025-02-yieldoor/blob/main/yieldoor/src/Strategy.sol#L159-L181

Root Cause

In Strategy.sol:159 the collectFees function uses mainPosition.tickUpper instead of vestPosition.tickUpper when collecting fees for the vesting position (Strategy.sol:167):

if (ongoingVestingPosition) {
@>            collectPositionFees(vestPosition.tickLower, mainPosition.tickUpper);
}

This causes two critical issues:

  1. Transaction reverts if no position exists with the incorrect range
  2. Collection of wrong fees if a position does exist with the incorrect range

Internal Pre-conditions

  1. A vesting position must be active (ongoingVestingPosition must be true)
  2. The vesting position's tick range must differ from the main position's tick range
  3. The collectFees function must be called while a vesting position is active

External Pre-conditions

  1. No Uniswap V3 position exists with the incorrect range (for revert scenario) or
  2. A Uniswap V3 position happens to exist with the incorrect range (for wrong fee scenario)

Attack Path

  1. Owner adds a vesting position via addVestingPosition to incentivize LPs
  2. Strategy attempts to collect fees via collectFees function
  3. If no position exists with the incorrect range:
  • The transaction reverts with "NP" (Not Position) error
  • All fee collection fails, including from main and secondary positions
  • Functions that depend on collectFees (like withdrawals and rebalancing) also fail
  1. If a position exists with the incorrect range:
  • Fees are collected from the wrong position
  • The actual vesting position fees are not collected
  • Vault depositors receive incorrect rewards

Impact

There are two main impacts:

  • System failure impact: the core protocol functions that call collectFees will revert (like users are unable to withdraw their funds).
  • Economic loss impact: vesting rewards are collected from the wrong position that causes the vault depositors to receive incorrect fees. This leads to potential loss of user funds (if they are less then expected) in the form of uncollected fees and ineffective incentivization mechanism as vesting rewards aren't properly distributed.

PoC

In the IStrategy.sol add:

function changePositionWidth(uint24 _newWidth) external;

Create a file named AuditTest.t.sol and copy/past this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import {Vault} from "../src/Vault.sol";
import {IVault} from "../src/interfaces/IVault.sol";
import {IStrategy} from "../src/interfaces/IStrategy.sol";
import {IUniswapV3Pool} from "../src/interfaces/IUniswapV3Pool.sol";
import {IERC20} from "@openzeppelin/token/ERC20/IERC20.sol";
import {Strategy} from "../src/Strategy.sol";
import {IMainnetRouter} from "../src/interfaces/IMainnetRouter.sol";
import {LiquidityAmounts} from "../src/libraries/LiquidityAmounts.sol";
import {TickMath} from "../src/libraries/TickMath.sol";

import {console2} from "forge-std/Test.sol";

contract AuditTest is Test {
    IUniswapV3Pool pool = IUniswapV3Pool(0x99ac8cA7087fA4A2A1FB6357269965A2014ABc35);
    IERC20 wbtc = IERC20(0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599); // token0
    IERC20 usdc = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48); // token1
    address uniRouter = address(0xE592427A0AEce92De3Edee1F18E0157C05861564);
    address vault;
    address strategy;
    address rebalancer = address(1001);
    address feeRecipient = address(9999);
    address user = address(1);
    address depositor = address(100000000001);
    address owner = address(222);

    function setUp() public virtual {
        vm.startPrank(owner);
        vault = address(new Vault(address(wbtc), address(usdc)));
        strategy = address(new Strategy(address(pool), address(vault), rebalancer, feeRecipient));
        IVault(vault).setStrategy(strategy);

        // make initial deposit
        deal(address(wbtc), user, 1e8);
        deal(address(usdc), user, 100_000e6);

        vm.startPrank(user);
        wbtc.approve(vault, type(uint256).max);
        usdc.approve(vault, type(uint256).max);

        IVault(vault).deposit(1e8, 100_000e6, 0, 0);
        assertEq(wbtc.balanceOf(user), 0);
        assertEq(usdc.balanceOf(user), 0);

        vm.startPrank(rebalancer);
        skip(10 minutes);
        IStrategy(strategy).rebalance();

        vm.startPrank(depositor);
        wbtc.approve(vault, type(uint256).max);
        usdc.approve(vault, type(uint256).max);

        vm.stopPrank();
    }

    function test_collectWrongPositionFeesForVesting() public {
        vm.startPrank(owner);

        deal(address(wbtc), owner, 2e8);
        deal(address(usdc), owner, 100_000e6);
        wbtc.approve(vault, type(uint256).max);
        usdc.approve(vault, type(uint256).max);

        (uint160 sqrtPriceX96, int24 tick,,,,,) = IUniswapV3Pool(pool).slot0();

        int24 tickLower = IStrategy(strategy).getMainPosition().tickLower;
        int24 tickUpper = IStrategy(strategy).getMainPosition().tickUpper;

        uint128 liquidity = LiquidityAmounts.getLiquidityForAmounts(
            sqrtPriceX96, TickMath.getSqrtRatioAtTick(tickLower), TickMath.getSqrtRatioAtTick(tickUpper), 2e8, 100_000e6
        );

        (uint256 amount0, uint256 amount1) = LiquidityAmounts.getAmountsForLiquidity(
            sqrtPriceX96, TickMath.getSqrtRatioAtTick(tickLower), TickMath.getSqrtRatioAtTick(tickUpper), liquidity
        );

        IVault(vault).addVestingPosition(amount0, amount1, 1 days);
        int24 tickLowerVesting = IStrategy(strategy).getVestingPosition().tickLower;
        int24 tickUpperVesting = IStrategy(strategy).getVestingPosition().tickUpper;

        assertEq(tickLower, tickLowerVesting);
        assertEq(tickUpper, tickUpperVesting);

        skip(1 days / 2);

        vm.startPrank(rebalancer);
        IStrategy(strategy).changePositionWidth(480);
        vm.stopPrank();

        int24 tickLowerAfterChange = IStrategy(strategy).getMainPosition().tickLower;
        int24 tickUpperAfterChange = IStrategy(strategy).getMainPosition().tickUpper;
        int24 tickLowerVestingAfterChange = IStrategy(strategy).getVestingPosition().tickLower;
        int24 tickUpperVestingAfterChange = IStrategy(strategy).getVestingPosition().tickUpper;

        assertNotEq(tickLowerAfterChange, tickLowerVestingAfterChange);
        assertNotEq(tickUpperAfterChange, tickUpperVestingAfterChange);

        uint256 wbtcFeeRecipientBefore = wbtc.balanceOf(feeRecipient);
        uint256 usdcFeeRecipientBefore = usdc.balanceOf(feeRecipient);
        console2.log("wbtcFeeRecipientBefore: ", wbtcFeeRecipientBefore);
        assertEq(wbtcFeeRecipientBefore, 0);
        assertEq(usdcFeeRecipientBefore, 0);

        // Execute trades to generate fees
        address trader = address(1234);
        deal(address(wbtc), trader, 10e8);
        deal(address(usdc), trader, 5_000_000e6);
        vm.startPrank(trader);
        wbtc.approve(uniRouter, type(uint256).max);
        usdc.approve(uniRouter, type(uint256).max);
        // Swap to generate fees
        IMainnetRouter(uniRouter).exactInputSingle(
            IMainnetRouter.ExactInputSingleParams({
                tokenIn: address(wbtc),
                tokenOut: address(usdc),
                fee: 3000,
                recipient: trader,
                deadline: block.timestamp + 300,
                amountIn: 1e8, // 1 BTC
                amountOutMinimum: 0,
                sqrtPriceLimitX96: 0
            })
        );
        vm.stopPrank();

        skip(1 days / 2);

        IStrategy(strategy).collectFees();

        uint256 wbtcFeeRecipientAfter = wbtc.balanceOf(feeRecipient);
        uint256 usdcFeeRecipientAfter = usdc.balanceOf(feeRecipient);
        console2.log("wbtcFeeRecipientAfter: ", wbtcFeeRecipientAfter);
        assertNotEq(wbtcFeeRecipientAfter, 0);
        assertEq(usdcFeeRecipientAfter, 0);
    }
}

Run forge test --match-test test_collectWrongPositionFeesForVesting -vvvv --fork-url https://eth-mainnet.g.alchemy.com/v2/yourAPIKey

Note: modify yourAPIKey with your Alchemy or other provider key.

0x99ac8cA7087fA4A2A1FB6357269965A2014ABc35::burn(67680 [6.768e4], 68040 [6.804e4], 0)
    │   │   └─ ← [Revert] revert: NP
    │   └─ ← [Revert] revert: NP
    └─ ← [Revert] revert: NP

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 29.89s (7.66s CPU time)

The test shows that if no Uniswap V3 position exists with the range from vestPosition.tickLower to mainPosition.tickUpper, then the burn call will revert with "NP" (Not Position) error. This completely blocks the collectFees.

Mitigation

Modify the second parameter in the collectPositionFees of the vesting position :

function collectFees() public {
        (uint256 preBal0, uint256 preBal1) = idleBalances();

        if (mainPosition.liquidity != 0) collectPositionFees(mainPosition.tickLower, mainPosition.tickUpper);
        if (secondaryPosition.liquidity != 0) {
            collectPositionFees(secondaryPosition.tickLower, secondaryPosition.tickUpper);
        }
        if (ongoingVestingPosition) {
-           collectPositionFees(vestPosition.tickLower, mainPosition.tickUpper);
+           collectPositionFees(vestPosition.tickLower, vestPosition.tickUpper);
        }

        (uint256 afterBal0, uint256 afterBal1) = idleBalances();

        uint256 protocolFees0 = (afterBal0 - preBal0) * protocolFee / 10_000;
        uint256 protocolFees1 = (afterBal1 - preBal1) * protocolFee / 10_000;

        if (protocolFees0 > 0) IERC20(token0).safeTransfer(feeRecipient, protocolFees0);
        if (protocolFees1 > 0) IERC20(token1).safeTransfer(feeRecipient, protocolFees1);

        if (ongoingVestingPosition) {
            _withdrawPartOfVestingPosition(); // doing that now, otherwise we'd charge protocol fee for the vested position
        }
    }

Run the test again: forge test --match-test test_collectWrongPositionFeesForVesting -vv --fork-url https://eth-mainnet.g.alchemy.com/v2/yourAPIKey

Logs:
  wbtcFeeRecipientBefore:  0
  wbtcFeeRecipientAfter:  757

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.48s (9.17s CPU time)

Now all work fine, the fees are collected.