code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

The type conversion overflow caused quantity calculation error #414

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L17-L27

Vulnerability details

Impact

The code inventory is in the case of a large number of type conversions, but there is no overflow detection, and overflow may occur when a large type is converted to a small type, resulting in calculation errors.

Proof of Concept

  function estimatedClaimCount(
    uint8 _numberOfTiers,
    uint16 _grandPrizePeriod
  ) internal pure returns (uint32) {
    uint32 count = 0;
    for (uint8 i = 0; i < _numberOfTiers; i++) {
      count += uint32(
        uint256(
          unwrap(sd(int256(prizeCount(i))).mul(getTierOdds(i, _numberOfTiers, _grandPrizePeriod)))
        )
      );
    }
    return count;
  }

  function getTierOdds(
    uint8 _tier,
    uint8 _numberOfTiers,
    uint16 _grandPrizePeriod
  ) internal pure returns (SD59x18) {
    SD59x18 _k = sd(1).div(sd(int16(_grandPrizePeriod))).ln().div(
      sd((-1 * int8(_numberOfTiers) + 1))
    );

    return E.pow(_k.mul(sd(int8(_tier) - (int8(_numberOfTiers) - 1))));
  }

This is the most obvious example, where _grandPrizePeriod is uint16 but converted to int16, and _tier and _numberOfTiers are uint8 but converted to int8. When they are greater than 1/2 of the corresponding type, they overflow into negative numbers, resulting in a calculation error.

Tools Used

Manual review

Recommended Mitigation Steps

Detect overflow before converting type

Assessed type

Math

Picodes commented 1 year ago

In order for this report to be considered valid you need to prove that this can happen during the lifecycle of the contract

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient proof