Incorrect rounding behavior in router::get_coin_in_with_fees
Description
In the function router::get_coin_in_with_fees, the result is rounded up incorrectly for both stable and uncorrelated curves, which can lead to an undue amount being paid in fees.
The formula for rounding up integer division is (n - 1)/d + 1 for n > 0.
let coin_in = (stable_curve::coin_in(
(coin_out as u128),
scale_out,
scale_in,
(reserve_out as u128),
(reserve_in as u128),
) as u64) + 1;
(coin_in * fee_scale / fee_multiplier) + 1The stable curve branch of router::get_coin_in_with_fees does not correctly implement the formula stated above.
let coin_in = math::mul_div(
coin_out, // y
reserve_in * fee_scale, // rx * 1000
new_reserves_out // (ry - y) * 997
) + 1;Furthermore, the uncorrelated curve branch also incorrectly implements the formula stated above.
Impact
For certain swap amounts, a user could end up paying more in fees than would be accurate.
Recommendations
In the case of the stable curve branch of router::get_coin_in_with_fees, the code should be rewritten to adhere to the rounded up integer division formula.
let coin_in = (stable_curve::coin_in(
(coin_out as u128),
scale_out,
scale_in,
(reserve_out as u128),
(reserve_in as u128),
) as u64);
let n = coin_in * fee_scale;
if (n > 0) {
((n - 1) / fee_multiplier) + 1
} else {
0
}Likewise, the uncorrelated curve branch also needs a revision.
// add to liquidswap::math
public fun mul_div_rounded_up(x: u64, y: u64, z: u64): u64 {
assert!(z != 0, ERR_DIVIDE_BY_ZERO);
let n = (x as u128) * (y as u128);
let r = if (n > 0) {
((n - 1) / (z as u128)) + 1
} else {
0
}
(r as u64)
}
let coin_in = math::mul_div_rounded_up(
coin_out, // y
reserve_in * fee_scale, // rx * 1000
new_reserves_out // (ry - y) * 997
);Remediation
Pontem Network fixed this issue in commit 0b01ed6↗