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

Zero-amount fill-or-kill orders can be executed more than once #29

Open
fedgiac opened this issue Oct 4, 2022 · 2 comments
Open

Zero-amount fill-or-kill orders can be executed more than once #29

fedgiac opened this issue Oct 4, 2022 · 2 comments

Comments

@fedgiac
Copy link
Contributor

fedgiac commented Oct 4, 2022

We currently rely on filledAmount[orderUid] to determine if a fill-or-kill order was executed or not. However, if sellAmount is zero (and buyAmount is zero, as noted below) and the order is executed then filledAmount does not change, which means that the order can be executed again. This is problematic as the fee would be taken multiple times from the user.

This is not critical for normal user orders as we don't generate zero-sell-amount orders in the interface. It's however unlikely but possible that a user creates an order selling nothing for a fee and would be affected by this issue. Most importantly, it could cause bugs in contracts that rely on filledAmount as well, see for example the ETH-flow contract case.

In the current code, this is not an issue for partially fillable orders.

@nlordell
Copy link
Contributor

nlordell commented Oct 5, 2022

Looking at the code, this only affects orders where sellAmount == 0 && buyAmount == 0, and AFAICT affects both SELL and BUY orders. Specifically, we have:

  1. The following assertion:
    require(
        order.sellAmount.mul(sellPrice) >= order.buyAmount.mul(buyPrice),
        "GPv2: limit price not respected"
    );
  2. This means, if sellAmount == 0, then 1. will only not revert if buyAmount * buyPrice == 0, which means buyAmount == 0 or buyPrice == 0.
  3. We also have the following division:
    executedBuyAmount = executedSellAmount.mul(sellPrice).ceilDiv(buyPrice);
  4. The division in 4. will revert if buyPrice == 0 hence buyAmount == 0 in order for 1. to not revert.

@fleupold
Copy link
Contributor

fleupold commented Oct 5, 2022

Let's document this in the README of this contract (maybe we can add a "known issues" section?) and potentially add a note on https://docs.cow.fi/tutorials/how-to-submit-orders-via-the-api/4.-signing-the-order

@fedgiac fedgiac changed the title Zero-sell-amount fill-or-kill orders can be executed more than once Zero-amount fill-or-kill orders can be executed more than once Oct 5, 2022
@cowprotocol cowprotocol deleted a comment Jan 16, 2023
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

No branches or pull requests

3 participants