Unhandled edge case in bid matching algorithm

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BidOrdersFacet.sol#L169-L179

Vulnerability details

Impact

The identified issue in the bid matching algorithm could have several significant impacts:

• Earning "free money": Malicious actors could exploit this vulnerability by creating multiple sell orders and then creating multiple buy orders that exactly match the created sell orders. Because the protocol does not update the lowestSell.ercAmount upon an exact match, it allows these sell orders to remain unchanged, essentially allowing the actor to earn "free money" without actually selling anything and thereby draining the protocol.

• Inaccurate Order Book: If lowestSell.ercAmount remains unchanged after an exact match, it may lead to inaccuracies in the order book. Sell orders that have been fully filled may still appear in the order book with non-zero amounts, misleading traders about the available liquidity.

Proof of Concept

The issue was identified through a manual review of the bid matching algorithm code in the provided GitHub repository. Specifically, the relevant code snippet where the concern was raised is as follows:

 function bidMatchAlgo(
        address asset,
        STypes.Order memory incomingBid,
        MTypes.OrderHint[] memory orderHintArray,
        MTypes.BidMatchAlgo memory b
    ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
            // more code

         if (incomingBid.ercAmount == lowestSell.ercAmount) {
            // @audit-issue Oversight, if we don't update lowestSell.ercAmount = 0 here, it means that it will remain unchanged, leading to issues

             if (lowestSell.isShort()) {
                  b.matchedShortId = lowestSell.id;
                  b.prevShortId = lowestSell.prevId;
                  LibOrders.matchOrder(s.shorts, asset, lowestSell.id);
               } else {
                   b.matchedAskId = lowestSell.id;
                    LibOrders.matchOrder(s.asks, asset, lowestSell.id);
                      }
               }

As the audit-issue comment suggests, lowestSell.ercAmount should be set to 0 after a bid matches exactly with the sell amount. However, there is no such update in the code, leaving lowestSell.ercAmount unchanged on the order book. This could lead to the same sell order being executed for all bids that match it, allowing malicious actors to exploit the system for financial gain.

Tools Used

VSCode

Recommended Mitigation Steps

To address this issue, it is recommended to update the lowestSell.ercAmount to 0 after an exact match occurs. This ensures that the order book accurately reflects the filled orders and prevents the risk of double execution.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[raymondfam]

Inadequate elaboration and proof given.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof