Orders may be matched based on stale prices, causing financial losses.

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BidOrdersFacet.sol#L130-L204 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L556-L626 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BidOrdersFacet.sol#L150 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L574

Vulnerability details

Description

bidMatchAlgo and sellMatchAlgo, which are responsible for matching incoming orders with existing orders in the orderbook based on price.

The bidMatchAlgo and sellMatchAlgo functions are expected to match incoming orders with existing orders in the orderbook based on accurate and up-to-date price information. The hint system should optimize the order placement process without compromising the integrity of the matching logic.

The expected inputs are the asset being traded, the incomingBid or incomingAsk order, and the orderHintArray for efficient order placement.

The intended outcome is to fill the incoming order as much as possible by matching it with existing orders at prices that reflect the current market conditions, ensuring fair and accurate trading for all users.

BidOrdersFacet.sol#bidMatchAlgo

function bidMatchAlgo(
    address asset,
    STypes.Order memory incomingBid,
    MTypes.OrderHint[] memory orderHintArray,
    MTypes.BidMatchAlgo memory b
) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
    // ... code ...
    STypes.Order memory lowestSell = _getLowestSell(asset, b);
    if (incomingBid.price >= lowestSell.price) {
        // ... code ...
        LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
        b.shortHintId = b.shortId = Asset.startingShortId;
        b.oraclePrice = LibOracle.getPrice(asset);
        return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
    }
    // ... code ...
}

LibOrders.sol#sellMatchAlgo

function sellMatchAlgo(
    address asset,
    STypes.Order memory incomingAsk,
    MTypes.OrderHint[] memory orderHintArray,
    uint256 minAskEth
) internal {
    // ... code ...
    if (incomingAsk.price < oraclePrice) {
        LibOrders.addSellOrder(incomingAsk, asset, orderHintArray);
    } else {
        LibOrders.sellMatchAlgo(asset, incomingAsk, orderHintArray, minAskEth);
    }
}

These functions use the cached oracle price and the provided order hints to match incoming orders with existing orders in the orderbook. The bidMatchAlgo function updates the oracle price and startingShortId if the price difference between the incoming bid and the cached oracle price exceeds a threshold. The sellMatchAlgo function adds the incoming ask order to the orderbook if its price is below the cached oracle price.

Vulnerability Details

The edge case arises from the combination of the 0.5% buffer, backwards matching logic, and the use of stale prices due to the 15-minute caching window.

The conditions that enable this

• The system allows for a 0.5% buffer when matching orders, considering prices within this range as valid matches.
• The backwards matching logic is employed to find a valid match in case of a price mismatch.
• The 15-minute caching window can lead to the use of stale prices if market conditions change significantly within that time frame.

The codes responsible are:

• In bidMatchAlgo, the comparison if (incomingBid.price >= lowestSell.price) allows matching based on the cached oracle price and the 0.5% buffer.
• In sellMatchAlgo, the condition if (incomingAsk.price <= highestBid.price) relies on the cached oracle price to determine whether to add the incoming ask order to the orderbook.

The actual behavior of the system deviates from the expected behavior in the following ways:

• Exploitation of the 0.5% buffer and backwards matching logic:
  • An attacker can place orders with prices just within the 0.5% buffer of the cached oracle price.
  • The attacker's orders can get matched with existing orders based on the cached price, even if the actual market price has diverged.
  • The attacker can profit from the price discrepancy between the cached price and the real-time market price.
• Use of stale prices:
  • The 15-minute caching window can lead to the use of stale prices if market conditions change significantly within that time frame.
  • Orders can be matched based on stale prices, resulting in trades that do not align with the current market conditions.
  • Users may end up buying or selling assets at prices that are no longer favorable or accurate.

These behaviors deviate from the intended fair and accurate matching of orders based on up-to-date price information.

Impact

• Users may suffer financial losses if their orders are matched at prices that are not reflective of the current market conditions.
  • Buying or selling assets based on stale prices can lead to unfavorable trades and potential losses.

Proof of Concept

Scenario:

1. The current market price of the asset is $100.
2. The cached oracle price for the asset is $100, which was updated 14 minutes ago.
3. The orderbook has the following existing orders:
   • Sell orders:
     • 1000 tokens at $100.50
     • 500 tokens at $101
   • Buy orders:
     • 800 tokens at $99.50
     • 1200 tokens at $99
4. The market price suddenly drops to $95 due to external factors.

Steps:

1. An attacker observes the market conditions and identifies an opportunity to exploit the vulnerability.

2. The attacker places a buy order for 1500 tokens at $99.75, which is within the 0.5% buffer of the cached oracle price ($100).

   function placeBuyOrder(address asset, uint256 amount, uint256 price) external {
      STypes.Order memory order = STypes.Order({
          addr: msg.sender,
          price: price,
          ercAmount: amount,
          id: nextOrderId,
          orderType: O.LimitBid,
          creationTime: block.timestamp
      });
   
      MTypes.OrderHint[] memory orderHintArray;
      uint16[] memory shortHintArray;
   
      (uint88 ethFilled, uint88 ercAmountLeft) = bidMatchAlgo(asset, order, orderHintArray, shortHintArray);
   
      // Update user's balance and order state
      // ...
   }

3. The bidMatchAlgo function is called with the attacker's buy order.

   • The cached oracle price is still $100, as it hasn't been updated within the 15-minute window.
   • The _getLowestSell function returns the sell order with the price of $100.50 as the lowest sell order.
   • The condition if (incomingBid.price >= lowestSell.price) is satisfied since the attacker's buy price of $99.75 is within the 0.5% buffer of the sell order price.
   • The updateOracleAndStartingShortViaThreshold function is called, but since the price difference is within the threshold, no update occurs.

4. The bidMatchAlgo function proceeds to match the attacker's buy order with the existing sell orders.

   • The sell order of 1000 tokens at $100.50 is matched with the attacker's buy order at the price of $100.50, even though the current market price has dropped to $95.
   • The remaining 500 tokens of the attacker's buy order are matched with the sell order of 500 tokens at $101, also at the stale price.

5. The attacker successfully buys 1500 tokens at an average price of $100.67, which is significantly higher than the current market price of $95.

Potential Consequences:

• Users who placed sell orders at prices above the current market price may experience opportunity costs if their orders are filled at the stale cached price.
• The protocol may accumulate losses if the market price continues to move against the filled orders.
• The integrity of the order matching process is compromised, as orders are being matched based on stale prices that do not reflect the current market conditions.
• Users may lose trust in the protocol if they consistently experience losses due to the use of stale prices and the exploitation of the 0.5% buffer.

Recommended Mititgation Steps

1. Reduce the caching window:

   • Shorten the caching window to a smaller time frame (e.g., 5 minutes) to reduce the chances of using stale prices.
   • Regularly update the cached price to reflect the latest market conditions.

   function updateCachedPrice(address asset) internal {
      uint256 currentTime = block.timestamp;
      if (currentTime - lastPriceUpdateTime[asset] >= 5 minutes) {
          cachedPrice[asset] = fetchLatestPrice(asset);
          lastPriceUpdateTime[asset] = currentTime;
      }
   }

2. Implement price deviation checks:

   • Compare the cached oracle price with the real-time market price obtained from reliable external sources.
   • If the price deviation exceeds a certain threshold (e.g., 1%), trigger a price update or halt the order matching process until the price is refreshed.

   function isValidPrice(address asset, uint256 price) internal view returns (bool) {
      uint256 cachedPrice = getCachedPrice(asset);
      uint256 realTimePrice = fetchRealTimePrice(asset);
      uint256 deviation = calculateDeviation(cachedPrice, realTimePrice);
      return deviation <= MAX_DEVIATION_THRESHOLD;
   }

3. Enhance the hint system:

   • Improve the hint system to take into account the potential price changes within the caching window.
   • Incorporate additional checks and validations to ensure the accuracy and reliability of the hints provided by users.

   function validateHint(address asset, uint256 hintPrice) internal view returns (bool) {
      uint256 cachedPrice = getCachedPrice(asset);
      uint256 realTimePrice = fetchRealTimePrice(asset);
      uint256 deviation = calculateDeviation(hintPrice, realTimePrice);
      return deviation <= MAX_HINT_DEVIATION_THRESHOLD;
   }

4. Implement circuit breakers:

   • Set up circuit breakers that halt trading or limit order matching when significant price deviations or unusual market activities are detected.
   • Define thresholds for price movements or trading volumes that trigger the circuit breakers.

   function checkCircuitBreaker(address asset) internal view {
      uint256 priceDeviation = calculatePriceDeviation(asset);
      uint256 tradingVolume = calculateTradingVolume(asset);
      if (priceDeviation >= CIRCUIT_BREAKER_PRICE_THRESHOLD ||
          tradingVolume >= CIRCUIT_BREAKER_VOLUME_THRESHOLD) {
          // Halt trading or limit order matching
          // ...
      }
   }

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #114

[raymondfam]

Kind of unstructured in terms of POC when compared to that of #114 and #128.

[c4-judge]

hansfriese marked the issue as satisfactory