Slippage protection is missing when executing limit orders

Summary

When executing limit orders, the lack of slippage protection means that if the price from Chainlink is manipulated, the keeper fee could be very high, resulting in users receiving significantly reduced settlement funds.

Vulnerability Detail

The execution time of a limit order is uncertain, so the keeper fee is dynamically calculated during execution. When calling LimitOrder.executeLimitOrder() to execute the limit order, the protocol invokes _closePosition(). Within this function, the protocol verifies that the price of rETH is within the threshold range.

 (uint256 price, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice();

        // Check that the minimum time delay is reached before execution
        if (block.timestamp < order.executableAtTime)
            revert FlatcoinErrors.ExecutableTimeNotReached(order.executableAtTime);

        uint256 minFillPrice;

        if (price <= _limitOrder.priceLowerThreshold) {
            minFillPrice = 0; // can execute below lower limit price threshold
        } else if (price >= _limitOrder.priceUpperThreshold) {
            minFillPrice = _limitOrder.priceUpperThreshold;
        } else {
            revert FlatcoinErrors.LimitOrderPriceNotInRange(
                price,
                _limitOrder.priceLowerThreshold,
                _limitOrder.priceUpperThreshold
            );
        }

Then, it begins calculating the keeper fee. The calculation of the keeper fee involves several steps: firstly, the protocol fetches the price of ETH from Chainlink and calculates the gas fee using this price, converting it to USD.

   uint256 ethPrice18;
        {
            (, int256 ethPrice, , uint256 ethPriceupdatedAt, ) = _ethOracle.latestRoundData();
            if (block.timestamp >= ethPriceupdatedAt + _STALENESS_PERIOD) revert FlatcoinErrors.ETHPriceStale();
            if (ethPrice <= 0) revert FlatcoinErrors.ETHPriceInvalid();
            ethPrice18 = uint256(ethPrice) * 1e10; // from 8 decimals to 18
        }
        // NOTE: Currently the market asset and collateral asset are the same.
        // If this changes in the future, then the following line should fetch the collateral asset, not market asset.
        (uint256 collateralPrice, uint256 timestamp) = _oracleModule.getPrice();

        if (collateralPrice <= 0) revert FlatcoinErrors.PriceInvalid(FlatcoinErrors.PriceSource.OnChain);

        if (block.timestamp >= timestamp + _STALENESS_PERIOD)
            revert FlatcoinErrors.PriceStale(FlatcoinErrors.PriceSource.OnChain);

        uint256 gasPriceL2 = _gasPriceOracle.gasPrice();
        uint256 overhead = _gasPriceOracle.overhead();
        uint256 l1BaseFee = _gasPriceOracle.l1BaseFee();
        uint256 decimals = _gasPriceOracle.decimals();
        uint256 scalar = _gasPriceOracle.scalar();

        uint256 costOfExecutionGrossEth = ((((_gasUnitsL1 + overhead) * l1BaseFee * scalar) / 10 ** decimals) +
            (_gasUnitsL2 * gasPriceL2));
        uint256 costOfExecutionGrossUSD = costOfExecutionGrossEth.mulDiv(ethPrice18, _UNIT); // fee priced in USD

        uint256 maxProfitMargin = _profitMarginUSD.max(costOfExecutionGrossUSD.mulDiv(_profitMarginPercent, _UNIT)); // additional USD profit for the keeper
        uint256 costOfExecutionNet = costOfExecutionGrossUSD + maxProfitMargin; // fee priced in USD

Secondly, it obtains the price of rETH and calculates the keeper fee by dividing the converted USD fee by the price of rETH.


        (uint256 collateralPrice, uint256 timestamp) = _oracleModule.getPrice();
      keeperFeeCollateral = (_keeperFeeUpperBound.min(costOfExecutionNet.max(_keeperFeeLowerBound))).mulDiv(
            _UNIT,
            collateralPrice
        ); // fee priced in collateral

The validation of rETH ensures it remains within the threshold range. If the price of ETH from Chainlink is manipulated to be significantly high, the keeper fee will also increase substantially. This portion of the keeper fee is deducted from the user's settled margin, potentially resulting in users receiving very low settlement fees and consequently experiencing losses in their funds.


        // Settle the collateral.
        vault.sendCollateral({to: _keeper, amount: _order.keeperFee}); // pay the keeper their fee
        vault.sendCollateral({to: _account, amount: uint256(settledMargin) - totalFee}); // transfer remaining amount to the trader

Impact

User fund loss

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L313-L314

Tool used

Manual Review

Recommendation

Implement slippage protection.

sherlock-admin commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid: manipulated as in?

nevillehuang commented 8 months ago

Invalid, you cannot manipulate chainlink price feeds directly due to the way they are designed. If other price feeds are manipulated, that is out of scope of this contests, and even then chainlink will still report an accurate price based on current market conditions, given this price feeds are trusted.

If the market-wide price of an asset is manipulated, Price Feeds will report that price because it accurately reflects the current state of the market—the accurate truth. But if only a small subset of an asset’s underlying market is manipulated (e.g. a few low-liquidity markets), then Chainlink Price Feeds are designed to still report the accurate overall market-wide price, helping protect from such manipulation attempts.

sherlock-audit / 2023-12-flatmoney-judging

Bauer - Slippage protection is missing when executing limit orders #63