LP providers can claim part of their own fee when adding liquidity

[hats-bug-reporter[bot]]

Github username: @0x3b Twitter username: 0x3b33 Submission hash (on-chain): 0x2573923fbe62af2cf90005e3222c40f4db4c38f8e5fc07a868d59f3979a72c83 Severity: medium

Description: Description\ add_liquidity is set up to collect fees from LP providers, however when adding shares users claim a aprt of their own fee.

Attack Scenario\ add_liquidity calls rated_compute_lp_amount_for_deposit in order to calculate the shares and fees for our user, where inside rated_compute_lp_amount_for_deposita call is made to the actual function that calculates LP amount and fees - compute_lp_amount_for_deposit.

Inside it we callculate the fees and shares based on the difference between the new and ideal reserves.

                let ideal_reserve: u128 = d_1
                    .checked_mul(old_reserves[i].into())
                    .ok_or(MathError::MulOverflow(17))?
                    .checked_div(d_0)
                    .ok_or(MathError::DivByZero(9))?
                    .try_into()
                    .map_err(|_| MathError::CastOverflow(2))?;

                let difference = ideal_reserve.abs_diff(new_reserves[i]);
                let fee = _fees.normalized_trade_fee(n_coins, difference)?;
                new_reserves[i] = new_reserves[i]
                    .checked_sub(fee)
                    .ok_or(MathError::SubUnderflow(18))?;

We need to take into account how are our LP shares calculated, as they are done for our old_reserves, not accounting the fact that add_liquidity will dedicate 50%, or more of the fee to share value increase.

            let mint_shares: u128 = U256::from(pool_token_supply)
                .checked_mul(d_2.checked_sub(d_0).ok_or(MathError::SubUnderflow(19))?)
                .ok_or(MathError::MulOverflow(18))?
                .checked_div(d_0)
                .ok_or(MathError::DivByZero(10))?
                .try_into()
                .map_err(|_| MathError::CastOverflow(3))?;

The above means that new LP providers will claim some portion of the fees that they have provided, resulting in smaller fee for the rest of the providers.

Example:

System before add_liquidity

Prerequisites	Values
Pool LP	1000
Total fee	10%
Fee LP providers	10%
Fee protocol	0%
Amount A	1000
Amount B	1000

1. User1 adds 1000 tokenA and 1000 tokenB, receiving 900 LP in return.
2. 100 from both tokens is kept as fee in the form of share value increase

System after add_liquidity

Prerequisites	Values
Pool LP	1950
Total fee	10%
Fee LP providers	10%
Fee protocol	0%
Amount A	2000
Amount B	2000

Notice how there are 1900 LP tokens and our user has 900 of them, resulting in 947.36 actual tokens.

(2000 / 1900) * 900 = 947.36

Our user technically paid a ~5% fee and not a 10% one.

Recommendation Calculate the shares minted based on the increased reserves from the fee.

[JanKuczma]

Thank you for your submission.

There are few assumptions that are incorrect:

• The maximum trade_fee is 1%.
• When adding/removing liquidity, the fee is calculated based on the difference between the new reserves and the ideal reserves, so in case the amounts are ideal the fee is 0.