Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not allowing zero sell amount orders #27

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Not allowing zero sell amount orders #27

merged 2 commits into from
Sep 27, 2022

Conversation

josojo
Copy link
Contributor

@josojo josojo commented Sep 26, 2022

Adam is such a good auditor!
https://cowservices.slack.com/archives/CM1HPMCE5/p1663959018673759?thread_ts=1662553939.402079&cid=CM1HPMCE5

Unless I have missed something, I think it's possible to submit an order that has: sellAmount == 0, buyAmount == 0, feeAmount > 0 and partiallyFillable == false . This order will be executable unlimited number of times and each time it will drain feeAmount from the contract. (edited)

This PR prevents zero sell amount orders. Even if the settlement contract would prevent 0 amounts for sell orders, we should have excluded it in the ethflow contract to be secure.

@josojo josojo requested a review from nlordell September 26, 2022 07:31
test/CoWSwapEthFlow.t.sol Outdated Show resolved Hide resolved
@adamkolar
Copy link

The fix looks good. I'm pretty sure the settlement contract does allow 0 amount sell orders when partiallyFillable == false btw, that's something that should be either fixed or at least documented, because it could cause trouble in other systems that integrate with cowswap.

@josojo josojo merged commit 9a3946f into main Sep 27, 2022
@josojo josojo deleted the auditFix branch September 27, 2022 06:34
@fedgiac
Copy link
Contributor

fedgiac commented Oct 4, 2022

Great catch for a very bad bug.
The (bad) implicit assumption here was that a fill-and-kill CoW Swap order can only be executed once. But this is indeed not true for fill-or-kill orders with zero sell amount: the order can be executed multiple times as filledAmount stays zero and doesn't tell the settlement contract that the order was executed. That's not an issue for partially fillable orders because of a division by zero in the case of a zero sell amount; also, if the executed amount is zero then the fee will also be zero.
I definitely think we should change the settlement contract to make this impossible (however, I don't think it's critical and we can wait until we release a new version of the contract).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants