Removing liquidity by amount not rounding up, user will burn less LP share

[hats-bug-reporter[bot]]

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

Description: Description:

The remove_liquidity_by_amounts allows users to withdraw liquidity from a pool by specifying the desired withdrawal amounts of tokens. It will burns LP tokens and withdraws underlying tokens back to user.

        fn remove_liquidity_by_amounts(
            &mut self,
            max_share_amount: u128,
            amounts: Vec<u128>,
            to: AccountId,
        ) -> Result<(u128, u128), StablePoolError> {
...
            // calc comparable amounts
@>          let (shares_to_burn, fee_part) = math::rated_compute_lp_amount_for_withdraw(
                &rates,
                &amounts,
                &self.reserves(),
                self.psp22.total_supply(),
                Some(&self.pool.fees),
                self.amp_coef(),
            )?;
...
        }

The part where converting amounts to shares_to_burn is where the issue occured. This rated_compute_lp_amount_for_withdraw call eventually goes to compute_lp_amount_for_withdraw.

/// Given `withdraw_amounts` user want get, calculates how many lpt
/// are required to be burnt
/// Returns a tuple of (lpt to burn, fee part)
fn compute_lp_amount_for_withdraw(
    withdraw_amounts: &[u128],
    old_reserves: &Vec<u128>,
    pool_token_supply: u128,
    fees: Option<&Fees>,
    amp_coef: u128,
) -> Result<(u128, u128), MathError> {
...
    if let Some(_fees) = fees {
...
        let burn_shares = U256::from(pool_token_supply)
            .checked_mul(d_0.checked_sub(d_2).ok_or(MathError::SubUnderflow(28))?)
            .ok_or(MathError::MulOverflow(23))?
@>          .checked_div(d_0)
            .ok_or(MathError::DivByZero(15))?
            .try_into()
            .map_err(|_| MathError::CastOverflow(8))?;
        let diff_shares = U256::from(pool_token_supply)
            .checked_mul(d_0.checked_sub(d_1).ok_or(MathError::SubUnderflow(29))?)
            .ok_or(MathError::MulOverflow(24))?
            .checked_div(d_0)
            .ok_or(MathError::DivByZero(16))?
            .try_into()
            .map_err(|_| MathError::CastOverflow(9))?;
        Ok((
            burn_shares,
            burn_shares
                .checked_sub(diff_shares)
                .ok_or(MathError::SubUnderflow(30))?,
        ))
    } else {
        let burn_shares = U256::from(pool_token_supply)
            .checked_mul(d_0.checked_sub(d_1).ok_or(MathError::SubUnderflow(31))?)
            .ok_or(MathError::MulOverflow(25))?
@>          .checked_div(d_0)
            .ok_or(MathError::DivByZero(17))?
            .try_into()
            .map_err(|_| MathError::CastOverflow(10))?;
        Ok((burn_shares, 0))
    }
}

based on code above, the checked_div code rounds down, this will cause calculations to be in favor of the user instead of the protocol.

In calculating the amount of shares to burn for amount of underlying tokens a user has to provide, it should round up. This is similar function logic with previewWithdraw in ERC4626, where it should be a rounding up.

The rounding down in current compute_lp_amount_for_withdraw code could lead to the protocol burning slightly fewer shares than calculated. This translates to users receiving a slightly higher proportional value back from the pool for their withdrawn assets.

Referring to Curve, the previewWithdraw which calculate number of shares which gets burned when withdrawing given amount of asset, is using a rounding up. Meanwhile in this compute_lp_amount_for_withdraw the rounding is down.

Scenario:

Using flashloan, user can deposit some amount of assets, get LP tokens with proportional share to what user deposits. Then they can proceed with withdrawing asset with remove_liquidity_by_amounts, by burning lesser share, they can repeatedly do this action until liquidity drained.

Impact:

Due to rounding issue, user will burn less LP share for amount they withdraw, thus this can negatively impact the protocol's liquidity or balance. The impact will amplitude if the amount is larger.

Mitigation:

Implement rounding up in compute_lp_amount_for_withdraw to align with Curve's approach and common ERC4626.

    let burn_shares = (pool_token_supply as u256)
        .checked_mul((d_0 as u256).checked_sub(d_2 as u256).ok_or(MathError::SubUnderflow(28)))?
        .ok_or(MathError::MulOverflow(23))?
-       .checked_div(d_0 as u256)
+       .checked_div_ceil(d_0 as u256)
        .ok_or(MathError::DivByZero(15))?
        .try_into()
        .map_err(|_| MathError::CastOverflow(8))?;

[JanKuczma]

Thank you for your submission.

That's an interesting finding. I tried to test the scenario you described with various setups and inputs but was unsuccessful. I wasn't able to withdraw the same amount of assets with a smaller amount of liquidity nor withdraw a slightly larger amount of assets with the same amount of liquidity.

Could you write a test (like this for example) proving the exploit or give the concrete setup and input data so it can be tested?

[JanKuczma]

Invalid due to lack of valid proof of the vulnerability.