Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L321

Vulnerability details

Impact

Whenever upkeep, dai and usds continue to be frozen to DAO.

Proof of Concept

In step3, 4 of upkeep, 5% and 20% of profits which DAO earned is deposited [usds/dai], [salt/usds] pools. Then, it deposits by using zapping. But when zapSwap, it attempts arbitrage so the rate of pool changes. This change of rate increases the amount of tokens returned to DAO. Salt returned can be used but usds and dai cannot be used. So they are frozen.

The functions which are called in step 3, 4 of upkeep are as follows.

    function _formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountWETH) internal
        {
129     uint256 wethAmountPerToken = amountWETH >> 1;

        // Swap WETH for the specified tokens
132     uint256 amountA = pools.depositSwapWithdraw( weth, tokenA, wethAmountPerToken, 0, block.timestamp );
133     uint256 amountB = pools.depositSwapWithdraw( weth, tokenB, wethAmountPerToken, 0, block.timestamp );

        // Transfer the tokens to the DAO
        tokenA.safeTransfer( address(dao), amountA );
        tokenB.safeTransfer( address(dao), amountB );

        // Have the DAO form POL
        dao.formPOL(tokenA, tokenB, amountA, amountB);
        }

    // 3. Convert a default 5% of the remaining WETH to USDS/DAI Protocol Owned Liquidity.
    function step3() public onlySameContract
        {
        uint256 wethBalance = weth.balanceOf( address(this) );
        if ( wethBalance == 0 )
            return;

        // A default 5% of the remaining WETH will be swapped for USDS/DAI POL.
        uint256 amountOfWETH = wethBalance * stableConfig.percentArbitrageProfitsForStablePOL() / 100;
        _formPOL(usds, dai, amountOfWETH);
        }

    // 4. Convert a default 20% of the remaining WETH to SALT/USDS Protocol Owned Liquidity.
    function step4() public onlySameContract
        {
        uint256 wethBalance = weth.balanceOf( address(this) );
        if ( wethBalance == 0 )
            return;

        // A default 20% of the remaining WETH will be swapped for SALT/USDS POL.
        uint256 amountOfWETH = wethBalance * daoConfig.arbitrageProfitsPercentPOL() / 100;
        _formPOL(salt, usds, amountOfWETH);
        }

As you can see, in step 3, 5% of weth as reward is swapped to [usds/dai] and in step 4, 20% of weth is swaped to [salt/usds]. On L129, 132 and 133 each half amount of weth is swapped and forms POL.

    File: DAO.sol
    function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "DAO.formPOL is only callable from the Upkeep contract" );

320     // Use zapping to form the liquidity so that all the specified tokens are used
321     collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );

        emit POLFormed(tokenA, tokenB, amountA, amountB);
        }

On DAO.sol#L321 by useZapping == true funds are deposited. The doc on L320 indicated that all tokens are used because it used zapping. But this is wrong. When zapSwap, it attempts arbitrage and rate of the pool is changed a little so all tokens cannot be used. Liquidity.sol#depositLiquidityAndIncreaseShare function is as follows.

    function depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 deadline, bool useZapping ) external nonReentrant ensureNotExpired(deadline) returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        require( PoolUtils._poolID( tokenA, tokenB ) != collateralPoolID, "Stablecoin collateral cannot be deposited via Liquidity.depositLiquidityAndIncreaseShare" );

@>      return _depositLiquidityAndIncreaseShare(tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, useZapping);
        }

Liquidity.sol#_depositLiquidityAndIncreaseShare function is as follows.

    function _depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        ...

        // Balance the token amounts by swapping one to the other before adding the liquidity?
        if ( useZapping )
@>          (maxAmountA, maxAmountB) = _dualZapInLiquidity(tokenA, tokenB, maxAmountA, maxAmountB );

        ...
        }

Because useZapping == true, it zaps through _dualZapLiquidity() function.

    function _dualZapInLiquidity(IERC20 tokenA, IERC20 tokenB, uint256 zapAmountA, uint256 zapAmountB ) internal returns (uint256 amountForLiquidityA, uint256 amountForLiquidityB  )
        {
        (uint256 reserveA, uint256 reserveB) = pools.getPoolReserves(tokenA, tokenB);
53      (uint256 swapAmountA, uint256 swapAmountB ) = PoolMath._determineZapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB );

        // tokenA is in excess so swap some of it to tokenB?
        if ( swapAmountA > 0)
            {
            tokenA.approve( address(pools), swapAmountA );

            // Swap from tokenA to tokenB and adjust the zapAmounts
            zapAmountA -= swapAmountA;
62          zapAmountB += pools.depositSwapWithdraw( tokenA, tokenB, swapAmountA, 0, block.timestamp );
            }

        // tokenB is in excess so swap some of it to tokenA?
        else if ( swapAmountB > 0)
            {
            tokenB.approve( address(pools), swapAmountB );

            // Swap from tokenB to tokenA and adjust the zapAmounts
            zapAmountB -= swapAmountB;
72          zapAmountA += pools.depositSwapWithdraw( tokenB, tokenA, swapAmountB, 0, block.timestamp );
            }

        return (zapAmountA, zapAmountB);
        }

As we can see on L53, it calculates the amount for zapping and on L62 and L72 it swaps tokens. The problem is to attempt arbitrage at the same time.

    File: Pools.sol
    function depositSwapWithdraw(IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
        // Transfer the tokens from the sender - only tokens without fees should be whitelisted on the DEX
        swapTokenIn.safeTransferFrom(msg.sender, address(this), swapAmountIn );

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

        // Send tokenOut to the user
        swapTokenOut.safeTransfer( msg.sender, swapAmountOut );
        }

_adjustReservesForSwapAndAttemptArbitrage() function is as follows.

    function _adjustReservesForSwapAndAttemptArbitrage( IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut ) internal returns (uint256 swapAmountOut)
        {
        // Place the user swap first
        swapAmountOut = _adjustReservesForSwap( swapTokenIn, swapTokenOut, swapAmountIn );

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

        // The user's swap has just been made - attempt atomic arbitrage to rebalance the pool and yield arbitrage profit
356     uint256 arbitrageProfit = _attemptArbitrage( swapTokenIn, swapTokenOut, swapAmountIn );

        emit SwapAndArbitrage(msg.sender, swapTokenIn, swapTokenOut, swapAmountIn, swapAmountOut, arbitrageProfit);
        }

Above it attempts arbitrage on L356 so the rate is changed a little because of imbalance.

On the other hand, upkeep is called frequently.

Tools Used

Manual Review

Recommended Mitigation Steps

DAO.sol#formPOL has to be modified as follows so that it can be reused at next time.

    function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
        {
        require( msg.sender == address(exchangeConfig.upkeep()), "DAO.formPOL is only callable from the Upkeep contract" );

        // Use zapping to form the liquidity so that all the specified tokens are used
-       collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );
+       (addedAmountA, addedAmountB, ) = collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );
+       if(amountA > addedAmountA){
+           uint256 amountOut = pools.depositSwapWithdraw(tokenA, weth, amountA - addedAmountA, 0, block.timestamp);
+           weth.safeTransfer(msg.sender, amountOut);
+       }
+       if(amountB > addedAmountB){
+           uint256 amountOut = pools.depositSwapWithdraw(tokenB, weth, amountB - addedAmountB, 0, block.timestamp);
+       }

        emit POLFormed(tokenA, tokenB, amountA, amountB);
        }

Se can reuse weth sent to upkeep.

Assessed type

Other

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

Funds continue to be frozen to DAO whenever upkeep. #843