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

12 stars 7 forks source link

```_nextDraw``` Does not check if the _nextNumberOfTiers is over the max possible value #319

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/abstract/TieredLiquidityDistributor.sol#L328-L375

Vulnerability details

Impact

The function _nextDraw is used to end a draw and set the next number of tiers, However it does have a possible issue. It checks if the next number of tiers is lower than the minimum. But it does not check for the upper limit.

Proof of Concept

  function _nextDraw(uint8 _nextNumberOfTiers, uint96 _prizeTokenLiquidity) internal {
    if (_nextNumberOfTiers < MINIMUM_NUMBER_OF_TIERS) {
      revert NumberOfTiersLessThanMinimum(_nextNumberOfTiers);
    }

    uint8 numTiers = numberOfTiers;
    UD60x18 _prizeTokenPerShare = fromUD34x4toUD60x18(prizeTokenPerShare);
    (
      uint16 closedDrawId,
      uint104 newReserve,
      UD60x18 newPrizeTokenPerShare
    ) = _computeNewDistributions(
        numTiers,
        _nextNumberOfTiers,
        _prizeTokenPerShare,
        _prizeTokenLiquidity
      );

Here we see that the lower bound is checked, but the upper bound for max number of tiers is not checked.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check for the MAXIMUM_NUMBER_OF_TIERS like it is done in the constructor.

    if (_nextNumberOfTiers < MINIMUM_NUMBER_OF_TIERS) {
      revert NumberOfTiersLessThanMinimum(_nextNumberOfTiers);
    }
    if (_nextNumberOfTiers> MAXIMUM_NUMBER_OF_TIERS) {
      revert NumberOfTiersGreaterThanMaximum(_nextNumberOfTiers);
    }

Assessed type

Error

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient proof

Picodes commented 1 year ago

This function is only called here, where the input is the result of this function, which implies the max limit. Invalidating as I don't consider that this report pushes the investigation far enough