hats-finance / Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f

0 stars 0 forks source link

First liquidity provider of a stable pair can DOS the pool #28

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc75fc74f410b735b4d7fb2f57342daccc15167ac95e6084e242afa093947d666 Severity: high

Description: Context: Pair.sol#511, Pair.sol#364

Description\ The invariant k of a stable pool is calculated as follows

    function _k(uint x, uint y) internal view returns (uint) {
        if (stable) {
            uint _x = (x * 1e18) / decimals0;
            uint _y = (y * 1e18) / decimals1;
            uint _a = (_x * _y) / 1e18;
            uint _b = ((_x * _x) / 1e18 + (_y * _y) / 1e18);
            return (_a * _b) / 1e18; // x3y+y3x >= k
        } else {
            return x * y; // xy >= k
        }
    }

Attack Scenario

The value of _a = (x y ) / 1e18 = 0 due to rounding error when xy < 1e18. The rounding error can lead to the invariant k of stable pools equals zero, and the trader can steal whatever is left in the pool. The first liquidity provider can DOS the pair by: 1.mint a small amount of liquidity to the pool, 2. Steal whatever is left in the pool, 3. Repeat step 1, and step 2 until the overflow of the total supply. To prevent the issue of rounding error, the reserve of a pool should never go too small. The mint function which was borrowed from uniswapV2 has a minimum liquidity check of sqrt(a * b) > MINIMUM_LIQUIDITY; This, however, isn't safe enough to protect the invariant formula of stable pools

    uint internal constant MINIMUM_LIQUIDITY = 10 ** 3;
    // ...
    function mint(address to) external lock returns (uint liquidity) {
        (uint _reserve0, uint _reserve1) = (reserve0, reserve1);
        uint _balance0 = IERC20(token0).balanceOf(address(this));
        uint _balance1 = IERC20(token1).balanceOf(address(this));
        uint _amount0 = _balance0 - _reserve0;
        uint _amount1 = _balance1 - _reserve1;

        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
        if (_totalSupply == 0) {
            liquidity = Math.sqrt(_amount0 * _amount1) - MINIMUM_LIQUIDITY;
            _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
    // ...
    }

Recommendation: Recommend to add two restrictions on the first lp of stable pools:

  1. only allows equal amounts of liquidity.
  2. invariant _k should be larger than the MINIMUM_K
    function mint(address to) external lock returns (uint liquidity) {
        (uint _reserve0, uint _reserve1) = (reserve0, reserve1);
        uint _balance0 = IERC20(token0).balanceOf(address(this));
        uint _balance1 = IERC20(token1).balanceOf(address(this));
        uint _amount0 = _balance0 - _reserve0;
        uint _amount1 = _balance1 - _reserve1;

        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
        if (_totalSupply == 0) {
            liquidity = Math.sqrt(_amount0 * _amount1) - MINIMUM_LIQUIDITY;
            _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
+      if (stable) {
+          require(_amount0 * 1e18 / decimals0 == _amount1 * 1e18 / decimals1, "Pair: Stable pair
+            ,! must be equal amounts");
+          require(_k(_amount0, _amount1) > MINIMUM_K, "Pair: Stable pair must be above minimum
+            ,! k");
+      }
// ...
    }
BohdanHrytsak commented 7 months ago

Thank you for the submission.

A conclusion that for Velodorme is also relevant in our case I would also point out that duplicate #45 is more informative in this regard

In my opinion, this presentation is on the verge of medium/high (The conclusion may be revised)

We have a case that shows the loss of funds by certain users if the added initial liquidity is too small, that will make it possible to withdraw this liqudiity through manipulation. Of course, these funds will be small, but it will also not be just dust

The code refers to the part that is OOS, but due to certain criticality(high) and actions with other people's funds