Inconsistency in functionality and spec will lead to DOS, and possibly theft of funds
The spec for `burn_liquidity()` claims multiple LP tokens from different pools can be burned in a single call, but the implementation reverts on any pool mismatch. Integrators relying on the spec can be frontrun and lose shares.
Description
On the call to burn_liquidity(), it is stated in the comment spec that multiple LP tokens from different pools can be burned on a single call:
/// * LP tokens for multiple pools can be mixed in the same call.#[storage(read, write)]fn burn_liquidity(args: BurnLiquidityArgs) -> Vec<Asset> {
However, this is not possible where the required_pool_id is cached before the loop to burn each LP token in the array subsequently:
//@auditlet required_pool_id = get_pool_id_from_lp_token(storage_keys,args.lp_assets.get(0).unwrap());let mut reserves_withdrawn: Vec<Asset> = Vec::new();for lp_asset in args.lp_assets.iter() {let (asset_1, asset_2) = burn_lp_token(storage_keys, lp_asset, args.to,required_pool_id);transfer_reserves(asset_1, asset_2, args.to);if asset_1.amount > 0 {reserves_withdrawn.push(asset_1);}if asset_2.amount > 0 {reserves_withdrawn.push(asset_2);}}
Since the required_pool_id is locked to the initial LP token's pool to be burned, such that burning an LP token from a different pool will revert on the validations in verify_lp_token():
/// Verifies that the LP token is valid#[storage(read)]fn verify_lp_token(storage_keys: BurnStorageKeys, asset_id: AssetId, required_pool_id:PoolId) {//.. snip ..require(pool_id.unwrap() == required_pool_id,PoolCurveStateError::LPTokenFromWrongPool);//@audit}
This inconsistency between the actual functionality and the commented spec can lead to harmful scenarios for protocols or users trying to integrate with Mira binned liquidity. For instance:
- A user sends LP tokens from different pools to the curve state prior to the
burn_liquidity()call (as this is a required). - Trying to call
burn_liquidity()now reverts due to the actual functionality in the code. - Before the user can place a certain number of transactions to burn the share of the different LP tokens they sent to the contract.
- An attacker can frontrun them to burn those shares for themselves.
Impact
Integrators following the spec are at risk of denial-of-service and frontrunning attacks where attackers can burn the residual LP shares left in the curve state.
Recommendation
Correct the spec and let users know how to properly integrate with the curve state, as every transaction to burn should be bundled in one call.
Resolution
Fixed in commit 96aa6677a491063d37c81e5fbce252aa97f16873.

