c4-bot-7 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

If zapping fails or loses precision, some arbitrage profits will be stuck in Dao.sol

Proof of Concept

In normal circumstances, when a user adds liquidity (_depositLiquidityAndIncreaseShare()), regardless whether zapping is enabled or not, they are assured to claim any unused token back due to checks on the token amount differences.

//src/staking/Liquidity.sol
    function _depositLiquidityAndIncreaseShare(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        bool useZapping
    )
...
       // If any of the user's tokens were not used, then send them back
        if (addedAmountA < maxAmountA)
|>            tokenA.safeTransfer(msg.sender, maxAmountA - addedAmountA);

        if (addedAmountB < maxAmountB)
|>            tokenB.safeTransfer(msg.sender, maxAmountB - addedAmountB);
...

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L110-L114)

However, such checks on token amount difference are not implemented during protocol upkeeps. In Upkeep.sol, step3() and step4() both add POL to DAO in a manner that amountA and amountB will always equal in ETH value, which in most cases will require zapping to swap and balance ratio of amountA/amountB for the pools. Zapping is always enabled in dao.formPOL().

//src/Upkeep.sol
    function _formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountWETH
    ) internal {
          //@audit note: this split weth in half for amountA and amounB
|>        uint256 wethAmountPerToken = amountWETH >> 1;
        uint256 amountA = pools.depositSwapWithdraw(
            weth,
            tokenA,
            wethAmountPerToken,
            0,
            block.timestamp
        );
        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);
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L140) But formPOL() doesn't check for actual input amount of tokenA/tokenB, and doesn't know whether zapping succeeds or whether there are still some tokens unused due to precision.

//src/dao/DAO.sol
    function formPOL(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 amountA,
        uint256 amountB
    ) external {
...
        //@audit : although zapping is set as true, but no check on actual input token amount of A and B, if zapping fails or loses precision, tokenA or tokenB will be left in DAO.sol
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            tokenA,
            tokenB,
            amountA,
            amountB,
            0,
            block.timestamp,
|>          true
        );
        emit POLFormed(tokenA, tokenB, amountA, amountB);
    }

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L321) Since DAO.sol is caller for depositLiquidityAndIncreaseShare(), any left over amountA or amountB will be sent to DAO.sol. And DAO.sol doesn't have methods to recover tokens, nor does DAO.sol scrape token balance in other flows. Funds will be stuck in DAO in the form of usds, salt or dai.

Note when zapping fails it will not revert but only passes the original amountA and amountB to pools.addLiquidity().

Upkeep.sol shouldn't assume zapping works 100% or will not leave any tokens behind. And the arbitrage profits stuck can accumulate over time.

Tools Used

Manual

Recommended Mitigation Steps

In formPOL() in DAO.sol, check the return values of collateralAndLiquidity.depositLiquidityAndIncreaseShare() and properly handle the left over tokenA and tokenB. For example, sending left over tokens back to Upkeep.sol and salt can be added back to salt rewards.

Assessed type

Other

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #721