Unwhitelisted tokens can be swapped in Pools and returned by PoolStats::arbitrageIndicies

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolStats.sol#L143-L146 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L205-L215 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L235-L403

Vulnerability details

Description

In the Pool contract, it is currently possible to use the deposit function to send any token, even those that are not whitelisted. The swap functions (swap, depositSwapWithdraw, depositDoubleSwapWithdraw) do not check if the tokens involved are whitelisted. Additionally, the arbitrage path is not deleted during the unwhitelisting process, allowing users to still process a swap. The PoolStats::arbitrageIndicies function can return paths of former tokens that were whitelisted. However, profits for arbitrage in this unwhitelisted pool will still be collected by Upkeep::performUpkeep.

Impact

Impact: High

• DAO cannot prevent users from using an unwhitelisted token.

Likelihood : Low

• This bug persists only until the unwhitelisted pool is empty. Moreover, once a pool is unwhitelisted, Liquidity Providers no longer receive rewards, encouraging them to withdraw all their shares and empty the pool.

Proof of Concept

Foundry PoC added in Arbitrage.t.sol

function testDepositAndSwapUnwhitelistedToken() public {
    address bob = address(0x2222);

    vm.startPrank(alice);
    IERC20 token1 = new TestERC20("TEST1", 18);
    token1.transfer(bob, 1000 ether);

    _setupTokenForTesting(token1);

    // DAO votes to unwhitelist a pool.
    vm.startPrank(address(dao));
    poolsConfig.unwhitelistPool(pools, token1, weth);
    poolsConfig.unwhitelistPool(pools, token1, wbtc);
    vm.stopPrank();

    // Bob deposits the unwhitelisted token
    vm.startPrank(bob);
    token1.approve(address(pools), type(uint).max);
    pools.deposit(token1, 100 ether);

    // Swap the unwhitelisted token with weth
    uint amountOut = pools.swap(
        token1,
        weth,
        100 ether,
        50 ether,
        block.timestamp
    );
    console.log(amountOut);
    pools.withdraw(weth, amountOut);

    assertTrue(weth.balanceOf(bob) == amountOut);
}

Recommended Mitigation

One efficient solution to address this issue is to check if tokens are whitelisted in _adjustReservesForSwapAndAttemptArbitrage. This function is the first common function called by all swap functions. Additionally, the same require statement can be added to the deposit function to prevent any user from depositing unwhitelisted tokens into the _userDeposits variable.

Pools.sol

function _adjustReservesForSwapAndAttemptArbitrage(
    IERC20 swapTokenIn,
    IERC20 swapTokenOut,
    uint256 swapAmountIn,
    uint256 minAmountOut
) internal returns (uint256 swapAmountOut) {
+    require(
+        poolsConfig.tokenHasBeenWhitelisted(swapTokenIn, wbtc, weth),
+        "Token is not whitelisted"
+    );
+    require(
+        poolsConfig.tokenHasBeenWhitelisted(swapTokenOut, wbtc, weth),
+        "Token is not whitelisted"
+    );
    ...
}

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #828

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid