code-423n4 / 2023-01-numoen-findings

0 stars 0 forks source link

## Significant roundoff error in invariant() function #198

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Pair.sol#L56 https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Pair.sol#L57

Vulnerability details

Significant roundoff error in invariant() function

When calculating scale0 and scale1 there is a significant roundoff error due to initial division (division by - uint256 liquidity).

    56  FullMath.mulDiv(amount0, 1e18, liquidity)

Proof of Concept

    53  function invariant(uint256 amount0, uint256 amount1, uint256 liquidity) public view override returns (bool) {
    54  if (liquidity == 0) return (amount0 == 0 && amount1 == 0);

    56  uint256 scale0 = FullMath.mulDiv(amount0, 1e18, liquidity) * token0Scale;
    57  uint256 scale1 = FullMath.mulDiv(amount1, 1e18, liquidity) * token1Scale;

    59  if (scale1 > 2 * upperBound) revert InvariantError();

    61  uint256 a = scale0 * 1e18;
    62  uint256 b = scale1 * upperBound;
    63  uint256 c = (scale1 * scale1) / 4;
    64  uint256 d = upperBound * upperBound;

    66  return a + b >= c + d;
    67  }

https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Pair.sol#L56

https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Pair.sol#L57

This FullMath.mulDiv(amount0, 1e18, liquidity) roundoff value again multiplied by 3 times when calculating a, b, c, d . Thats mean if consider calculating a

    a = FullMath.mulDiv(amount0, 1e18, liquidity)*token0Scale*scale0 * 1e18 

This cause even worse because roundoff value multiply by another 3 factors so finally its ended up with significant different value which it should be.

Tools Used

Vs code

Recommended Mitigation Steps

Perform all the multiplication first and divide eventually.

c4-judge commented 1 year ago

berndartmueller marked the issue as duplicate of #264

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by berndartmueller

berndartmueller commented 1 year ago

Due to the lack of a concrete impact besides the mentioning of the rounding issue (in comparison to https://github.com/code-423n4/2023-01-numoen-findings/issues/264) and the overall lack of quality, I'm downgrading this to QA (Low).

c4-judge commented 1 year ago

berndartmueller marked the issue as not a duplicate

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

berndartmueller marked the issue as grade-c