Good Entry - kutugu's results

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L544-L551

Vulnerability details

Impact

The purpose of addDust is to ensure that both the token0Amount and token1Amount are greater than 100 units. The current implementation is to calculate the value of 100 units 1e18 scales of token0 and token1, take the maximum value as liquidity, and add to the repayAmount. The calculation does not take into account the actual getTokenAmounts result:

1. The calculated dust amount is based on the oracle price, while the actual amount consumed is based on the lp tick price
2. Even if the price of lp tick is equal to the spot price, which can't guarantee that the token0Amount and token1Amount of getTokenAmounts will be greater than 100 units.

Proof of Concept

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

import "forge-std/Test.sol";
import "../contracts/lib/LiquidityAmounts.sol";
import "../contracts/lib/TickMath.sol";

interface IERC20 {
    function decimals() external view returns (uint8);
}

interface IUniswapV3Pool{
  function slot0()
    external
    view
    returns (
      uint160 sqrtPriceX96,
      int24 tick,
      uint16 observationIndex,
      uint16 observationCardinality,
      uint16 observationCardinalityNext,
      uint8 feeProtocol,
      bool unlocked
    );
    
    function token0() external view returns (address);
    function token1() external view returns (address);
    function tickSpacing() external view returns (int24);
}

contract TestDust is Test {
    IUniswapV3Pool constant uniswapPool = IUniswapV3Pool(0xC2e9F25Be6257c210d7Adf0D4Cd6E3E881ba25f8);
    address constant token0 = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
    address constant token1 = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    uint256 constant token0Price = 1e8;
    uint256 constant token1Price = 1830e8;

    function setUp() public {
        vm.createSelectFork("https://rpc.ankr.com/eth", 17863266);
    }

    function testDustPrice() public {
        (uint token0Amount, uint token1Amount) = getTokenAmountsExcludingFees(getDust());
        console.log(token0Amount, token1Amount);
    }

    function getTokenAmountsExcludingFees(uint amount) public view returns (uint token0Amount, uint token1Amount){
        (uint160 sqrtPriceX96, int24 tick,,,,,)  = IUniswapV3Pool(uniswapPool).slot0();
        int24 tickSpacing  = IUniswapV3Pool(uniswapPool).tickSpacing();
        (token0Amount, token1Amount) = LiquidityAmounts.getAmountsForLiquidity(sqrtPriceX96, TickMath.getSqrtRatioAtTick(tick), TickMath.getSqrtRatioAtTick(tick + tickSpacing), uint128(amount));
    }

    function getDust() internal view returns (uint amount){
        uint scale0 = 10**(20 - IERC20(token0).decimals()) * token0Price / 1e8;
        uint scale1 = 10**(20 - IERC20(token1).decimals()) * token1Price / 1e8;

        if (scale0 > scale1) amount = scale0;
        else amount = scale1;
    }
}

For DAI/ETH pool, the token0Amount and token1Amount are 23278 and 0, dust value is close to 0, doesn't seem work.

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

import "forge-std/Test.sol";
import "../contracts/lib/LiquidityAmounts.sol";
import "../contracts/lib/TickMath.sol";

interface IERC20 {
    function decimals() external view returns (uint8);
}

interface IUniswapV3Pool{
  function slot0()
    external
    view
    returns (
      uint160 sqrtPriceX96,
      int24 tick,
      uint16 observationIndex,
      uint16 observationCardinality,
      uint16 observationCardinalityNext,
      uint8 feeProtocol,
      bool unlocked
    );
    
    function token0() external view returns (address);
    function token1() external view returns (address);
    function tickSpacing() external view returns (int24);
}

contract TestDust is Test {
    IUniswapV3Pool constant uniswapPool = IUniswapV3Pool(0xCBCdF9626bC03E24f779434178A73a0B4bad62eD);
    address constant token0 = 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599;
    address constant token1 = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    uint256 constant token0Price = 29000e8;
    uint256 constant token1Price = 1830e8;

    function setUp() public {
        vm.createSelectFork("https://rpc.ankr.com/eth", 17863266);
    }

    function testDustPrice() public {
        (uint token0Amount, uint token1Amount) = getTokenAmountsExcludingFees(getDust());
    }

    function getTokenAmountsExcludingFees(uint amount) public view returns (uint token0Amount, uint token1Amount){
        (uint160 sqrtPriceX96, int24 tick,,,,,)  = IUniswapV3Pool(uniswapPool).slot0();
        int24 tickSpacing  = IUniswapV3Pool(uniswapPool).tickSpacing();
        (token0Amount, token1Amount) = LiquidityAmounts.getAmountsForLiquidity(sqrtPriceX96, TickMath.getSqrtRatioAtTick(tick), TickMath.getSqrtRatioAtTick(tick + tickSpacing), uint128(amount));
    }

    function getDust() internal view returns (uint amount){
        uint scale0 = 10**(20 - IERC20(token0).decimals()) * token0Price / 1e8;
        uint scale1 = 10**(20 - IERC20(token1).decimals()) * token1Price / 1e8;

        if (scale0 > scale1) amount = scale0;
        else amount = scale1;
    }
}

For BTC/ETH pool and current tick, the dust calculation will revert.

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

import "forge-std/Test.sol";
import "../contracts/lib/LiquidityAmounts.sol";
import "../contracts/lib/TickMath.sol";

interface IERC20 {
    function decimals() external view returns (uint8);
}

interface IUniswapV3Pool{
  function slot0()
    external
    view
    returns (
      uint160 sqrtPriceX96,
      int24 tick,
      uint16 observationIndex,
      uint16 observationCardinality,
      uint16 observationCardinalityNext,
      uint8 feeProtocol,
      bool unlocked
    );
    
    function token0() external view returns (address);
    function token1() external view returns (address);
    function tickSpacing() external view returns (int24);
}

contract TestDust is Test {
    IUniswapV3Pool constant uniswapPool = IUniswapV3Pool(0xCBCdF9626bC03E24f779434178A73a0B4bad62eD);
    address constant token0 = 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599;
    address constant token1 = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    uint256 constant token0Price = 29000e8;
    uint256 constant token1Price = 1830e8;

    function setUp() public {
        vm.createSelectFork("https://rpc.ankr.com/eth", 17863266);
    }

    function testDustPrice() public {
        (uint token0Amount, uint token1Amount) = getTokenAmountsExcludingFees(getDust());
    }

    function getTokenAmountsExcludingFees(uint amount) public view returns (uint token0Amount, uint token1Amount){
        (uint160 sqrtPriceX96, int24 tick,,,,,)  = IUniswapV3Pool(uniswapPool).slot0();
        int24 tickSpacing  = IUniswapV3Pool(uniswapPool).tickSpacing();
        (token0Amount, token1Amount) = LiquidityAmounts.getAmountsForLiquidity(sqrtPriceX96, TickMath.getSqrtRatioAtTick(tick - 10 * tickSpacing), TickMath.getSqrtRatioAtTick(tick), uint128(amount));
    }

    function getDust() internal view returns (uint amount){
        uint scale0 = 10**(20 - IERC20(token0).decimals()) * token0Price / 1e8;
        uint scale1 = 10**(20 - IERC20(token1).decimals()) * token1Price / 1e8;

        if (scale0 > scale1) amount = scale0;
        else amount = scale1;
    }
}

For the last 10 ticks, the token0Amount and token1Amount are 0 and 341236635433582778849. That's a huge number, not dust.

Tools Used

Foundry

Recommended Mitigation Steps

Check the amount of token0Amount and token1Amount corresponding to repayAmount instead of adding dust manually

Assessed type

Context

Findings Summary

Detailed Findings

[L-01] modifyTick did not check that the tick is properly ordered

Description

  function modifyTick(address tr, uint index) public onlyOwner {
    (ERC20 t0,) = TokenisableRange(tr).TOKEN0();
    (ERC20 t1,) = TokenisableRange(tr).TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    ticks[index] = TokenisableRange(tr);
    emit ModifyTick(tr, index);
  }

modifyTick did not check that the tick is properly ordered. If the owner enters an incorrect tick, the ordering is damaged, and the correct tick cannot be found to complete the order.

Recommendations

Check the tick is properly ordered when modifyTick

[L-02] FixedOracle roundId should use the L2 block number

Description

  function latestRoundData()
      external
      view
      returns (
          uint80 roundId,
          int256 answer,
          uint256 startedAt,
          uint256 updatedAt,
          uint80 answeredInRound
      )
  {
      roundId = uint80(block.number);
      answer = hardcodedPrice;
      startedAt = block.timestamp;
      updatedAt = block.timestamp;
      answeredInRound = roundId;
  }

In arbitrum, block.number obtains the block number of L1, and the block interval is 12s. If the owner executes setHardcodedPrice, there are two prices in the same roundId, and the price conflict lasts for 12s. By using L2 blocks, interval is only 1s, this problem can be greatly alleviated.

Recommendations

Use l2 block number

[L-03] OracleConvert latestAnswer may overflow

Description

  function latestAnswer() public view returns (int256) {
    uint priceA = uint(getAnswer(CL_TOKENA));
    uint priceB = uint(getAnswer(CL_TOKENB));
    // @audit CL_TOKENB.decimals() + CL_TOKENA.decimals() < 8
    return int(priceA * priceB / (10 ** (CL_TOKENB.decimals() + CL_TOKENA.decimals() - 8))); 
  }

There is no rule that CL_TOKENB.decimals() + CL_TOKENA.decimals() must be greater than 8, so subtraction may overflow

Recommendations

Not use subtraction, but priceA * priceB * 10 ** 8 / (10 ** (CL_TOKENB.decimals() + CL_TOKENA.decimals()))

[L-04] Should be compatible with the zero-value transfer revert

Description

  // @audit Some token zero-value transfers will revert
  TOKEN0.token.transferFrom(msg.sender, address(this), n0);
  TOKEN1.token.transferFrom(msg.sender, address(this), n1);
  
  uint newFee0; 
  uint newFee1;
  // Calculate proportion of deposit that goes to pending fee pool, useful to deposit exact amount of liquidity and fully repay a position
  // Cannot repay only one side, if fees are both 0, or if one side is missing, skip adding fees here
    // if ( fee0+fee1 == 0 || (n0 == 0 && fee0 > 0) || (n1 == 0 && fee1 > 0) ) skip  
    // DeMorgan: !( (n0 == 0 && fee0 > 0) || (n1 == 0 && fee1 > 0) ) = !(n0 == 0 && fee0 > 0) && !(n0 == 0 && fee1 > 0)
  // @audit From here you can see that n0 == 0 is supported
  if ( fee0+fee1 > 0 && ( n0 > 0 || fee0 == 0) && ( n1 > 0 || fee1 == 0 ) ){
    address pool = V3_FACTORY.getPool(address(TOKEN0.token), address(TOKEN1.token), feeTier * 100);
    (uint160 sqrtPriceX96,,,,,,)  = IUniswapV3Pool(pool).slot0();
    (uint256 token0Amount, uint256 token1Amount) = LiquidityAmounts.getAmountsForLiquidity( sqrtPriceX96, TickMath.getSqrtRatioAtTick(lowerTick), TickMath.getSqrtRatioAtTick(upperTick), liquidity);
    if (token0Amount + fee0 > 0) newFee0 = n0 * fee0 / (token0Amount + fee0);
    if (token1Amount + fee1 > 0) newFee1 = n1 * fee1 / (token1Amount + fee1);
    fee0 += newFee0;
    fee1 += newFee1; 
    n0   -= newFee0;
    n1   -= newFee1;
  }

For some tokens, such as BNB, zero-value transfers will revert.

Recommendations

transfer only when the quantity is not zero

[L-05] claimFee should not use 1% of balance as a threshold

Description

  // If accumulated more than 1% worth of fees, compound by adding fees to Uniswap position
  if ((fee0 * 100 > bal0 ) && (fee1 * 100 > bal1)) { 

claimFee uses 1% of the balance as a threshold to consider whether to add fee to the position.
However, the problem is that the balance may be very large. For a pool with deep liquidity, the fee cannot reach 1% for a long time.
Because fee has been idle, resulting in low utilization rate and low income.

Recommendations

Should use USD value to decide whether to add liquidity, not balance.

[L-06] TokenisableRange deposit should check lpAmt is not zero

Description

  lpAmt = totalSupply() * newLiquidity / (liquidity + feeLiquidity); 
  liquidity = liquidity + newLiquidity;
  
  _mint(msg.sender, lpAmt);

When calculating the lp amount, if the number of tokens deposited by the user is low and the liquidity is large, the calculated lpAmt may be 0, and the user loses funds. Combined with an attack similar to ERC4626, a malicious user may frontrun to deposit a large amount of liquidity, causing the lpAmt to calculate 0. Of course, since totalSupply is initialized as 1e18, this kind of cost is high, the possibility is low, and the benefit is also low. But for the sake of the user, there should check lpAmt > 0.

Recommendations

check lpAmt > 0

[L-07] The FixedOracle update may conflict when the coin price is volatile

Description

  function setHardcodedPrice(int256 _hardcodedPrice) external onlyOwner {
    hardcodedPrice = _hardcodedPrice;
    emit SetHardcodedPrice(_hardcodedPrice);
  }

When the coin price fluctuates dramatically, the fixedOracle update interval is very small.
If there are two update txs at the same time, their execution will conflict, possibly resulting in a stale price

Recommendations

Pass update time through the parameter, and only the latest timestamp price can be updated

[L-08] Set deadline to block.timestamp has no effect

Description

The protocol uses a lot of block.timestamp as deadline for external interactions such as swap, but this has no effect in practice because deadline is the time when tx is executed and is always valid.

Recommendations

Pass deadline through the parameter.

[L-09] Hard-coded slippage cannot be modified in subsequent governance

Description

uint[] memory amounts = ammRouter.swapExactTokensForTokens(
  amount, 
  getTargetAmountFromOracle(oracle, path[0], amount, path[1]) * 99 / 100, // allow 1% slippage 
  path, 
  address(this), 
  block.timestamp
);

The protocol uses a lot of hard-coded slippage, which cannot be modified in subsequent governance.
In particular, the swap slippage is hard-coded to 1%, oracle prices are taken from DEX and CEX, and CEX is dominated. Chainlink oracle also has an update threshold of 0.5%.
The uniswap pool token price is likely to be more than 1% different from the price of chainlink oracle, resulting in the swap being DOS for a period of time until the difference returns to 1%.

Recommendations

Use variable slippage