WeightedLiquidityPool - Dexodus

When getEffectivePoolValue approaches zero due to unrealized profits held by traders, the LP share calculations in calcLpOut exhibit inconsistent behavior. Specifically:

1. Inflated LP Shares: As getEffectivePoolValue approaches zero (but remains slightly positive), the calcLpOut function mints excessively high LP shares.

2. Reset to Base Shares: When getEffectivePoolValue drops to exactly zero, LP shares return to baseline behavior, ignoring the unrealized profit adjustment.

This discrepancy creates a significant bug where LP providers may inadvertently mint disproportionately high shares in a near-zero pool state. While this scenario may be rare due to protocol mechanisms, it represents a critical vulnerability for the integrity of LP share calculations.

    int256 public unrealizedPnL;

    function _setUnrealizedPnL(int256 _unrealizedPnL) public {
        unrealizedPnL = _unrealizedPnL;
    }

    function test_halborn_002() public {
        uint256 eth_price;

        vm.startPrank(deployer);
        liquidityPool.setFuturesCore(address(this));
        vm.stopPrank();

        eth_price = liquidityPool.getEthPriceInUSD(1 ether);
        _setUnrealizedPnL(0);

        // deal(address(_usdc), address(liquidityPool), 1000e6);
        // deal(address(_weth), address(liquidityPool), 1 ether);
        address user = address(0x1001);
        address user2 = address(0x1002);
        address user3 = address(0x1003);

        console.log("USDC balance of pool", IERC20(_usdc).balanceOf(address(liquidityPool)));
        console.log("USDC balance of user", IERC20(_usdc).balanceOf(user));
        console.log("WETH balance of pool", IERC20(_weth).balanceOf(address(liquidityPool)));
        console.log("WETH balance of user", IERC20(_weth).balanceOf(user));
        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
        console.log("Pool balance of user2", IERC20(address(liquidityPool)).balanceOf(user2));
        console.log("getEffectivePoolValue", liquidityPool.getEffectivePoolValue());
        console.log("calcLpOut", liquidityPool.calcLpOut(liquidityPool.getEthPriceInUSD(1 ether)));

        // getEthPriceInUSD
        console.log("getEthPriceInUSD", liquidityPool.getEthPriceInUSD(1 ether));

        console.log("");

        vm.startPrank(user);
        deal(user, 1 ether);
        liquidityPool.lockLiquidity{value: 1 ether}(4, "Crab");
        vm.stopPrank();

        vm.startPrank(user2);
        deal(user2, 1 ether);
        liquidityPool.lockLiquidity{value: 1 ether}(4, "Crab");
        vm.stopPrank();


        console.log("USDC balance of pool", IERC20(_usdc).balanceOf(address(liquidityPool)));
        console.log("USDC balance of user", IERC20(_usdc).balanceOf(user));
        console.log("WETH balance of pool", IERC20(_weth).balanceOf(address(liquidityPool)));
        console.log("WETH balance of user", IERC20(_weth).balanceOf(user));
        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
        console.log("Pool balance of user2", IERC20(address(liquidityPool)).balanceOf(user2));
        console.log("getEffectivePoolValue", liquidityPool.getEffectivePoolValue());
        console.log("calcEthOut user", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user)));
        console.log("calcEthOut user2", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user2)));
        console.log("calcLpOut", liquidityPool.calcLpOut(liquidityPool.getEthPriceInUSD(1 ether)));
        console.log("");


        eth_price = liquidityPool.getEthPriceInUSD(1 ether);
        _setUnrealizedPnL(int256(1 * eth_price));

        console.log("USDC balance of pool", IERC20(_usdc).balanceOf(address(liquidityPool)));
        console.log("USDC balance of user", IERC20(_usdc).balanceOf(user));
        console.log("WETH balance of pool", IERC20(_weth).balanceOf(address(liquidityPool)));
        console.log("WETH balance of user", IERC20(_weth).balanceOf(user));
        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
        console.log("Pool balance of user2", IERC20(address(liquidityPool)).balanceOf(user2));
        console.log("getEffectivePoolValue", liquidityPool.getEffectivePoolValue());
        console.log("calcEthOut user", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user)));
        console.log("calcEthOut user2", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user2)));
        console.log("calcLpOut", liquidityPool.calcLpOut(liquidityPool.getEthPriceInUSD(1 ether)));
        console.log("");

        // vm.startPrank(user3);
        // deal(user3, 1 ether);
        // liquidityPool.lockLiquidity{value: 1 ether}(4, "Crab");
        // vm.stopPrank();

        // console.log("Pool balance of user3", IERC20(address(liquidityPool)).balanceOf(user3));
        // console.log("calcEthOut user3", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user3)));

        console.log("");

        eth_price = liquidityPool.getEthPriceInUSD(1 ether);
        _setUnrealizedPnL(int256(2 * eth_price) - 1);

        console.log("USDC balance of pool", IERC20(_usdc).balanceOf(address(liquidityPool)));
        console.log("USDC balance of user", IERC20(_usdc).balanceOf(user));
        console.log("WETH balance of pool", IERC20(_weth).balanceOf(address(liquidityPool)));
        console.log("WETH balance of user", IERC20(_weth).balanceOf(user));
        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
        console.log("Pool balance of user2", IERC20(address(liquidityPool)).balanceOf(user2));
        console.log("getEffectivePoolValue", liquidityPool.getEffectivePoolValue());
        console.log("calcEthOut user", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user)));
        console.log("calcEthOut user2", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user2)));
        console.log("calcLpOut", liquidityPool.calcLpOut(liquidityPool.getEthPriceInUSD(1 ether)));
        console.log("");

        eth_price = liquidityPool.getEthPriceInUSD(1 ether);
        _setUnrealizedPnL(int256(2 * eth_price));

        console.log("");

        console.log("USDC balance of pool", IERC20(_usdc).balanceOf(address(liquidityPool)));
        console.log("USDC balance of user", IERC20(_usdc).balanceOf(user));
        console.log("WETH balance of pool", IERC20(_weth).balanceOf(address(liquidityPool)));
        console.log("WETH balance of user", IERC20(_weth).balanceOf(user));
        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
        console.log("Pool balance of user2", IERC20(address(liquidityPool)).balanceOf(user2));
        console.log("getEffectivePoolValue", liquidityPool.getEffectivePoolValue());
        console.log("calcEthOut user", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user)));
        console.log("calcEthOut user2", liquidityPool.calcEthOut(IERC20(address(liquidityPool)).balanceOf(user2)));
        console.log("calcLpOut", liquidityPool.calcLpOut(liquidityPool.getEthPriceInUSD(1 ether)));
        console.log("");
    }

Output:

Logs:
  USDC balance of pool 0
  USDC balance of user 0
  WETH balance of pool 0
  WETH balance of user 0
  Pool balance of user 0
  Pool balance of user2 0
  getEffectivePoolValue 0
  calcLpOut 3124870000000000000000
  getEthPriceInUSD 3124870000

  USDC balance of pool 0
  USDC balance of user 0
  WETH balance of pool 2000000000000000000
  WETH balance of user 0
  Pool balance of user 3124870000000000000000
  Pool balance of user2 3124870000000000000000
  getEffectivePoolValue 6249740000000000000000
  calcEthOut user 1000000000000000000
  calcEthOut user2 1000000000000000000
  calcLpOut 3124870000000000000000

  USDC balance of pool 0
  USDC balance of user 0
  WETH balance of pool 2000000000000000000
  WETH balance of user 0
  Pool balance of user 3124870000000000000000
  Pool balance of user2 3124870000000000000000
  getEffectivePoolValue 3124870000000000000000
  calcEthOut user 500000000000000000
  calcEthOut user2 500000000000000000
  calcLpOut 6249740000000000000000


  USDC balance of pool 0
  USDC balance of user 0
  WETH balance of pool 2000000000000000000
  WETH balance of user 0
  Pool balance of user 3124870000000000000000
  Pool balance of user2 3124870000000000000000
  getEffectivePoolValue 1000000000000
  calcEthOut user 160006656
  calcEthOut user2 160006656
  calcLpOut 19529625033800000000000000000000


  USDC balance of pool 0
  USDC balance of user 0
  WETH balance of pool 2000000000000000000
  WETH balance of user 0
  Pool balance of user 3124870000000000000000
  Pool balance of user2 3124870000000000000000
  getEffectivePoolValue 0
  calcEthOut user 0
  calcEthOut user2 0
  calcLpOut 3124870000000000000000

To address this issue, modify the calcLpOut logic to gracefully handle scenarios where getEffectivePoolValue approaches or equals zero. This ensures consistent share calculations even in extreme states. Also, clearly document how the system handles near-zero getEffectivePoolValue scenarios to align user and developer expectations.

SOLVED: The code is now using a different incentive for the described scenario by not letting the totalValueInUSD drop less than 10 USD on the calcLpOut function. Also, the client stated that:

This is an edge case that with all the mechanisms we have from the perpetual side and the overall protocol should never happen. But we thought even a solution for an isolated case of taking into account only the liquidity pool to help that case.

The setswapSlippage function does not validate that the provided slippage value is less than or equal to BASIS_POINTS. If slippage is set to a value greater than BASIS_POINTS, it will result in underflows during calculations in the swapEthToUsdc and swapUsdcToEth functions. Specifically, the subtraction for minUsdcOut and minWethOut could yield negative results, causing the contract to revert.

Add a validation check in the setswapSlippage function to ensure that slippage does not exceed BASIS_POINTS. For example:

function setswapSlippage(uint256 slippage) external onlyOwner {
    require(slippage <= BASIS_POINTS, "Slippage exceeds allowable range");
    swapSlippage = slippage;
}

This ensures the swapEthToUsdc and swapUsdcToEth functions operate correctly and avoids unintended underflows. By restricting slippage to a valid range, the contract's behavior remains predictable and secure.

SOLVED: The code is now checking for the upper bounds.

The lockLiquidity function does not prevent the creation of a locked liquidity entry when the calculated lpAmountOut is zero. Similarly, addLiquidity does not restrict minting when lpAmountOut is zero. These issues can result in:

1. Operational Errors: Zero-value locks will not be correctly managed in unlockLiquidity, leading to unexpected behavior or failure.

2. Inflation Attacks: Zero-value entries may allow unnoticed manipulation of pool shares or operational errors in the system.

3. Rounding Issues: Small deposit values may round down to zero, introducing inaccuracies and unexpected share calculations.

    function test_halborn_zero_mint() public {
        uint256 eth_price;

        vm.startPrank(deployer);
        liquidityPool.setAddLiqAvailable(1);
        liquidityPool.setFuturesCore(address(this));
        vm.stopPrank();

        _setUnrealizedPnL(0);

        address user = address(0x1234);

        uint256 less_min_value = 300000000;

        vm.startPrank(user);
        deal(user, 10 ether);
        liquidityPool.addLiquidity{value: less_min_value}();
        vm.stopPrank();

        console.log("Pool balance of user", IERC20(address(liquidityPool)).balanceOf(user));
    }

Output:

Logs:
  Pool balance of user 0

Add validation checks in both functions to ensure lpAmountOut is greater than zero before proceeding.

SOLVED: The code is now checking for zero calculated lpAmountOut.

The calcEthOut function does not account for scenarios where totalSupply() is zero. If called under such conditions, the function will attempt to perform a division by zero, causing the contract to revert. This could lead to unexpected contract failures when calculating WETH amounts for LP tokens during initialization or under specific edge cases.

Introduce a check for totalSupply() at the beginning of the function. If totalSupply() is zero, the function should return 0 to handle this edge case gracefully.

SOLVED: The total supply function now returns 0 when there is no supply.

The rebalancePool function and the swapUsdcToEthExactOutput logic rely on the Chainlink oracle for ETH/USD price data while executing swaps through the Uniswap router. Discrepancies between the Chainlink oracle price and the Uniswap price can arise due to market volatility, differences in data sources, or short-term pool manipulation. This discrepancy can result in:

1. Inefficient rebalancing that diverges from actual pool values.

2. Failed user transactions due to mismatches in expected and actual swap outcomes, requiring frequent adjustments of swapSlippage.

3. Vulnerability to price manipulation attacks, especially if Chainlink prices lag behind real-time market conditions.

Consider using Uniswap's prices directly for rebalancing and swaps. Use a Volume-Weighted Average Price (VWAP) over a suitable time frame to mitigate the risk of price manipulation. This approach ensures price alignment with the swap router and provides more accurate and secure operations. Specific actions include:

1. Integrate Uniswap Price Feeds:

2. Fetch VWAP data using Uniswap's pool observations or by querying a reliable Uniswap price oracle.

3. Use this price as the basis for calculations in rebalancePool and swapUsdcToEthExactOutput.

4. Implement Manipulation Safeguards:

5. Use time-weighted average prices rather than spot prices to reduce susceptibility to flash loan or sandwich attacks.

6. Adapt Slippage Management:

7. Ensure setswapSlippage does not require frequent adjustments by aligning pricing mechanisms, reducing user transaction failures.

By aligning the pricing source with the swap mechanism (Uniswap), this approach ensures operational consistency, reduces risks of transaction failures, and mitigates pool manipulation threats.

RISK ACCEPTED: The Dexodus team accepted the risk of this finding.

The contract lacks a mechanism to update the priceFeed address, which could be problematic if the oracle becomes deprecated or unavailable. Additionally, the getEthPriceInUSD function uses hardcoded static values for decimal adjustments, which may not accommodate changes in the oracle's precision over time. This could lead to incorrect price calculations and potential loss of accuracy or functionality.

Introduce a function, restricted to the owner, to allow updating the priceFeed address dynamically. Ensure the new address is validated for zero address and AggregatorV3Interface compliance. Modify getEthPriceInUSD to be decimal-agnostic by storing the price feed's decimals during initialization or update. For example:

1. Add a uint8 public priceFeedDecimals variable to store the oracle's decimal information.

2. During initialization or when updating the priceFeed, retrieve and store its decimals using AggregatorV3Interface.decimals().

3. Update the getEthPriceInUSD function to use priceFeedDecimals instead of static decimal values, ensuring dynamic adjustment.

This approach enhances flexibility and accuracy while reducing reliance on external calls to the oracle for each calculation.

SOLVED: The code is now including a setPriceFeed function and the getEthPriceInUSD will be using dynamic decimals for the oracle calculations.

The getEthPriceInUSD function relies on Chainlink's price feed data but does not validate the freshness of the retrieved price. If the price feed data becomes stale (e.g., no updates for over an hour), it could lead to the use of outdated or invalid price information, introducing significant risks to calculations and decision-making within the contract.

Incorporate a staleness check in the getEthPriceInUSD function. Use the timestamp provided by Chainlink's latestRoundData to ensure that the price feed data is recent.

SOLVED: The getEthPriceInUSD function does now check for 1 hour of price window.

The addLiquidity function lacks a minimal liquidity check, indirectly relying on the calcLpOut function's behavior, which returns 0 if wethValueInUSD is less than 1e6 due to 1e18 / 1e6 rounding. While this prevents minting of fractional shares for very low ETH deposits, it does not explicitly enforce a minimal msg.value, potentially causing confusion and preventing predictable behavior for edge cases. Additionally, the lack of explicit checks may lead to misinterpretation of the function's behavior and reliance on implicit decimal handling.

Introduce an explicit check for a minimal msg.value to ensure that the calculated lpAmountOut is meaningful and avoids unintentional zero-minting scenarios. The minimal value can be dynamically computed based on the current ETH price and the smallest share size.

By implementing these changes, the function avoids edge cases where users mistakenly deposit ETH with no resulting minted shares and ensures the contract's behavior is explicit and predictable.

SOLVED: This issue was directly solved by the fact that lpAmountOut is being checked for a 0 value, those not requiring to verify the msg.value.

The initialize function does not perform zero address validation or check for ERC165 interface compliance for the provided contract addresses (_usdc, _weth, _uniswapRouter, _priceFeed). This omission can lead to the initialization of the contract with invalid or misconfigured dependencies, potentially causing unexpected behavior or failure during runtime.

In the initialize function, implement checks to ensure that the provided addresses are non-zero and validate the compliance of contracts (e.g., IERC165 support) when applicable. For example:

• Check if each address is non-zero, using require(address != address(0), "Invalid address").

• Validate that contracts implementing interfaces support required functions using IERC165.supportsInterface.

Adding these validations during initialization will enhance the robustness of the contract and prevent potential misconfigurations.

SOLVED: The code now checks for 0 address.

The lockLiquidity function relies on string comparisons to determine the animalType tier using keccak256 hashing. This approach incurs unnecessary gas costs and increases computational complexity. Strings are less gas-efficient than alternatives like enums, which are compact and avoid the need for hashing or string processing.

Replace the string-based animalType mechanism with an enum. Enums are gas-efficient, more readable, and prevent invalid inputs. Example:

1. Define an enum for animal tiers:

   enum AnimalType { Crab, Octopus, Fish, Dolphin, Shark, Whale }

1. Update lockLiquidity to accept AnimalType as an input:

   function lockLiquidity(uint256 months, AnimalType animalType) external payable {
       ...
       require(
           (animalType == AnimalType.Crab && ethValueInUSD >= 100e6) ||
           (animalType == AnimalType.Octopus && ethValueInUSD >= 500e6) ||
           ...
       );
       ...
   }

1. Emit the animalType enum as an integer in the event for easier off-chain processing.

Switching to enums eliminates the gas costs associated with string hashing and improves overall contract efficiency and clarity.

SOLVED: The code is now using enum instead of strings.

The unlockLiquidity function does not validate whether the provided id is within the bounds of the lockedLiquidity array for the caller. If an invalid or out-of-bounds id is passed, the function will revert with a low-level error, which lacks context and can lead to confusion or unexpected failures.

Add an explicit bound check to ensure the provided id is valid. For example:

require(id < lockedLiquidity[msg.sender].length, "Invalid unlock ID");

Incorporate this check before accessing lockedLiquidity[msg.sender][id]. This ensures that only valid indices are processed, preventing out-of-bounds access and enhancing the robustness of the function. The added validation also makes errors more user-friendly and informative.

SOLVED: The code is verifying that the given id is not out of bounds.

The codebase uses a mix of 18-decimal factors (e.g., WETH) and 6-decimal factors (e.g., USDC). However, function descriptions, particularly for critical functions like getEffectivePoolValue, do not explicitly clarify the decimal format of their return values. For example, getEffectivePoolValue returns values in 18 decimals, but this is not mentioned in its documentation, which could lead to misinterpretation and errors during integration or auditing.

Enhance function documentation across the codebase to specify the decimal factor of input and output values clearly.

SOLVED: The natspec of the functions was updated to include details on the decimals used.

The contract does not use a consistent naming convention to distinguish between internal and external functions. Internal functions (e.g., swapEthToUsdcExactOutput, swapUsdcToEthExactOutput) are not prefixed with an underscore (_), making it harder to identify their intended visibility at a glance. This could lead to confusion, accidental exposure, or misuse during contract upgrades or maintenance.

Adopt a convention where all internal functions are prefixed with an underscore (_).

This naming convention enhances readability, minimizes the risk of accidental exposure, and provides a clear distinction between public-facing and internal logic, making the codebase easier to understand and maintain.

SOLVED: The internal functions are now using _ prefix.

The setswapSlippage function name does not follow standard camelCase naming conventions, which can reduce code readability and consistency. This inconsistency may lead to confusion when interacting with the function programmatically or reviewing the codebase.

Rename the function from setswapSlippage to setSwapSlippage to adhere to standard camelCase conventions.

SOLVED: The camel case version was used for setswapSlippage.