c4-bot-2 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolUtils.sol#L54 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolUtils.sol#L67 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L382-L391 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L387 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L350-L353 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L235 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L257-L268

Vulnerability details

Impact

No slippage protection, so attackers can build bots to front-run trades

Proof of Concept

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

When _placeInternalSwap is executed, the parameters are only pools, token address, amountIn, maximumInternalSwapPercentTimes1000. The problem occurs here: https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolUtils.sol#L67

        swapAmountOut = pools.depositSwapWithdraw(
            tokenIn,
            tokenOut,
            amountIn,
            0,
            block.timestamp
        );

In the pools.depositSwapWithdraw function, the fourth parameter is minAmountOut. Setting it to 0 here will cause attackers to monitor the memory pool and exploiting slippage to steal funds.

Next, I prove that this vulnerability can be exploited by an attacker:

The value of swapAmountOut depends on the pools.depositSwapWithdraw function. In the depositSwapWithdraw function, we can see that the fourth parameter is set to 0. https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L382-L391

    function depositSwapWithdraw(
        IERC20 swapTokenIn,
        IERC20 swapTokenOut,
        uint256 swapAmountIn,
        uint256 minAmountOut,
        uint256 deadline
    )
        external
        nonReentrant
        ensureNotExpired(deadline)
        returns (uint256 swapAmountOut)

The value of swapAmountOut depends on the _adjustReservesForSwapAndAttemptArbitrage function,minAmountOut==0 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L387

        swapAmountOut = _adjustReservesForSwapAndAttemptArbitrage(
            swapTokenIn,
            swapTokenOut,
            swapAmountIn,
            minAmountOut
        );

The value of swapAmountOut depends on the _adjustReservesForSwapAndAttemptArbitrage function, minAmountOut==0 will allow hackers to manipulate the value of swapAmountOut, so that swapAmountOut is just >= minAmountOut, resulting in a very small number of tokens output and a large amount of profits. https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L350-L353 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L235 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L257-L268

        swapAmountOut = _adjustReservesForSwap(
            swapTokenIn,
            swapTokenOut,
            swapAmountIn
        );

        // Make sure the swap meets the minimums specified by the user
        require(
            swapAmountOut >= minAmountOut,
            "Insufficient resulting token amount"
        );

        if (flipped)
            {
            reserve1 += amountIn;
            amountOut = reserve0 * amountIn / reserve1;
            reserve0 -= amountOut;
            }
        else
            {
            reserve0 += amountIn;
            amountOut = reserve1 * amountIn / reserve0;
            reserve1 -= amountOut;
            }

The attacker only needs to increase the gas and change the reserve amount first to complete an attack. It can be understood as an MEV robot.

Tools Used

Manual review

Recommended Mitigation Steps

Calculate minimum output and check for slippage

Assessed type

Math

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #761

c4-judge commented 6 months ago

Picodes marked the issue as not a duplicate

Picodes commented 6 months ago

There is a slippage check in _adjustReservesForSwapAndAttemptArbitrage