Open code423n4 opened 2 years ago
Should this be tagged as Med or Low? Funds are not directly at risk unless the centralised order book server goes down and loses all the data. Perhaps there is a non-negligible chance that this could happen. But even then, orders have an "expiration" field attached to them which will render them useless after some set time period. There are also easy fixes on the frontend, such as allowing users to download a txt file with their order/orderHash so that they don't have to rely on the centralised DB for data availability.
But will defer to judges.
Report: Cannot cancel orders without reliance on centralised database
The sponsor's point is valid: there is an expiration param that the maker signs as part of the order that marks its validity.
However, the warden(s) concerns are valid too. While it is an edge case that is very unlikely to happen, there would arguably be a "loss" of assets of the maker because of the protocol's loss of functionality, as per the scenario described above. Hence, the medium severity rating is justified.
I recommend implementing the warden's recommended fix; having a minimumValidNonce
would be great in allowing easy on-chain cancellation of an order. It makes the system a little more trust-less and provides a "red button" option for makers to use if necessary.
PR with fix: https://github.com/outdoteth/putty-v2/pull/10
Lines of code
https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L526-L535
Vulnerability details
Impact
Order cancellation requires makers to call
cancel()
, inputting the order as a function parameter. This is the only cancellation method, and it can cause two issues.This first issue is that it is an on-chain signal for MEV users to frontrun the cancellation and fill the order.
The second issue is the dependency to a centralized service for cancelling the order. As orders are signed off chain, they would be stored in a centralized database. It is unlikely that an end user would locally record all the orders they make. This means that when cancelling an order, maker needs to request the order parameters from the centralized service. If the centralized service goes offline, it could allow malicious parties who have a copy of the order database to fill orders that would have been cancelled otherwise.
Proof of Concept
Tools Used
Pen & paper.
Recommended Mitigation Steps
Aside from the standard order cancellation method, have an extra method to cancel all orders of a caller. This can be achieved using a "minimum valid nonce" state variable, as a mapping from user address to nonce.
Allow users to increment their
minimumValidNonce
. Make sure the incrementation function do not allow incrementing more than2**64
such that callers cannot lock themselves out of creating orders by increasingminimumValidNonce
to2**256-1
by mistake. Then, prevent filling orders iforder.nonce < minimumValidNonce
.Another method to achieve bulk cancelling is using counters. For example, Seaport uses counters, which is an extra order parameter that has to match the corresponding counter state variable. It allows maker to cancel all his orders by incrementing the counter state variable by one.
Either of these extra cancellation methods would enable cancelling orders without signalling to MEV bots, and without a dependency to a centralized database.