c4-bot-10 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolStats.sol#L134-L140 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L157-L164

Vulnerability details

Impact

When a token is removed from the whitelist, this action has no impact on whether users can still swap through the token-WBTC and token-WETH pools. When users are swapping through the token-WBTC pool in particular (WBTC -> token), the arbitrage path used will be: WETH->token->WBTC->WETH. When there are arbitrage profits from this trade, they are first allocated to the token-WBTC pool, then in PoolStats:profitsForWhitelistedPools they are distributed across the (WETH,token), (WBTC,token), and (WBTC,WETH) pools. However in this case, since the token was removed from the whitelist, this logic will no longer be run, meaning that the WBTC-WETH pool will not receive their share of arbitrage rewards. Since these rewards are how LPs actually make money (no swap fees), this means that WETH-WBTC LPs are directly losing money.

Proof of Concept

Consider the case in which there is a token which had been whitelisted previously. This means that both the token-WETH and token-WBTC are whitelisted pools. Now lets assume that after a while, token has been removed from the whitelist, meaning both token-WETH and token-WBTC are both no longer whitelisted pools. Now consider what happens when a user swaps from WBTC -> token: Pools:swap -> Pools:_adjustReservesForSwapAndAttemptArbitrage -> Pools:_attemptArbitrage. The Pools:_attemptArbitrage function is defined as follows:

function _attemptArbitrage( IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn ) internal returns (uint256 arbitrageProfit )
    {
    ...
@>    (IERC20 arbToken2, IERC20 arbToken3) = _arbitragePath( swapTokenIn, swapTokenOut );

    (uint256 reservesA0, uint256 reservesA1) = getPoolReserves( weth, arbToken2);
    (uint256 reservesB0, uint256 reservesB1) = getPoolReserves( arbToken2, arbToken3);
    (uint256 reservesC0, uint256 reservesC1) = getPoolReserves( arbToken3, weth);

    // Determine the best amount of WETH to start the arbitrage with
    uint256 arbitrageAmountIn = _bisectionSearch(swapAmountInValueInETH, reservesA0, reservesA1, reservesB0, reservesB1, reservesC0, reservesC1 );

    // If arbitrage is viable, then perform it
@>    if (arbitrageAmountIn > 0)
@>        arbitrageProfit = _arbitrage(arbToken2, arbToken3, arbitrageAmountIn);
@>    }

The output of the ArbitrageSearch:_arbitragePath function will be (token, WBTC) for (arbToken2, arbToken3). This means that the arbitrage path will be: WETH -> token -> WBTC -> WETH. Notice that this includes the unwhitelisted pools (WETH,token) and (WBTC,token), while also including the whitelisted pool (WBTC,WETH).

This is fed into the Pools:_arbitrage -> PoolStats:_updateProfitsFromArbitrage. The PoolStats:_updateProfitsFromArbitrage function stores the arbitrage profit and is defined as follows:

function _updateProfitsFromArbitrage( IERC20 arbToken2, IERC20 arbToken3, uint256 arbitrageProfit ) internal
    {
    // Though three pools contributed to the arbitrage we can record just the middle one as we know the input and output token will be WETH
    bytes32 poolID = PoolUtils._poolID( arbToken2, arbToken3 );

@>    _arbitrageProfits[poolID] += arbitrageProfit;
    }

Note that this will increment the profit for the (token, WBTC) pool only.

Later during the upkeep flow, Upkeep:step7 is run which will calculate the profit of all the whitelisted pools, which is defined as follows:

function step7() public onlySameContract
    {
@>  uint256[] memory profitsForPools = pools.profitsForWhitelistedPools();

    bytes32[] memory poolIDs = poolsConfig.whitelistedPools();
    saltRewards.performUpkeep(poolIDs, profitsForPools );
    pools.clearProfitsForPools();
    }

It calls PoolStats:profitsForWhitelistedPools which is defined as follows:

function profitsForWhitelistedPools() external view returns (uint256[] memory _calculatedProfits)
    {
@>  bytes32[] memory poolIDs = poolsConfig.whitelistedPools();

    _calculatedProfits = new uint256[](poolIDs.length);
    _calculateArbitrageProfits( poolIDs, _calculatedProfits );
    }

Notice that this will only loop over the whitelisted pools (which (token, WBTC) is not) to calculate the profits for all pools along the arbitrage path for a given pool. Since (token, WBTC) will be ignored, this means that the (WBTC, WETH) pool, which was used for the arbitrage, will also not have their recorded arbitrage profit incremented. Later this profit is used to determine SALT emissions for each pool, meaning the (WBTC, WETH) pool will be cheated out of rewards.

Tools Used

Manual review

Recommended Mitigation Steps

The PoolStats:_calculateArbitrageProfits function should check recently unwhitelisted tokens/pools to see if any of the arbitrage profit should be allocated to pools which are still whitelisted.