In `DAO::formPOL()`, `DAO::withdrawPOL()`, `Liquidity::_dualZapInLiquidity()`, swaps are executed without slippage protection, invites sandwich attacks and will cause loss of funds

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L321 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L372 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L62-L72

Vulnerability details

Impact

• The token receiving parameters are set to zero in the following functions:

  • DAO::formPOL()

    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 ); // @audit - This line hardcodes 0 for parameter `minLiquidityReceived` in Pools::addLiquidity()
    
    emit POLFormed(tokenA, tokenB, amountA, amountB);
    }

  • DAO::withdrawPOL()

    function withdrawPOL( IERC20 tokenA, IERC20 tokenB, uint256 percentToLiquidate ) external
    {
    require(msg.sender == address(liquidizer), "DAO.withdrawProtocolOwnedLiquidity is only callable from the Liquidizer contract" );
    
    bytes32 poolID = PoolUtils._poolID(tokenA, tokenB);
    uint256 liquidityHeld = collateralAndLiquidity.userShareForPool( address(this), poolID );
    if ( liquidityHeld == 0 )
        return;
    
    uint256 liquidityToWithdraw = (liquidityHeld * percentToLiquidate) / 100;
    
    // Withdraw the specified Protocol Owned Liquidity
    (uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block. timestamp ); // @audit - This line hardcodes 0s for parameters `minReclaimedA` and `minReclaimedB` in Pools::removeLiquidity();
    
    // Send the withdrawn tokens to the Liquidizer so that the tokens can be swapped to USDS and burned as needed.
    tokenA.safeTransfer( address(liquidizer), reclaimedA );
    tokenB.safeTransfer( address(liquidizer), reclaimedB );
    
    emit POLWithdrawn(tokenA, tokenB, reclaimedA, reclaimedB);
    }

  • Liquidity::_dualZapInLiquidity()

    function _dualZapInLiquidity(IERC20 tokenA, IERC20 tokenB, uint256 zapAmountA, uint256 zapAmountB ) internal returns (uint256 amountForLiquidityA, uint256 amountForLiquidityB  )
    {
    (uint256 reserveA, uint256 reserveB) = pools.getPoolReserves(tokenA, tokenB);
    (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;
        zapAmountB += pools.depositSwapWithdraw( tokenA, tokenB, swapAmountA, 0, block.timestamp ); // @audit This line hardcodes 0 for parameter `minAmountOut` in Pools::depositSwapWithdraw() 
        }
    
    // 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;
        zapAmountA += pools.depositSwapWithdraw( tokenB, tokenA, swapAmountB, 0, block.timestamp ); // @audit This line hardcodes 0 for parameter `minAmountOut` in Pools::depositSwapWithdraw() 
        }
    
    return (zapAmountA, zapAmountB);
    }

  • The zero here means that the sender is allowing unpredictable slippage up to 100%. MEV bots can do a flash loan sandwich attack that will result in a huge slippage.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Parameterize the token receiving value.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as duplicate of #805

[c4-judge]

Picodes marked the issue as not a duplicate

[Picodes]

How this is still actionable in a real scenario is not discussed.

[c4-judge]

Picodes marked the issue as duplicate of #224

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory