Summary

Based on the documentation, short orders should not be filled at prices less than the prevailing oracle price. However, the actual implementation does not guarantee this to be the case, breaking the intended logic. This is because, when a bid order is created, the cached oracle price (b.oraclePrice) which is compared against potential short order prices (to determine whether those shorts are fillable), can be outdated.

Vulnerability Details

When creating a bid using createBid, the cached price is gathered using the following call:

This simply returns the cached price, which can be vastly different than the prevailing oracle price. Later in this function call the following code is run in order to update the oracle price and the associated startingShortId (if necessary):

Notice that even if LibOrders.updateOracleAndStartingShortViaThreshold(..) updates the oracle price and updates the starting Asset.startingShortId, there is no change to b.oraclePrice which is now effectively out of date.

Let's consider the following scenario where there are no limitAsk orders and two limitShort orders: shortA and shortB, where shortA.price = 10 and shortB.price = 15. The original oracle price = b.oraclePrice = 9, and after calling LibOrders.updateOracleAndStartingShortViaThreshold(..), the oracle price is now 14. Also lets assume that a user has created a bid in which the bid.price = 15. This means that the current state of shorts is: HEAD -> shortA -> shortB -> TAIL.

Let's walk through the call to createBid:

1. BidOrdersFacet#L129-L132:
   b.oraclePrice is set to 9. b.shortHintId = b.shortId = shortA.

2. BidOrdersFacet#L136-L142:
   lowestSell will reference shortA as there are no ask orders. The if-statement will pass as incomingBid.price=15 >= lowestSell.price=10.

3. BidOrdersFacet#L144-L148:
   LibOrders.updateOracleAndStartingShortViaThreshold(..) will update the oracle price to 14 (the prevailing oracle price) and save it. The startingShortId will be updated to equal shortB, as shortB.price >= oraclePrice. Note that shortA would not be a valid fill now, as it's price is less than the prevailing oracle price after this update. This then calls bidMatchAlgo.

4. BidOrdersFacet#L185-L200:
   In the bidMatchAlgo function, lowestSell will reference shortB, and upon calling matchlowestSell, we assume that the bid was not fully filled yet. In this situation, b.prevShortId = lowestSell.prevId; is first called, which since lowestSell = shortB, b.prevShortId=shortA. Then, _shortDirectionHandler will be called.

5. BidOrdersFacet#L440-L443:
   In _shortDirectionHandler, since b.oraclePrice is still the old cached value of 9, and shortA.price=10, s.shorts[asset][b.prevShortId].price >= b.oraclePrice will return true and b.shortId = b.prevShortId;. In english, this means that the next short id will be shortA, because it's price was greater than the cached b.oraclePrice.

This will then mean that the bid order will be matched/filled against shortA. However, this behavior is invalid because the current prevailing oracle price is 14 and not 9. Because shortA.price = 10 < 14, it means that this short should not be fillable.

Impact

It is possible that short orders with prices less than the prevailing oracle price can be filled, which breaks this requirement that is outlined in the documentation.

Tools Used

Manual review

Recommendations

When there is a change in the oracle price (in the call to LibOrders.updateOracleAndStartingShortViaThreshold(..)), update b.oraclePrice to be equal to the new oracle price.