c4-bot-6 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

Rounding errors during repeated depositing/withdrawing of tokens may allow attackers to exploit value discrepancies
The asset ratio within the pool can experience a slight shift due to leak of one token reserve.

Proof of Concept

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L140

When a user invokes addLiquidity() to provide liquidity in specific pools, the internal _addLiquidity() function is called after checking parameters. This function calculates the liquidity for the deposited tokens.

function _addLiquidity( bytes32 poolID, uint256 maxAmount0, uint256 maxAmount1, uint256 totalLiquidity ) internal returns(uint256 addedAmount0, uint256 addedAmount1, uint256 addedLiquidity)
        {
        PoolReserves storage reserves = _poolReserves[poolID];
        uint256 reserve0 = reserves.reserve0;
        uint256 reserve1 = reserves.reserve1;

        // If either reserve is zero then consider the pool to be empty and that the added liquidity will become the initial token ratio
        if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) )
            {
            // Update the reserves
            reserves.reserve0 += uint128(maxAmount0);
            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) );
            }

        // Add liquidity to the pool proportional to the current existing token reserves in the pool.
        // First, try the proportional amount of tokenB for the given maxAmountA
        uint256 proportionalB = ( maxAmount0 * reserve1 ) / reserve0;

        // proportionalB too large for the specified maxAmountB?
        if ( proportionalB > maxAmount1 )
            {
            // Use maxAmountB and a proportional amount for tokenA instead
            addedAmount0 = ( maxAmount1 * reserve0 ) / reserve1;
            addedAmount1 = maxAmount1;
            }
        else
            {
            addedAmount0 = maxAmount0;
            addedAmount1 = proportionalB;
            }

        // Update the reserves
        reserves.reserve0 += uint128(addedAmount0);
        reserves.reserve1 += uint128(addedAmount1);

        // 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.
        if ( addedAmount0 > addedAmount1)
            addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
        else
            addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;
        }

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

In cases where both tokens have different decimals(with same too), the precision of liquidity can vary, potentially leading to rounding errors. An attacker can manipulate parameters to cause reserve leakage, resulting in a tiny loss in most of the trades.

eg addedLiquidity1 > addedLiquidity2 and corressponding to addedLiquidity1 liquiduty given to user. Now during withdrawl

    function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
        {
        //////////////////////////
        // Determine how much liquidity is being withdrawn and round down in favor of the protocol
        PoolReserves storage reserves = _poolReserves[poolID];
        reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
        reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

        reserves.reserve0 -= uint128(reclaimedA);
        reserves.reserve1 -= uint128(reclaimedB);

        ///////////////////////////
        emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
        }

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L170

we can see in such case reclaimedB is more than it should be and protocol will continuously leak a tiny amouny in most of the trade

Reputated DEX, lending protocols etc mitigate such rounding issue in favour of protocol as already we have seen may big hacks(recent kyberswap) due to rounding issue.

addedLiquidity= min((totalLiquidity * addedAmount0) / reserve0,(totalLiquidity * addedAmount1) / reserve1 )

Uniswap takes minLiquidity from both of two and it seems a good solution

Tools Used

Manual

Recommended Mitigation Steps

Ensure minimum liquidity by taking the minimum of addedLiquidity1 and addedLiquidity2

Assessed type

Other

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #237