Driver causes "GPv2: limit price not respected" on limit orders by rounding the price wrong

[tokenwood]

Description
Driver adds custom prices for limit orders in crates/solver/settlement_encoder.rs
it sets the sell token price as "buy amount" and the buy token price as "sell amount" in the settlement transaction.
However buy_amount is rounded down instead of up which causes the price limit to not be respected and the transaction simulation to fail.

the root cause is in line 351 of crates/solver/settlement_encoder.rs

let buy_amount = sell_amount
                    .checked_mul(uniform_sell_price)
                    .context("buy_amount computation failed")?
                    .checked_div(uniform_buy_price)
                    .context("buy_amount computation failed")?;

** Solution**
Fix would be to implement something like this:

let buy_amount = sell_amount
    .checked_mul(uniform_sell_price)
    .context("buy_amount computation failed")?
    .ceil_div(&uniform_buy_price)
    .context("buy_amount computation failed")?;

** Logs **
Driver logs:
2024-03-01T09:53:20.351Z INFO request{id="1"}:/solve{solver=MySolver auction_id=8531103}: driver::infra::observe: discarded solution: settlement encoding id=Id(0) err=Simulation(Revert(RevertError { err: Blockchain(Web3(Rpc(Error { code: ServerError(-32015), message: "Reverted 0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001f475076323a206c696d6974207072696365206e6f742072657370656374656400",
My solver passed a solution with the following clearing prices for order "0xfdade0ee290adcab31f2323190d3bc7e9b12e0f5c3d6b599334fc8dfb6375aa8844dc85edd8492a56228d293cfebb823ef3e10ec66050708":

uniform_sell_price= 1000000000000000000
uniform_buy_price= 1002968787607555350111258518195

and the driver set the buy_amount to buy_amount= 34558023172 , but
sell_amount * uniform_sell_price / uniform_buy_price = 34558023172.5,
the buy_amount should be 34558023173

[fleupold]

The issue with rounding up would be that the user would potentially receive more tokens than the settlement contract would be able to generate from executing the solution leading to a potential loss for the protocol or solver.

Solvers should compute prices in a way that takes this rounding behavior into account.

[tokenwood]

OK got it thanks

[marcovc]

I'm having the exact same problem, but I disagree with the "solution". The smartcontract rounds up:

https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2Settlement.sol#L389-L391

So essentially we have a [Solver] <-> [Driver] <-> [Smartcontract] chain, where [Solver] is satisfying [Smartcontract] rules, but then there is a transformation in [Driver] that makes the solution invalid. My opinion is that any transformation between the solver and the smartcontract must preserve the fact if a solution is valid or not.

There is a second problem with this transformation. The fact that the smartcontract is rounding up actually makes the task of rounding solutions to integers possible on the solver side. If it was rounding down, then we must solve a diophantine equation which does not seem trivial at all.

So please, fix this.

[tokenwood]

I figured out that the root cause of my problem was not the rounding down per se. My solving engine was submitting limit orders with 0 fee and the driver is adding a fee and modifying my clearing prices for these orders which turns my valid limit order solution into an invalid one.

Ideally I'd like to be able to disable the entire driver logic of adding custom limit order prices and only use the price vector I provided instead. I'd save on gas and it wouldn't cause revert errors.

[marcovc]

Indeed, disabling the driver would solve it. But I would like to use the driver, and even if going forward the driver is managed by the solver, I'd prefer to patch it as least as possible. I have a lot of respect for the people that work on the driver, and don't think I can really make a better job there.

In the smartcontract the exec_buy_amount (we are always talking about sell orders here) is computed as:

exec_buy_amount = ceil(exec_sell_amount * price_sell_token / price_of_buy_token) [1]

To me it makes sense the driver uses the same formula for any transformation it applies, at least when the transformation is advertised as "transparent" for the solver, which was the case for the protocol fee.

[tokenwood]

Just to be clear, I was asking for a driver config flag to not add custom limit prices, not to bypass the driver entirely
The driver adding a hidden fee was compounding the rounding problem, but even after I set a non-zero fee I still see the issue due to the rounding. I also see that in some parts of the codebase a ceil div is used:
line 181 of solver/src/settlement.rs:

let buy_amount = sell_amount .checked_mul(sell_price)? .checked_ceil_div(&buy_price)?;

Anyway, I agree with @marcovc that the driver shouldn't break a working solution and that it should use the same rounding method as the smart contract.

[fleupold]

Bypassing driver fees is captured by #2440. It should be straight forward so that issue could also be taken by an external contributor (see help-wanted tag).

Let's keep this issue here focussed on the settlement encoder not rounding the same way as the smart contract.