stellar-protocol icon indicating copy to clipboard operation
stellar-protocol copied to clipboard

Race condition in using ManageOffer operation to modify an offer

Open ScottHogge opened this issue 6 years ago • 2 comments

When using the ManageOffer operation to update an offer, the amount field replaces the currently available (unfilled) amount on the offer, rather than replacing the total liability specified when the offer was made. This leads to the following behavior: ​ Person A: Submit transaction 1: Sell 100 Marbles @ X1 Network processes transaction 1. Person B: Submit transaction 2: Buy 90 Marbles @ X1 Person A: Submit transaction 3: Change offer to Sell 100 Marbles @ X2 Person C: Submit transaction 4: Buy 100 Marbles @ X2 Network processes transactions 2,3,4 ​ At the end of this, Person A has sold 190 Marbles because of the race condition, whereas he intended to only sell 100 but update his price.

Pretty much all financial markets allow for atomic changes to orders, but its always very clear in the specs that it updates the total liability, not the outstanding liability, specifically because of the race condition with in-flight messages. For example, Nasdaq's proprietary specification states the following for the quantity field: "Total number of shares liable, inclusive of previous executions and Self Match Prevention decremented shares on this order chain"

ScottHogge avatar Sep 27 '18 01:09 ScottHogge

As far as I understand, Person A will sell either 190 or 100 marbles in your example depending on the transactions application order, which is pseudo-random. The offer update is atomic, so the transaction 2 will either update the order if it was not fully executed yet or return an error if tx 4 is applied before the tx 2 (offer "Sell 100 Marbles @ X1" has been already executed and the expected offer_id does not exist anymore).

It makes things even more interesting.

orbitlens avatar Sep 27 '18 21:09 orbitlens

@jonjove @MonsieurNicolas — any thoughts on this? Seems gnarly, and I'm not sure if any past changes about liabilities in the protocol have changed this scenario.

theaeolianmachine avatar Mar 14 '19 23:03 theaeolianmachine