The omnipool pallet is missing slippage protection when adding and removing liquidity. That makes users vulnerable from receiving less shares/assets than intended, as the amount depends on the current state of the pool, which can be manipulated to a certain point. That's why Uniswap makes it possible for users to specify the minimum amount they are willing to receive (check this and this), so that their transaction will revert if they receive less than intended.
Proof of Concept
NOTE $\Rightarrow$ I will work with the addition of liquidity as a general example. The same applies (with different state equations) to the remove of liquidity.
If we go to the math implementation of the state transition when adding liquidity in
which depend directly on the "current" state of the chain. On top of that, such value is never checked to be above a certain threshold specified either by the caller or the system in omnipool, function add_liquidity, whcih effectively makes users receive ANY amount of shares, even 0, for their deposited assets, which is indeed a loss of funds.
The runnable POC is the next one (put it inside the tests folder, file add_liquidity.rs):
#[test]
fn poc_add_liquidity_slippage() {
ExtBuilder::default()
.add_endowed_accounts((LP1, 1_000, 5000 * ONE))
.add_endowed_accounts((LP2, 1_000, 5000 * ONE))
.with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
.with_token(1_000, FixedU128::from_float(0.65), LP2, 2000 * ONE)
.build()
.execute_with(|| {
let token_amount = 2000 * ONE;
let liq_added = 400 * ONE;
// @audit say a transaction from other user got first to the HydraDX node
assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP2), 1_000, liq_added));
// ACT
let position_id = last_position_id();
assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));
@audit what LP1 user got
let position = Positions::<Test>::get(position_id).unwrap();
// @audit what the LP1 user expected
let expected = Position::<Balance, AssetId> {
asset_id: 1_000,
amount: liq_added,
shares: liq_added,
price: (1560 * ONE, token_amount + liq_added),
};
// @audit they are definetly not the same, slippage control missing
assert_ne!(position, expected);
});
}
Recommended Mitigation Steps
Make it possible for users to specify a certain threshold by which the transaction will revert if they receive les shares/assets for their funds, as done in stableswap, function remove_liquidity_one_asset or the Uniswap example I mentioned above.
Lines of code
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L577 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L716
Vulnerability details
Impact
The omnipool pallet is missing slippage protection when adding and removing liquidity. That makes users vulnerable from receiving less shares/assets than intended, as the amount depends on the current state of the pool, which can be manipulated to a certain point. That's why Uniswap makes it possible for users to specify the minimum amount they are willing to receive (check this and this), so that their transaction will revert if they receive less than intended.
Proof of Concept
NOTE $\Rightarrow$ I will work with the addition of liquidity as a general example. The same applies (with different state equations) to the remove of liquidity.
If we go to the math implementation of the state transition when adding liquidity in
omnipool/math.rs, function calculate_add_liquidity_state_changes
we can see that the amount of shares users will receive for their assets will be, roughly:
$$issued_shares = shares_prev * amount_deposited \ / \ reserves$$
which depend directly on the "current" state of the chain. On top of that, such value is never checked to be above a certain threshold specified either by the caller or the system in omnipool, function add_liquidity, whcih effectively makes users receive ANY amount of shares, even 0, for their deposited assets, which is indeed a loss of funds.
The runnable POC is the next one (put it inside the tests folder, file
add_liquidity.rs
):Recommended Mitigation Steps
Make it possible for users to specify a certain threshold by which the transaction will revert if they receive les shares/assets for their funds, as done in stableswap, function remove_liquidity_one_asset or the Uniswap example I mentioned above.
Assessed type
Other