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

bug: Driver creates non-settleable cow amm orders #3102

Closed
sunce86 opened this issue Nov 1, 2024 · 3 comments
Closed

bug: Driver creates non-settleable cow amm orders #3102

sunce86 opened this issue Nov 1, 2024 · 3 comments
Labels
bug Something isn't working oncall Issue/PR for consideration during oncall rotation

Comments

@sunce86
Copy link
Contributor

sunce86 commented Nov 1, 2024

Problem

On Sepolia, driver created a cow amm order with some sell_amount and buy_amount.

Baseline and Quasimodo solvers solved this order successfully.

But, both solutions reverted when simulated.

The reason is a check in the BCowPool:

uint256 buyTokenBalance = order.buyToken.balanceOf(address(this));
if (order.buyAmount > bmul(buyTokenBalance, MAX_IN_RATIO)) {
   revert BPool_TokenAmountInAboveMaxRatio();
}

This code checks if the cow amm order's buy_amount is too large, to prevent big price impacts when rebalanced. If the buy_amount is too large, and since cow amm orders are SELL orders that need to be executed with AT LEAST buy_amount of buy token, then buy tokens inserted into the BCowPool would move the price too much, and that should be prevented with this check, which is fine.

However, this check is not good for partially fillable orders, which have the same order's sell_amount and buy_amount, but are executed partially (with let's say 1% of full amount), so they wouldn't move the price as the full order's buy_amount would (but this can't be fixed as BCowPool implements isValidSignature and that is the only place where pool constraints are checked and only the signed order data is available there).

Anyway, the BCowPool is deployed, so the only workaround is to adjust the CowAmmLegacyHelper contract, which defines the created order's sell_amount and buy amount, to consider the BCowPool's check and lower the buy_amount, otherwise the buy_amount is so high that it would require solutions that move the price too much and all solutions would revert always.

Impact

Solver properly solves cow amm order but the order always fails the simulation because the order buy_amount never passes the check.

Expected behaviour

Order's sell and buy amounts should be defined in such a way to always pass these basic checks, and then the solvers would propose another solutions, where some of those solutions would not cause too much price impact and would have a chance to pass simulation.

@sunce86 sunce86 added bug Something isn't working oncall Issue/PR for consideration during oncall rotation labels Nov 1, 2024
@MartinquaXD
Copy link
Contributor

AFAICS the partiallyFillable flag doesn't play any part in this. During the isValidSignature() check the pool doesn't even have the ability to know how big of a partial fill it's dealing with so it has to use the order's underlying total sell and buy amounts. Could this just be a pool which is so shallow that any rebalancing order would trigger this MAX_RATIO_IN check?
I guess one thing we could try is to verify the signature right after generating it in the driver to at least avoid sending solvers rebalancing orders that will revert. Unfortunately that would still make us rely on external solvers with better cow amm support than the "official" helper contract.

@sunce86
Copy link
Contributor Author

sunce86 commented Nov 5, 2024

What happens in our example from above: Pool has 0.5WETH and 5 USDC reserve, we properly create cow amm template order with sell_amount = 0.25 WETH and buy_amount = 4.5 USDC. This is the core issue.

If we created template order with sell_amount = 0.25 WETH and buy_amount = 2.5 USDC (so that is passes the MAX_RATIO_IN check), then all solutions from solvers would pass the check and there would be no simulation failures.

@fleupold
Copy link
Contributor

fleupold commented Nov 5, 2024

Yes, I think the core issue is in the helper contract (it should only return orders that also pass the verify function later). Let's close this once #3110 is working

MartinquaXD added a commit that referenced this issue Dec 2, 2024
# Description
Signature verification is now part of order creation for cow amm orders.

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] isValidSignature call is executed as part of creating the cow amm
order

## How to test
Cow amm e2e test should confirm no new issues are introduced.

Fixes #
#3102

---------

Co-authored-by: MartinquaXD <[email protected]>
@sunce86 sunce86 closed this as completed Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working oncall Issue/PR for consideration during oncall rotation
Projects
None yet
Development

No branches or pull requests

3 participants