c4-bot-2 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L363 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L366-L378 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolsConfig.sol#L29

Vulnerability details

Impact

The Pools.swap function is used to swap one token for another via a direct whitelisted pool as described in the following natspec comment:

// Swap one token for another via a direct whitelisted pool.

Even though the natspec comment states that the swap should happen via a whitelisted pool there is no check implemented in the Pools.swap function to ensure that the Pool is whitelisted. The whitelisted pools are stored in the PoolsConfig._whitelist variable.

But during the Pools.swap function execution flow there was no reference made to the PoolsConfig._whitelist Bytes32Set to verify whether the pool that contains the swapTokenIn and swapTokenOut tokens is whitelisted even though the Natspec comments states it should be.

Hence there seems to be a lack of conditional check in the Pools.swap function which deviates the functionality from the expected behavior.

Proof of Concept

    // Having simpler swaps without multiple tokens in the swap chain makes it simpler (and less expensive gas wise) to find suitable arbitrage opportunities.

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

    function swap( IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
        // Confirm and adjust user deposits
        mapping(IERC20=>uint256) storage userDeposits = _userDeposits[msg.sender];

        require( userDeposits[swapTokenIn] >= swapAmountIn, "Insufficient deposited token balance of initial token" );
        userDeposits[swapTokenIn] -= swapAmountIn;

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

        // Deposit the final tokenOut for the caller
        userDeposits[swapTokenOut] += swapAmountOut;
        }

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

    EnumerableSet.Bytes32Set private _whitelist;

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to implement a check in the Pools.swap function to verify whether the pool containing the tokens to be swapped are indeed whitelisted in the DEX as the natspec comment states. This can be done by checking the poolID of the tokens to be swapped against the whitelisted pools stored in the PoolsConfig._whitelist variable.

Assessed type

Other

c4-judge commented 5 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 5 months ago

othernet-global (sponsor) disputed

othernet-global commented 5 months ago

Liquidity can only be added to whitelisted pools. If a pool was previously whitelisted, liquidity added, and then unwhitelisted - it is acceptable that users can still swap on the unwhitelisted pool as it has no effect on the exchange.