c4-bot-8 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1462-L1465 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L560

Vulnerability details

Impact

In stableswap, Incorrect d value might be used in various trading and liquidity calculation, resulting in either pool's loss or users' loss.

Proof of Concept

Newton methods is used in converging the d-value from stableswap's invariant model, to determine the delta reserve amount in trading or liquidity management. See curve's stable swap whitepaper - page 4 for the invariant model.

The weakness of Newton method is if the initial guess is far from the actual root, or if the function has multiple roots in close proximity, it may fail to converge.

The problem is current implementation of Newton method in calculate_d_internal() reduces the number of iterations (compared to Curve's code) but doesn't have a safeguard against potential fail, and might allow any value to be used for delta reserve calculation.

In calculate_d_internal(), D is max iterations of Newton run and is 64. (pub const MAX_D_ITERATIONS: u8 = 64;). (1) Success case: If convergence is reached (d == d_prev or d ==d_prev+1), d is a satisfied value and will be returned. This is fine.

(2) failure case: If convergence is not reached within 64 iterations, no check on how close d value is from has_converged criteria. Any d value will be passed and used.

pub(crate) fn calculate_d_internal<const D: u8>(xp: &[Balance], amplification: Balance) -> Option<Balance> {
...
    let mut d = s_hp;
...
    for _ in 0..D {
...
        d = ann_hp
            .checked_mul(s_hp)?
            .checked_add(d_p.checked_mul(n_coins)?)?
            .checked_mul(d)?
            .checked_div(
                ann_hp
                    .checked_sub(U256::one())?
                    .checked_mul(d)?
                    .checked_add(n_coins.checked_add(U256::one())?.checked_mul(d_p)?)?,
            )?

        if has_converged(d_prev, d, precision_hp) {
            // If runtime-benchmarks - don't return and force max iterations
            #[cfg(not(feature = "runtime-benchmarks"))]
//@audit-info success case: d returned here satisfy `has_converged()` criteria and is considered correct.
|>          return Balance::try_from(d).ok();
        }
    }
//@audit potential failure case: no check on how close d is to be considered as convergent. Any `d` value will be passed and used. 
|>  Balance::try_from(d).ok()
}

((https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L508))

Note: In comparison, Curve uses 255 iterations - much more than 64. A lower number of iterations means a higher risk of unsatisfied and incorrect values will be returned.

If this is the case, calculate_d_internal() should check the d value after max 64 iterations to only allow acceptably close d to be used, instead of any d value.

In addition, same vulnerability also applies to calculate_y_internal(). See links to the affected code.

Tools Used

Manual

Recommended Mitigation Steps

Either increase the number of iterations Or establish a wider range of allowable convergence to be checked at the end of max iterations. And revert when the returned d doesn't satisfy an acceptable wider range.

Assessed type

Math

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as insufficient quality report

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)

OpenCoreCH commented 6 months ago

Valid recommendation that increasing the number of iterations may be something to consider, but no proof provided that the current number is a problem in any way -> QA