c4-bot-6 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Malicious users can redefine the ratio of pool

Proof of Concept

In the removeLiquidity function, we can see that there is a restriction to prevent the user from setting the liquidity to 0. However, only reserves.reserve0>PoolUtils.DUST is restricted, not reserves.reserve1

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

    function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
        {
...
        // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
-->        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal"); // Limit the balance after removal but only reserves.reserve0

...
        }

A malicious user can first deposit a certain amount of liquidity and then trade through the swap method to make reserve1 very close to PoolUtils.DUST, and then remove the liquidity to make reserves.reserve1=0. Then add liquidity to redefine the ratio of the currency pair. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L90-L135

    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 ) ) // Redefine the ratio of the currency pair by making 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) );
            }

...
        }

For example, define the original ratio of 1:1000 as 10:1. arbitrage is achieved by taking the reserve0 deposited by the previous transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

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

        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Modify the code to

require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Invalid Validation

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #647

c4-judge commented 8 months ago

Picodes changed the severity to 2 (Med Risk)