DittoETH

Summary

addOrder makes multiple function calls to find the position of the order that must be added, however when it reaches verifySellId it can accidentally add the order in the wrong place.

Vulnerability Details

When addOrder is called it tries to find the prev and next ID and insert the new order in the middle. However the call to findPrevOfIncomingId can actually return the next order and not the previous which result in the orders being 2 places further than they should be (since it is prev => current => next).

When findPrevOfIncomingId is called it will try and find the direction, with the help of verifyId

However this call to verifyId can actually return NEXT as the direction while it should return EXACT. This happens because verifyId calls verifySellId to check the direction, and verifySellId has a wrong check, more perilously an inconsistency in the checks.

As we can see from the above code snippet check1 checks for if orders[asset][_prevId].price is <= _newPrice while check2 checks for if _newPrice is < orders[asset][_nextId].price.

This discrepancy in the operators can cause an issue if the orders are 1 => 2 => 3 where the prices of 2 and 3 are equal. As check1 will be true, while check2 false, which will trigger else if (!check2) { return Constants.NEXT; } and return as NEXT while it should have returned EXACT => if (check1 && check2) { return Constants.EXACT; } .

With the return of NEXT from verifyId findPrevOfIncomingId will trigger this else if returning the next ID, which will be saved as prevId in addOrder, from there on it will get the next next ID and save it as nextId, and the mess will continue.

This all happens if the orders have 2 orders with the exact prices, which may be often occurring.

Impact

Complete mess in the orders, may lead to further complications

Tools Used

Manual review

Recommendations

Remove the discrepancy and change check 2. I would also suggest to do the same in verifyBidId since the checks there are the same and the issue could also occur there. However since it is the same I don't feel it is necessary to do a separate explanation for it.