code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/libraries/PositionMath.sol#L12-L18

Vulnerability details

Impact

For solidity version >= 0.8, PositionMath.addDelta reverts in an edge case while it can return a valid value.

Proof of Concept

PositionMath.addDelta reverts when y is the minimum value of int256.

  function addDelta(uint256 x, int256 y) internal pure returns (uint256 z) {
    if (y < 0) {
      require((z = x - uint256(-y)) < x, "LS"); //@audit -2**255
    } else {
      require((z = x + uint256(y)) >= x, "LA");
    }
  }

When y = -2*255, it reverts when it calculates -y. But it can return a valid result for x >= -y so this can be a problem. Although this is a very rare case, and practically it is not possible to reach this value in position math. But it's a bug so I raise this issue.

Tools Used

Manual Review

Recommended Mitigation Steps

- require((z = x - uint256(-y)) < x, "LS");
+ require((z = x - uint256(y == -2**255 ? y : -y)) < x, "LS");

berndartmueller commented 1 year ago

Great find! For anyone interested, I can recommend reading https://docs.soliditylang.org/en/v0.8.17/types.html#addition-subtraction-and-multiplication

Leaving it open for sponsor review, but due to the low likelihood of becoming a real issue, I'm inclined to downgrade to QA (Low).

kyscott18 commented 1 year ago

Okay this is only a problem when y == -2**256? because taking the negative of this number reverts?

c4-sponsor commented 1 year ago

kyscott18 requested judge review

berndartmueller commented 1 year ago

Okay this is only a problem when y == -2**256? because taking the negative of this number reverts?

It reverts if y equals -2*255 due to Solidity using the two’s complement representation internally.

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)