`Pools._addLiquidity` implementation is not correct.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L90-L135

Vulnerability details

Impact

In Pools._addLiquidity's implemenation, there are two places that don't conform UniswapV2's whitepaper and algorithm, which might break the system.

Proof of Concept

There are two places in Pools._addLiquidity that don't conform with UniswapV2's implemenation. 1) Initialization of liquidity supply in Pools._addLiquidity, the initialization liquidity supply will be maxAmount0 + maxAmount1

 90         function _addLiquidity( bytes32 poolID, uint256 maxAmount0, uint256 maxAmount1, uint256 totalLiquidity ) internal returns(uint256 addedAmount0, uint256 addedAmount1, uint256 addedLiquidity)
 91                 {
...
 96                 // If either reserve is zero then consider the pool to be empty and that the added liquidity will become the initial token ratio
 97                 if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) )
 98                         {
 99                         // Update the reserves
100                         reserves.reserve0 += uint128(maxAmount0);
101                         reserves.reserve1 += uint128(maxAmount1);
102 
103                         // Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
104                         return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) ); <<<--- Here initialization liquidity supply is calculated
105                         }
106 
...

but in uniswapV2, the liquidity supply is calculated as:

    function mint(address to) external lock returns (uint liquidity) {
        ...
        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
        if (_totalSupply == 0) { <<<--- Here  is used to calculate initialization liquidity supply
            liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
           _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
        } else {
            liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);
        }
        ...
    }

And the quoting from the UniswapV Core paper:

This formula ensures that the value of a liquidity pool share at any time is essentially independent of the ratio at which liquidity was initially deposited

2) Another issue is that while calclate the liquidity, Pools._addLiquidity incorrectly assume a larger amount of token can maintain better numeric resolution in Pools.sol#L128-L135

128                 // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
129                 // Use whichever added amount was larger to maintain better numeric resolution.
130                 // Rounded down in favor of the protocol.
131                 if ( addedAmount0 > addedAmount1)
132                         addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
133                 else
134                         addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;
135                 }

while uniswapV2 using liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1); to calculate the the liquidity in UniswapV2Pair.sol#L123C13-L123C112

To prove the assumption is wrong, suppose there is a pool with tokenX=101e18, and tokenY=10e8 Alice stakes 10e18 TokenX and 99009900 TokenY to make the pool balance, in such case:

1. if we calcuate the lp supply by tokenX, we'll get liquidity = totalLiquidity*10e18/101e18
2. if we calcuate the lp supply by tokenY, we'll get liquidity = totalLiquidity*99009900 / 10e8 In pyhton, we can compare those two values as:

   In [7]: (10e18/101e18) > 99009900 / 10e8
   Out[7]: True

   Which means 99009900 / 10e8 is smaller, so the assumption is not correct

Tools Used

VS

Recommended Mitigation Steps

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol
index 7adb563..a5fe40f 100644
--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
@@ -101,7 +101,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable
                        reserves.reserve1 += uint128(maxAmount1);

                        // Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
-                       return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) );
+                       return ( maxAmount0, maxAmount1, (Math.sqrt(maxAmount0.mul(maxAmount1)));
                        }

                // Add liquidity to the pool proportional to the current existing token reserves in the pool.
@@ -125,6 +125,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable
                reserves.reserve0 += uint128(addedAmount0);
                reserves.reserve1 += uint128(addedAmount1);

+               addedLiquidity = min((totalLiquidity * addedAmount0) / reserve0, (totalLiquidity * addedAmount1) / reserve1);
                // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
                // Use whichever added amount was larger to maintain better numeric resolution.
                // Rounded down in favor of the protocol.

Assessed type

Uniswap

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

Added liquidity is now based on both addedAmount0 and addedAmount1: addedLiquidity = (totalLiquidity * (addedAmount0+addedAmount1) ) / (reserve0+reserve1);

Fixed in: https://github.com/othernet-global/salty-io/commit/0bb763cc67e6a30a97d8b157f7e5954692b3dd68

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as duplicate of #221

[c4-judge]

Picodes marked the issue as not selected for report

[InfectedIsm]

I disagree with the severity of this finding. First of all, the author didn't show how this could break the system (as he stated at the very beginning). The only thing he demonstrated is that the precision isn't always optimal for the calculation of the LP tokens to mint. We are very far from a direct loss of fund, or breaking of the system. At best, this is a questionnable design decision from the developer.

The main benefit of this decision is that such formula ensures that the initial liquidity ratio doesn’t affect the value of a pool share. But what is the impact of such a situation? It seems it hasn't been explained or demonstrated. In fact, the impact is explained in this article here (at the end) : its to account correctly for the fees. Which are zero on Salty.IO.

So, the only thing that holds here is that the precision isn’t always the best, in cases like the one shown by the author of this issue. That’s why I think this cannot be considered high. I would even say its a QA as no fund impact has been demonstrated.

[c4-sponsor]

othernet-global marked the issue as disagree with severity

[crazy4linux]

Hi @Picodes Thank you for your judging. But I don't think this report is a duplicate of #221 As I stated in my report, there are two issues in Pools._addLiquidity function:

1. Initialization of liquidity supply For this part, it's a duplicate of 221
2. Another issue is that while calclate the liquidity, Pools._addLiquidity incorrectly assume a larger amount of token can maintain better numeric resolution in Pools.sol#L128-L135 For this issue, it's not mentioned in 221

[Picodes]

The second part of the report shows a difference with Uniswap but isn't showing why this could be an issue.

As for the first part, I do agree with @InfectedIsm. The impact is very poorly demonstrated and isn't of high severity so I'll switch to partial credit. Thank you for flagging.

[c4-judge]

Picodes marked the issue as partial-25

[c4-judge]

Picodes changed the severity to 2 (Med Risk)