Protocol fee is rounded incorrectly, allowing users to circumvent it

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/math/src/omnipool/math.rs#L1

Vulnerability details

Bug Description

The Omnipool imposes a protocol fee which is used to rebalance the pool. This rebalancing is intended to protect the pool against the imbalance that occurs due to impermanent loss in the case of price changes. The protocol fee is imposed when a user swaps tokens using the pool.

let protocol_fee_amount = protocol_fee.mul_floor(delta_hub_reserve_in);

Unfortunately this rounds in favor of the user instead of in favor of the protocol, rounding down the fee on each calculation. So if a user swaps his funds in very small tranches, he can circumvent the protocol fee and get a cheaper swap price. The maximum swap amount for this to work will be

$$amountToWithdraw < 1/feePercentage$$

For a fee of 0.05% this would for example be:

$$amountToWithdraw < 1/0.0005$$

$$amountToWithdraw < 2000$$

As the protocol fee is substantial to keep the pool balanced, this will also allow the pool to get into imbalance way quicker.

Feasibility

In the runtime configuration the minimum trading limit is defined as MinTradingLimit : Balance = 1_000u128;. The protocol fee is a dynamic fee which is restricted by the runtime configuration in the range from 0.05% to 0.1%. Using our calculation from above, this means that this attack will be possible almost all the time as the minTradingLimit could only not be reached if the protocol fee is exactly 0.1%.

Impact

This issue allows a user to intentionally trade in smaller portions, so that the protocol fee will be 0. This will allow the user to trade at better prices and circumvent the protocol fee which is one of the security measures that protects the pool from imbalance.

Proof of Concept

Logical

The rounding direction can clearly be seen in the following LOC when selling, as well as this LOC when buying:

let protocol_fee_amount = protocol_fee.mul_floor(delta_hub_reserve_in);

Mathematical Example

A short mathematical example can also add to describe the issue. In this case a user wants to trade 5997 Token1 for Token2. In the first case he does this in one trade, in the second he splits it into multiple smaller swaps. For simplicity we will assume that the prices to LRNA are pegged 1:1 and don't significantly change (due to there being huge volume in the pool).

// Config
ProtocolFee = 0.05% = 0.0005
AssetFee = 0% //For simplicity

-------------------------------------------------------------------
// Case1: A user swaps all at once

delta_hub_reserve_in = 5997 * (1/1) = 5997
protocol_fee_amount = math.floor(5997*0.0005) = 2
delta_hub_reserve_out = 5997-2 = 5995
amountOut = 5997 * 1/1 = 5997

//So in this case the user got out 5997 Token2 and 2 tokens were used to rebalance the pool

-------------------------------------------------------------------
//Case2: Now a user does 3 exactly identical trades to circumvent the protcol fee

delta_hub_reserve_in = 1999 * (1/1) = 1999
protocol_fee_amount = math.floor(1999*0.0005) = 0
delta_hub_reserve_out = 1999-0 = 1999
amountOut = 1999 * 1/1 = 1999

//When he does this 3 times, he will receive 5997 tokens out, giving him a better price and not rebalancing the pool

Testcase

The following testcase shows an exemplary situation.

#[test]
fn sellProtocolFeeCanBeBypassed() {
    let asset_in_state = AssetReserveState {
        reserve: 1000000 * UNIT,
        hub_reserve: 1000000 * UNIT,
        shares: 1000000 * UNIT,
        protocol_shares: 0u128,
    };
    let asset_out_state = AssetReserveState {
        reserve: 1000000 * UNIT,
        hub_reserve: 1000000 * UNIT,
        shares: 1000000 * UNIT,
        protocol_shares: 0u128,
    };

    let amount_to_sell = 1999;
    let asset_fee = Permill::from_percent(0);
    let protocol_fee = Permill::from_float(0.0005); // 0.05% which is the minimum protocol fee
    let imbalance = 2 * UNIT;

    let state_changes = calculate_sell_state_changes(
        &asset_in_state,
        &asset_out_state,
        amount_to_sell,
        asset_fee,
        protocol_fee,
        imbalance,
    );

    assert!(state_changes.is_some());
    let state_changes = state_changes.unwrap();
    assert_eq!(
        state_changes.fee,
        TradeFee {
            asset_fee: 0,
            protocol_fee: 0, // No protocol fee was charged
        }
    );
}

The testcase must be added to the math/src/omnipool/tests.rs folder to run it correctly.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by rounding in favor of the protocol, instead of in favor of the user. This can be achieved by rounding the imposed fee up instead of down. This can be achieved by using the mul_ceil() instead of the mul_floor() function.

let protocol_fee_amount = protocol_fee.mul_ceil(delta_hub_reserve_in);

In the case described above this will not allow the user to circumvent the protocol fee in the way described:

//Case2: Now a user does 3 exactly same trades to circumvent the protcol fee

delta_hub_reserve_in = 1999 * (1/1) = 1999
protocol_fee_amount = math.ceil(1999*0.0005) = 1
delta_hub_reserve_out = 1999-1 = 1998
amountOut = 1998 * 1/1 = 1998

//When he does this 3 times, he will receive 5994 tokens out and 3 tokens are used to rebalance the pool

Assessed type

Math

[0xRobocop]

Consider QA, round loss seems too low considering tokens may have precision of at least 6 decimals.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

Dup of #163 by the same warden, closing this one as the other one was already downgraded to QA

[c4-judge]

OpenCoreCH marked the issue as nullified