Bauer - The protocol lacks slippage protection when executing the `exec()` function

[sherlock-admin3]

Bauer

High

The protocol lacks slippage protection when executing the exec() function

Summary

The protocol lacks slippage protection when executing the exec() function, making it vulnerable to sandwich attacks and resulting in potential financial losses.

Vulnerability Detail

In the FlapperUniV2.exec() function, the protocol first swaps DAI for GEM tokens, then adds liquidity using the remaining DAI and GEM tokens. The issue here is the lack of slippage protection when calling pair.mint() to add liquidity. This makes the protocol vulnerable to sandwich attacks, potentially leading to losses.

   // Swap
        console.log(address(pair));
        GemLike(dai).transfer(address(pair), _sell);
        (uint256 _amt0Out, uint256 _amt1Out) = daiFirst ? (uint256(0), _buy) : (_buy, uint256(0));
        console.log(_amt0Out);
        console.log(_amt1Out);
        pair.swap(_amt0Out, _amt1Out, address(this), new bytes(0));
        //

From the Uniswap V2 router's addLiquidity() function, we see that parameters amountAMin and amountBMin are used for slippage protection.

function addLiquidity(
        address tokenA,
        address tokenB,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
        (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
        TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
        liquidity = IUniswapV2Pair(pair).mint(to);
    }

However, these parameters are missing in the current implementation. Additionally, when the protocol uses _getAmountOut() to calculate the amount of GEM tokens needed for purchase and subsequently adds liquidity, the check require(_buy >= _sell * want / (uint256(pip.read()) * RAY / spotter.par()), "FlapperUniV2/insufficient-buy-amount") does not effectively provide slippage protection. Since _getAmountOut() is calculated within the protocol, if the pool has already been manipulated, the computed value may not be accurate.

  uint256 _sell = _getDaiToSell(lot, _reserveDai);

        uint256 _buy = _getAmountOut(_sell, _reserveDai, _reserveGem);

        require(_buy >= _sell * want / (uint256(pip.read()) * RAY / spotter.par()), "FlapperUniV2/insufficient-buy-amount");
        //

Testing showed a significant discrepancy between the _buy() and _sell * want / (uint256(pip.read()) * RAY / spotter.par()) calculations, indicating that this check does not offer adequate slippage protection.

 1079773319885657774
 1053763436384426919

FlapperUniV2SwapOnly.exec() also has the same issue.

Impact

The protocol incurs losses.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/dss-flappers/src/FlapperUniV2.sol#L141-L164

Tool used

Manual Review The recommended fix is to implement slippage protection to prevent such vulnerabilities.

Recommendation

[telome]

Liquidity is added in the same ratio as the current ratio of the reserves, hence slippage protection is unnecessary. For the swap, the require implements slippage protection. Note that, as per the rules: "The dss-flappers (SBE) solution assumes losing several percentages of funds by design during the Uniswap interactions due to slippage, MEV, sudden market movements, sandwich attacks, oracles imprecision and update resolution, etc."