Test function left behind can expose order book

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

The getBid() order book function is noted in its Natspec @dev comments as “@dev just to pass old tests, not needed otherwise @dev to be deleted once tests updated” but is left behind here.

This function could externally expose orderbook ordering (prev/next linked list) for malicious contracts to potentially time or price bids to its advantage.

Proof of Concept

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCOrderbook.sol#L807-L827

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove function as noted.

[Splidge]

Remove function as noted.

It is to be removed one the tests are updated. However they are not yet, so it isn't to be removed yet.

It is simple enough for other tools to determine the order of the orderbook without this, the frontend manages this and displays the information to the users. This is not a vulnerability, but useful information for users to have at their disposal.

[dmvt]

The issue as I see it is that this exposes orderbook order to other contracts, not just other tools. This opens up some potentially unwanted vectors if left in. IMO, you should have removed this prior to the contest or specifically commented that it should be ignored during the contest. Another potential approach would be to have a testing contract which adds this functionality for testing but does not get automatically deployed to production. If you forget about it and leave it in, it does represent a small risk.