Splidge
commented
3 years ago TL:DR: _requiredPrice is recalculated in each iteration of _searchOrderbook here
In your example starting with _searchOrderbook we first calculate the required price:
uint256 _requiredPrice = (_nextUser.price * (_minIncrease + (100))) / (100);
Which comes to 7.7 (with the default 10% increase)
Now we need to check if we can do an iteration of the while loop, this first check is against the previous and next positions:
(_price != _prevUser.price || _price <= _nextUser.price)
= ( 4 != 9 || 4 <= 7)
= ( true || true )
The second check is against the required price:
_price < _requiredPrice
= 4 < 7.7
= true
The third check is the number of iterations, I'll skip writing this out for brevity.
We have now entered the While loop, _prevUsernow becomes the _nextUser, so _prevUser.price becomes 7
_nextUsermoves along one more in the orderbook, so _nextUser.price becomes 5
AND we recalculate the _requiredPrice which is now 5.5
Now we re-evaluate if the while loop should iterate again, first check:
(_price != _prevUser.price || _price <= _nextUser.price)
= ( 4 != 7 || 4 <= 5)
= ( true || true )
Second check:
_price < _requiredPrice
= 4 < 5.5
= true
We do the while loop again, _prevUser.price becomes 5
_nextUser.price becomes 3
_requiredPrice becomes 3.3
Once again re-evaluate if the while loop should iterate again, first check:
(_price != _prevUser.price || _price <= _nextUser.price)
= ( 4 != 5 || 4 <= 3)
= ( true || false )
We don't make it to the second check because of short-circuiting rules after we discover the False. Your position has been found between the users at 5 and 3.
Besides, this is known working as not only are there tests specifically for this but in the majority of our tests we don't pass a starting location at all (we give address(0)), also in the current deployed beta we haven't always given a starting location either.
0xean
Handle
0xImpostor
Vulnerability details
Impact
I understand that it is
however should there be a custom UI made for these contracts and it is not sorted correctly, some of the logic in the code will break.
Proof of Concept
Instead of relying on the official UI, I'm using a community made UI but this UI doesn't maintain the sorting of the orderbook.
When I
addBidToOrderbookis called, the_prevUserAddressthat is passed into the function may not necessarily be the lowest bid that is higher than the bid to be added.For example, the correct sorted order (ignoring the Minimum bid increase percentage) should be
market, 10, 9, 7, 5, 3, 1for a particular card. I want to make a bid of price = 4 so the correct_prevUserAddressshould be 5 however if the address corresponding to the user who bid 9 is passed into the function so when_requiredPricewithin_searchOrderbookis calculated, it is using the value of 7 to calculate the required price instead of 3. This means that no matter how many iterations are made, you will never be able to find a position in the orderbook.Tools Used
Manual analysis
Recommended Mitigation Steps
There is no quick fix for this and requires quite a substantial revamp of the design. A naive band solution would be to iterate through the linked lists but this is a bad solution.