add_liquidity : fee charging is missed on the first caller when adding liquidity

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xec91a1d40021f8dc0d4c43c0ee4f219964045bafd3d9867143639b231b2a9d25 Severity: medium

Description: Description\

when a user calls the function add_liquidity to add liquidity.

add_liquidity calls the rated_compute_lp_amount_for_deposit to compute the lp token amount and the fee. when the pool does not have any funds deposited, i.e when this deposit is first , the fee value retured from the rated_compute_lp_amount_for_deposit function is zero.

rated_compute_lp_amount_for_deposit -> compute_lp_amount_for_deposit

compute_lp_amount_for_deposit

fn compute_lp_amount_for_deposit(
    deposit_amounts: &Vec<u128>,
    old_reserves: &Vec<u128>,
    pool_token_supply: u128,
    fees: Option<&Fees>,
    amp_coef: u128,
) -> Result<(u128, u128), MathError> {
    if pool_token_supply == 0 { --->> this will come for first deposit.
        if deposit_amounts.contains(&0) {
            return Err(MathError::DivByZero(8));
        }
        Ok((
            compute_d(deposit_amounts, amp_coef)?
                .try_into()
                .map_err(|_| MathError::CastOverflow(1))?,
            0, ---->>> @@@ zero as fee
        ))
    } 

Impact

when the pool_token_supply is zero, no fee is charged. So, the protocol misses the fee.

2. Revised Code File (Optional)

We would sugges to consider fee factor on the first deposit call as well.

[JanKuczma]

Thank you for your submission.

When adding liquidity, the fee is calculated based on the difference between the new reserves and the ideal reserves. When the pool is empty (all reserves are 0, total LP tokens is 0), it is impossible to calculate the ideal reserves. Not accounting for fees in this case is intentional.

[aktech297]

Thank you for your submission.

When adding liquidity, the fee is calculated based on the difference between the new reserves and the ideal reserves. When the pool is empty (all reserves are 0, total LP tokens is 0), it is impossible to calculate the ideal reserves. Not accounting for fees in this case is intentional.

hey, I am not sure this is mentioned already in the readme. since the fee charging is applied whenever adding liquidity, we thought it is missed.

One way to avoid this issue is, when creating the pool, add some dust amount, so that when user makes deposit, the fee factor would be considered flawlessly.

[aktech297]

adding solution before the contest closed:

when creating the stable pool, consider depositing small amount . so that when the first user deposit, the fee will be considered.

[aktech297]

@JanKuczma

[JanKuczma]

It is not a bug or vulnerability but an intentional behavior of the contract.

[aktech297]

It is not a bug or vulnerability but an intentional behavior of the contract.

Hey.. understood.. thanks for your input

[aktech297]

After reviewing this issue and the comments from the sponsor, we would like to raise this issue. This issue exists in all the pools. Sponsor says this is intended behavior, but we could not provided any detail about it. When we see the issue #39, the sponsor says one of the reason to accept despite it falls under owner mistake is,

Why award a bounty at all? This vulnerability is not quite typical but one could argue that if 1) the contract has an active admin, 2) the admin is not aware of this possible issue, 3) there is huge liquidity in the contract, then a specialized party could exploit the vulnerability and siphon some funds. Since we didn't include any warning about this in the documentation or in-code comments, the security researchers could assume that we were not aware of that and hence rightfully make a submission about it.

refer this comment - Since we didn't include any warning about this in the documentation or in-code comments, the security researchers could assume that we were not aware of that and hence rightfully make a submission about it

The same scenario applies here, this behavior is not notified anywhere so went on to submit the issue as the fee part is missed on the first depositor. If you see the solution we provided, it is right solution to fix this issue.

it would be fair to treat both of these cases equally.