raymogg
commented
3 years ago Nonces aren't actually needed with our trading interface due to the fact that the trading interface keeps track of the amount filled in each order, and does not execute the order if it is already completely filled.
For example, in the presented attack, when Eve replays the transaction, if the order is not yet fully filled, the order will only be filled up to the signed amount (say 50 units). After that this trade will not process. Since the user originally signed this order for 50 units, it makes sense to process the order up to that many units. The hash of the order is used as its unique identifier too, so collisions should not occur.
The trader contract is also going to be whitelisted which further mitigates this attack.
Good pick up on something that should definitely be added to our docs though.
cemozerr
Handle
0xRajeev
Vulnerability details
Impact
Nonces are typically used in signatures/verifications of orders to prevent MITM replay attacks for scenarios where orders have to execute only once. It is not clear if this is a concern in this protocol. However, given the lack of nonces, it may be possible for a relayer/observer to replay the orders in executeTrade() and cause unintentional trades to execute again or griefing in the worse case.
Impact: Alice performs executeTrade(). Malicious Eve observes and copies this transaction from mempool and replays it. This could also happen accidentally if a trading interface submits a batch twice by mistake. Trades execute twice and change the protocol state in unexpected ways which could put makers/takers under margin and at liquidation risk.
Proof of Concept
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/Trader.sol#L67-L137
Tools Used
Manual Analysis
Recommended Mitigation Steps
Evaluate the need for nonces or other measures for preventing trades to be replayed. If this is not a concern, document the rationale.