The protocol lacks slippage protection when removing liquidity

Summary

The protocol lacks slippage protection when removing liquidity, making it vulnerable to sandwich attacks, which can lead to losses.

Vulnerability Detail

The function UniV2PoolMigratorInit.init() is designed to remove all liquidity from a Uniswap V2 pool. The DAI obtained from this liquidity removal is then converted into NST, and MKR is converted into NGT. These tokens are subsequently added as liquidity in a new pool, pairing NST and NGT.

        uint256 daiAmtPrev = dai.balanceOf(pProxy);
        uint256 mkrAmtPrev = mkr.balanceOf(pProxy);

        GemLike(pairDaiMkr).transfer(pairDaiMkr, GemLike(pairDaiMkr).balanceOf(pProxy));
        PoolLike(pairDaiMkr).burn(pProxy);

        DaiNstLike daiNst = DaiNstLike(dss.chainlog.getAddress("DAI_NST"));
        MkrNgtLike mkrNgt = MkrNgtLike(dss.chainlog.getAddress("MKR_NGT"));

        uint256 daiAmt = dai.balanceOf(pProxy) - daiAmtPrev;
        uint256 mkrAmt = mkr.balanceOf(pProxy) - mkrAmtPrev;
        dai.approve(address(daiNst), daiAmt);
        mkr.approve(address(mkrNgt), mkrAmt);
        daiNst.daiToNst(pairNstNgt, daiAmt);
        mkrNgt.mkrToNgt(pairNstNgt, mkrAmt);
        PoolLike(pairNstNgt).mint(pProxy);

However, the protocol lacks slippage protection during the burn() process, making it vulnerable to sandwich attacks. Malicious users can buy tokens in advance to manipulate the price of one token, causing the amount received from burn() to be less than expected, resulting in a loss for the protocol. The attacker then sells the tokens afterward to profit from the manipulated price. In the Uniswap V2 router's remove liquidity function, we see that the protocol specifies amountAMin and amountBMin to set the minimum expected amounts to be received. https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol

    // **** REMOVE LIQUIDITY ****
    function removeLiquidity(
        address tokenA,
        address tokenB,
        uint liquidity,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) public virtual override ensure(deadline) returns (uint amountA, uint amountB) {
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        IUniswapV2Pair(pair).transferFrom(msg.sender, pair, liquidity); // send liquidity to pair
        (uint amount0, uint amount1) = IUniswapV2Pair(pair).burn(to);
        (address token0,) = UniswapV2Library.sortTokens(tokenA, tokenB);
        (amountA, amountB) = tokenA == token0 ? (amount0, amount1) : (amount1, amount0);
        require(amountA >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
        require(amountB >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
    }

However, in the init() function, these parameters are missing. Although the Maker Governance Contract has 99.97% of the liquidity(https://etherscan.io/token/0x9f8f72aa9304c8b593d555f12ef6589cc3a579a2#balances), the protocol is still susceptible to sandwich attacks. If such an attack occurs, the potential loss could be significant.

Impact

The protocol will incur losses.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/univ2-pool-migrator/deploy/UniV2PoolMigratorInit.sol#L41-L73

Tool used

Manual Review

Recommendation

It is recommended to implement slippage protection to mitigate this risk.

sherlock-audit / 2024-06-makerdao-endgame-judging

Bauer - The protocol lacks slippage protection when removing liquidity #25