Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L316-L324

Vulnerability details

Impact

When DAO.formPol() is called by the UpKeep contract USD/DAI & SALT/USDS are being deposited to form the DAOs Protocol owned liquidity in those pools. However no appropriate deadline and minLiquidityReceived parameters are provided when depositLiquidityAndIncreaseShare get's called inside formPol(), which creates an opportunity for sandwich attacks.

Proof of Concept

This is the function inside DAO responsible for adding liquidity inside the SALT/USDS & USDS/DAI pools:

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L316-L324

 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,
            //@audit-issue - slippage protection ?
            0,  <------- minLiquidityReceived
            block.timestamp,  <------- deadline
            true
        );

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

As you can see the parameter values for minLiquidityReceived & deadline are 0 & block.timestamp respectively.

This means that the tokens can be deposited against any liquidity share in return , even if it is quite unprofitable (what will happen when someone decides to front run it).

Setting the deadline to block.timestamp means that the trx can be executed at any time (because block.timestamp will always be the current timestamp). This allows the trx to be held in the mempool for unbounded amount of time and be executed later when the conditions are more favourable.

I would like to make a clear differentiation here with the internal swaps in the protocol which follow the same pattern (not providing min liquidity and setting block.timestamp as deadline). The developer has clearly explained that before each swap an Atomic Arbitrage occurs that extracts all MEV-able value to the protocol, which is why those trxs are considered guarded against front-running attacks even with the deadline/minLiquidity params not being set

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolUtils.sol#L42-L53

    // Swaps tokens internally within the protocol with amountIn limited to be a certain percent of the reserves.
    // The limit, combined with atomic arbitrage makes sandwich attacks on this swap less profitable (even with no slippage being specified).
    // This is due to the first swap of the sandwich attack being offset by atomic arbitrage within its same transaction.
    // This effectively reverses some of the initial swap of the attack and creates an initial loss for the attacker proportional to the size of that swap (if they were to swap back immediately).

    // Simulations (see Sandwich.t.sol) show that when sandwich attacks are used, the arbitrage earned by the protocol sometimes exceeds any amount lost due to the sandwich attack itself.
    // The largest swap loss seen in the simulations was 1.8% (under an unlikely scenario).   More typical losses would be 0-1%.
    // The actual swap loss (taking arbitrage profits generated by the sandwich swaps into account) is dependent on the multiple pool reserves involved in the arbitrage (which are encouraged by rewards distribution to create more reasonable arbitrage opportunities).

    // Also, the protocol awards a default 5% of pending arbitrage profits to users that call Upkeep.performUpkeep().
    // If sandwiching performUpkeep (where these internal swaps happen) is profitable it would encourage "attackers" to call performUpkeep more often.
    // With that in mind, the DAO could choose to lower the default 5% reward for performUpkeep callers - effectively making sandwich "attacks" part of the performUpkeep mechanic itself.
    function _placeInternalSwap( IPools pools, IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 maximumInternalSwapPercentTimes1000 ) internal returns (uint256 swapAmountIn, uint256 swapAmountOut)

However when it come to depositing liquidity (not swapping), no such arbitrage occurs to protect the trx from being sandwiched.

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L140

This is why I'm emphasizing on the deposits.

The amounts deposited are for the whole DAO, which means they are going to get quite substantial if the protocol gains traction, which would be a big motivation for advantageous parties to exploit it

All of the above applies to the DAO.withdrawPOL() (called by Liquidizer.sol) function as well. Difference here is that no minimums are provided for minReclaimedA & minReclaimedA against the liquidity being withdrawn. Withdrawing liquidity just as depositing is not protected by atomic arbitrage

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider setting some percentLimit when depositing or some other type of constraint as you have done with internal swaps, so that the chance for exploiting deposits is reduced to minimum

Assessed type

MEV

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

Deposits that form protocol owned liquidity are not protected against sandwich attacks #236