search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Swaps can be made for token not in the whitelist #648

Open c4-bot-1 opened 7 months ago

c4-bot-1 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L366 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L382 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L395 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/SaltRewards.sol#L117

Vulnerability details

Vulnerability Details:

The protocol implements a swap feature to allow users to swap one token for another via a direct whitelisted pool. The swap function also checks if profitable arbitrage is possible after the user swap that was just made and if there is it will perform it. This profit is then distributed to liquidity providers of the pools used in the arbitrage.

    // Swap one token for another via a direct whitelisted pool.
    // Having simpler swaps without multiple tokens in the swap chain makes it simpler (and less expensive gas wise) to find suitable arbitrage opportunities.
    // Cheap arbitrage gas-wise is important as arbitrage will be atomically attempted with every user swap transaction.
    // Requires that the first token in the chain has already been deposited for the caller.
    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;
    }

Currently the swap function has no checks to ensure only whitelisted tokens can be swapped, this isn’t a problem for random tokens as they would not have any current liquidity so the swap would fail. However, if a token has recently been voted off the whitelist by the DAO the pool will still have liquidity and a swap can be made even though it shouldn’t.

Impact:

Proof Of Concept

function testSwapWithTokensNotInWL() public {
        IERC20 tokenIn = tokens[0];
        IERC20 tokenOut = tokens[1];
        uint256 swapAmountIn = 1 ether;
        //add token to wl
        vm.startPrank(address(dao));
        poolsConfig.whitelistPool(pools, tokenIn, weth);
        poolsConfig.whitelistPool(pools, tokenIn, wbtc);
        // check if token in wl, should be true
        (bool tokenHasBeenWL) = poolsConfig.tokenHasBeenWhitelisted( tokenIn, wbtc, weth );
        assertTrue(tokenHasBeenWL, "Token should be whitelisted");
        vm.stopPrank();
        vm.startPrank(DEPLOYER);
        // Ensure sufficient balance of tokenIn is deposited for the swap
        tokenIn.approve(address(pools), swapAmountIn);
        pools.deposit(tokenIn, swapAmountIn);
        vm.stopPrank();
        // remove token from wl
        vm.startPrank(address(dao));
        poolsConfig.unwhitelistPool(pools, tokenIn, weth);
        poolsConfig.unwhitelistPool(pools, tokenIn, wbtc);
        vm.stopPrank();
        // check if token in wl, should be false
        (bool tokenHasBeenWLAft) = poolsConfig.tokenHasBeenWhitelisted( tokenIn, wbtc, weth );
        assertFalse(tokenHasBeenWLAft, "Token should not be whitelisted");

        vm.startPrank(DEPLOYER);
        // Attempt a swap 
        pools.swap(tokenIn, tokenOut, swapAmountIn, 0, block.timestamp + 1 minutes);
        // Get the amount out after swap
        uint256 swapAmountOut = pools.depositedUserBalance(DEPLOYER, tokenOut);
        // Ensure swapAmountOut is greater than zero
        assertTrue(swapAmountOut > 0, "Swap amount out should be greater than zero.");
        vm.stopPrank();
    }

Tools Used:

Recommendation:

Limit all swaps to only whitelisted pools by adding a check to all function that can be used to perform a swap.

require(poolsConfig.isWhitelisted(poolID), "Invalid pool");

Assessed type

Invalid Validation

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #965

c4-judge commented 6 months ago

Picodes changed the severity to QA (Quality Assurance)

0xCiphky commented 6 months ago

Could you take another look at this? The primary report fails to grasp the true impact:

Thank you

Picodes commented 6 months ago

@0xCiphky thanks for your comment.

I still don't get what the fundamental error would be.

LPs of the unwhitelisted pool aren't earning anymore from arbitrages as desired, and the fact that the pool could still lead to arbitrages and be used isn't detrimental to anyone except the LPs of this pool who should leave. In addition, it could lead to other LPs earning more as you are showing which would be a bonus.