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

12 stars 7 forks source link

Integer Overflow Vulnerability in `canaryPrizeCount` Function and Insecure Pseudo-Random Number Generation in `calculatePseudoRandomNumber` Function. #279

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#L52-L64 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L104-L111

Vulnerability details

Impact

calculatePseudoRandomNumber function computes a pseudo-random number based on user-specific parameters. However, it uses the keccak256 hash function, which may not provide a sufficiently secure and unpredictable random number. It is recommended to use a more robust and secure random number generation algorithm, such as using a cryptographic library or an external oracle, to ensure the randomness and security of the pseudo-random number.

Proof of Concept

  1. Integer Overflow Vuln in canaryPrizeCount Function: The canaryPrizeCount function contains a potential integer overflow vulnerability when performing large multiplications.

Code block of the canaryPrizeCount function: TierCalculationLib.sol#L52-L63

function canaryPrizeCount(
  uint8 _numberOfTiers,
  uint8 _canaryShares,
  uint8 _reserveShares,
  uint8 _tierShares
) internal pure returns (UD60x18) {
  uint256 numerator = uint256(_canaryShares) *
    ((_numberOfTiers + 1) * uint256(_tierShares) + _canaryShares + _reserveShares);
  uint256 denominator = uint256(_tierShares) *
    ((_numberOfTiers) * uint256(_tierShares) + _canaryShares + _reserveShares);
  UD60x18 multiplier = toUD60x18(numerator).div(toUD60x18(denominator));
  return multiplier.mul(toUD60x18(prizeCount(_numberOfTiers)));
}

In this code block, the numerator and denominator variables are calculated by multiplying large uint256 values. If the result of the multiplication exceeds the maximum value representable by a uint256, an integer overflow can occur.

The vulnerability arises from the multiplication of the following values:

If any of these multiplications result in a value that exceeds the maximum value representable by a uint256, an integer overflow occurs, leading to incorrect results or unexpected behavior in the calculation.

To demonstrate the exploitability of this vulnerability, consider the following example:

// Set the inputs that trigger the vulnerability
uint8 numberOfTiers = 10;
uint8 canaryShares = 100;
uint8 reserveShares = 200;
uint8 tierShares = 30;

// Calculate the numerator and denominator
uint256 numerator = uint256(canaryShares) *
  ((numberOfTiers + 1) * uint256(tierShares) + canaryShares + reserveShares);
uint256 denominator = uint256(tierShares) *
  (numberOfTiers * uint256(tierShares) + canaryShares + reserveShares);

// Output the numerator and denominator values
console.log("Numerator:", numerator);
console.log("Denominator:", denominator);

In this example, the inputs are chosen in a way that triggers the multiplication of large values. The resulting numerator and denominator values can potentially exceed the maximum value representable by a uint256, leading to an integer overflow.

By this proof and example, we have seen and illustrated the existence and exploitability of the potential integer overflow vulnerability in the canaryPrizeCount function.

  1. Insecure Pseudo-Random Number Generation in calculatePseudoRandomNumber Function: The calculatePseudoRandomNumber function uses the keccak256 hash function to generate a pseudo-random number. However, this method may not provide a sufficiently secure and unpredictable random number, compromising the randomness and security of the generated value.

The calculatePseudoRandomNumber function generates a pseudo-random number based on user-specific parameters using the keccak256 hash function. However, this method may not provide a sufficiently secure and unpredictable random number.

Code block: TierCalculationLib.sol#L104-L111

function calculatePseudoRandomNumber(
  address _user,
  uint8 _tier,
  uint32 _prizeIndex,
  uint256 _winningRandomNumber
) internal pure returns (uint256) {
  return uint256(keccak256(abi.encode(_user, _tier, _prizeIndex, _winningRandomNumber)));
}

The keccak256 hash function is deterministic and susceptible to potential predictability attacks. An attacker may be able to manipulate the inputs to influence the generated pseudo-random number, compromising the randomness and security of the calculation.

The impact of this vulnerability is that it undermines the fairness and security of any mechanism relying on the pseudo-random number generated by this function, potentially enabling attackers to exploit the system and gain unfair advantages.

References:

Tools Used

Manual review.

Recommended Mitigation Steps

  1. Integer Overflow:

    • Implement proper input validation and bounds checking to prevent potential integer overflow.
    • Use safe mathematical operations or libraries that handle overflow conditions gracefully.
  2. Insecure Pseudo-Random Number Generation:

    • Replace the current keccak256 hash function with a more robust and secure random number generation algorithm.
    • Consider using a cryptographic library or an external oracle to obtain secure and unpredictable random numbers.
    • Follow best practices and established standards for random number generation to ensure the fairness and security of the system.

Assessed type

Math

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid