Mismatch of math specification and implementation might cause too little LRNA compensation

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L373-L382

Vulnerability details

Impact

The current implementation does not match the math specification. The current implementation grants the user much less LRNA compensation for impermanent loss than what they ought to get according to the formal specification.

Proof of Concept

Accroding to the math specification LRNA compensation that the user ought to receive is calculated as:

$$\Delta q^0_{\alpha} = -p^Q_i\left(\frac{2p^Q_i}{p^Qi+p{\alpha}} \frac{\Delta s_{\alpha}}{Si}R{i} + \Delta^0r_{\alpha}\right)$$

This also matches the code comment in omnipool/math.rs#calculate_remove_liquidity_state_changes:

// delta_q_a = -pi * ( 2pi / (pi + pa) * delta_s_a / Si * Ri + delta_r_a ) 
// note: delta_s_a is < 0

However, the following code does the following calculation:

let hub_transferred = if current_price > position_price { 
    let sub = current_hub_reserve_hp.checked_sub(p_x_r)?;
    let sum = current_hub_reserve_hp.checked_add(p_x_r)?;
    let div1 = current_hub_reserve_hp.checked_mul(sub)?.checked_div(sum)?;
    to_balance!(div1.checked_mul(delta_shares_hp)?.checked_div(current_shares_hp)?).ok()?
} else {
    Balance::zero()
};

Expressed in math notation this is:

$$\Delta q^0_{\alpha} = Q_i(Qi-p{\alpha}R_i)/(Qi+p{\alpha}Ri)*\Delta s{\alpha} / S_i$$

To show that these two formulas are not equivalent, the following example is given:

• x = 120 ($Q_i$ at LP time)
• y = 100 ($R_i$ at LP time)
• $p_{\alpha} = x/y = 1.2$ (price of asset at LP time)
• $x' = Q_i \approx 124.9$
• $y' = R_i \approx 96.0769$
• $p^Q_i = x'/y' = 1.3$ (current price of asset)
  • the swap invariant of $Q_i*R_i = 12000$ is maintained
• $\Delta s_{\alpha} = -1$ (shares to remove) for the specification and $1$ for the implementation (due to use of unsigned integers)
• $S_i = 100$ (total shares for that asset)
• $\Delta^0 r_{\alpha} = R_i/Si \Delta s{\alpha} \approx -0.9608$

Putting these values into the math specification formula:

$-1.3(21.3/(1.3+1.2)-1/100*96.0769+(-0.9608)) = 2.548$

Now the implementation formula:

$124.9(124.9-1.296.0769)/(124.9+1.296.0769)1/100 \approx 0.05$

As can be seen, the difference is about 1 to 2 order of magnitudes. However, looking at the numbers and using some common sense it appears that the current implementation is actually more reasonable. Hence I dont believe this issue to be of high severity and chances are that this is an issue in the specification.

Tools Used

Manual Review

Recommended Mitigation Steps

Adjust the implementation (can do a 1:1 conversion of formula to code as all the necessary values are accessible in the function scope) or the specification formula, depending on which one does not match the intended design.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[0xRobocop]

Report only describes "not as spec" but fails to give any impact. Leaving for sponsor review.

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

There is some mistakes in the calculation using math spec formula

all his numbers are right, except that the last -0.96... should actually be positive

if you do that change and then group things correctly, you get ~0.05, which matches his other answer

[OpenCoreCH]

There seems to be indeed a small sign error. Equation 7 in the linked math document states that $\Delta^0 r_\alpha$ is equal to $- \Delta R_i^0$, so equal to $- R_i/S_i \Delta S_i $. This minus is missing in the derivation of the warden, resulting in the wrong result.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid