Slippage and stale tx protections always disabled for protocol owned liquidity add , withdraw and swap

[c4-bot-9]

Lines of code

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/dao/DAO.sol#L321 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L132-L133 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L178

Vulnerability details

Impact

Key protocol liquidity management and swapping (formPOL and withdrawPOL) are prone to front-running and the loss can accumulate overtime with upkeep calls.

Note that despite the arbitrage mechanism, front-running is still possible and profitable.

Proof of Concept

In Upkeep.sol, perfromUpkeep() is permissionless and includes multiple protocol liquidity management and token-swapping calls. However, all of the protocol-owned liquidity (POL) related swaps and liquidity management have both slippage and stale tx protections disabled at all times.

step1(), step3(), step4() and step5() all contain unprotected swapping or POL liquidity management calls. In all cases, minimum out value is hardcoded to 0 and tx deadline is block.timestamp. 0 out value allows any amount of swap out or liquidity assets; block.timestamp means the transaction can be settled at any time.

For example, step5() swap remaining WETH (arbitrage profits) into SALT, which will be distributed as rewards across pools. But pools.depositSwapWithdraw() allows any amount of SALT output and allow performUpKeep() to settle at any time. step1() might call Dao.sol to withdrawPOL(). step3()step4() will swap tokens to deposit liquidity through formPOL().

See locations of above-mentioned cases:

//src/Upkeep.sol
    function step5() public onlySameContract {
...
        uint256 amountSALT = pools.depositSwapWithdraw(
            weth,
            salt,
            wethBalance,
            0,
            block.timestamp
        );
...

    function _formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountWETH
    ) internal {
        uint256 amountA = pools.depositSwapWithdraw(
            weth,
            tokenA,
            wethAmountPerToken,
            0,
            block.timestamp
        );
        uint256 amountB = pools.depositSwapWithdraw(
            weth,
            tokenB,
            wethAmountPerToken,
            0,
            block.timestamp
        );
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L132-L133)

//src/dao/DAO.sol
    function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
...
        collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );

    function withdrawPOL( IERC20 tokenA, IERC20 tokenB, uint256 percentToLiquidate ) external
...
        (uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block.timestamp );

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

Even though there is an arbitrage mechanism, front-running is still possible and profitable. And when both slippage and stale tx protections are disabled, front-running is easier and the loss will accumulate over time.

Tools Used

Manual

Recommended Mitigation Steps

Although difficult to implement in the permissionless context, consider implementing priceFeed to ensure a reasonable minimal output amount for WETH to usds, dai and salt swap.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as duplicate of #224

[c4-judge]

Picodes marked the issue as satisfactory