PoolTogether - volodya's results

Lines of code

https://github.com/generationsoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L781

Vulnerability details

Impact

Detailed description of the impact of this finding. Number of tiers calculations doesn't work like supposed to.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. According to docs Number of tiers only increased when both

the number of claimed standard prizes exceeds the claim threshold % of the expected number of standard prizes. the number of claimed canary prizes exceeds the expected number of canary prizes

And decreases when

The number of tiers is decreased if the highest tier claimed is less than n-2

But from the code number of tiers will increase when largestTierClaimed >= _numTiers - 1 If is suppose to work like that than what should happen if largestTierClaimed + 2 > _numTiers + 1 there should be maximum

  function _computeNextNumberOfTiers(uint8 _numTiers) internal view returns (uint8) {
    UD2x18 _claimExpansionThreshold = claimExpansionThreshold;

    uint8 _nextNumberOfTiers = largestTierClaimed + 2; // canary tier, then length
    _nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
      ? _nextNumberOfTiers
      : MINIMUM_NUMBER_OF_TIERS;

    if (_nextNumberOfTiers >= MAXIMUM_NUMBER_OF_TIERS) {
      return MAXIMUM_NUMBER_OF_TIERS;
    }

    // check to see if we need to expand the number of tiers
    if (
      _nextNumberOfTiers >= _numTiers &&
      canaryClaimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
      ) &&
      claimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
      )
    ) {
      // increase the number of tiers to include a new tier
      _nextNumberOfTiers = _numTiers + 1; //@audit what if largestTierClaimed + 2 >= _numTiers + 1
    }
// @audit will just return largestTierClaimed + 2
    return _nextNumberOfTiers;
  }

src/PrizePool.sol#L781

Tools Used

Recommended Mitigation Steps

Rewrite whole function according to docs and video so it will increases only on meeting threshold and decreasing when The number of tiers is decreased if the highest tier claimed is less than

  function _computeNextNumberOfTiers(uint8 _numTiers) internal view returns (uint8) {
    UD2x18 _claimExpansionThreshold = claimExpansionThreshold;

    uint8 _nextNumberOfTiers = largestTierClaimed + 2; // canary tier, then length
    _nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
      ? _nextNumberOfTiers
      : MINIMUM_NUMBER_OF_TIERS;

-    if (_nextNumberOfTiers >= MAXIMUM_NUMBER_OF_TIERS) {
-      return MAXIMUM_NUMBER_OF_TIERS;
-    }
+    if (_nextNumberOfTiers >= _numTiers) {
+      _nextNumberOfTiers = _numTiers;
+    }

    // check to see if we need to expand the number of tiers
    if (
      _nextNumberOfTiers >= _numTiers &&
      canaryClaimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
      ) &&
      claimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
      )
    ) {
      // increase the number of tiers to include a new tier
      _nextNumberOfTiers = _numTiers + 1;
    }

    return _nextNumberOfTiers;
  }

Assessed type

Error

Lines of code

https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L76

Vulnerability details

Impact

Detailed description of the impact of this finding. Inconsistent behavior for canary claims in claimer

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Whenever claimer claims prizes he computes fees per claim without considering canary claims

  function claimPrizes(
    Vault vault,
    uint8 tier,
    address[] calldata winners,
    uint32[][] calldata prizeIndices,
    address _feeRecipient
  ) external returns (uint256 totalFees) {
    uint256 claimCount;
    for (uint i = 0; i < winners.length; i++) {
      claimCount += prizeIndices[i].length;
    }

    uint96 feePerClaim = uint96(
      _computeFeePerClaim(
        _computeMaxFee(tier, prizePool.numberOfTiers()),
        claimCount,
        prizePool.claimCount()
      )
    );

    vault.claimPrizes(tier, winners, prizeIndices, feePerClaim, _feeRecipient);

    return feePerClaim * claimCount;
  }

src/Claimer.sol#L76

Tools Used

Recommended Mitigation Steps

Consider canary as claim

    uint96 feePerClaim = uint96(
      _computeFeePerClaim(
        _computeMaxFee(tier, prizePool.numberOfTiers()),
        claimCount,
-        prizePool.claimCount()
+        prizePool.claimCount() + prizePool.canaryClaimCount()
      )
    );

Assessed type

Error

Lines of code

https://github.com/generationsoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L26

Vulnerability details

Impact

Detailed description of the impact of this finding. Tiers odds compuations is not accurate

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. If you will go to wolfram link than equation for tiers can be simplified to this

So code for ods will become like this

  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))));
  }
// new equation
  function getTierOdds2(
    uint8 _tier,
    uint8 _numberOfTiers,
    uint16 _grandPrizePeriod
  ) internal pure returns (SD59x18) {

    return sd(1).div(sd(int16(_grandPrizePeriod))).pow((sd(int8(_tier) - (int8(_numberOfTiers) - 1))).div(sd((-1 * int8(_numberOfTiers) + 1))));
  }

blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L26

Here is an example before and after, its not a huge difference. But seems like real tier odds are slightly less than current this means that protocol will give out prize a little bit more than it should. Above is current formula, below for new formula.

  SD59x18 internal constant TIER_ODDS_1_4 = SD59x18.wrap(19579642462506911);
  SD59x18 internal constant TIER_ODDS_1_4 = SD59x18.wrap(19579642462506911);
  ----------
  SD59x18 internal constant TIER_ODDS_2_4 = SD59x18.wrap(139927275620255366);
  SD59x18 internal constant TIER_ODDS_2_4 = SD59x18.wrap(139927275620255364);
  ----------
  SD59x18 internal constant TIER_ODDS_3_4 = SD59x18.wrap(1000000000000000000);
  SD59x18 internal constant TIER_ODDS_3_4 = SD59x18.wrap(1000000000000000000);
  ----------
  SD59x18 internal constant TIER_ODDS_0_5 = SD59x18.wrap(2739726027397260);
  SD59x18 internal constant TIER_ODDS_0_5 = SD59x18.wrap(2739726027397260);
  ----------
  SD59x18 internal constant TIER_ODDS_1_5 = SD59x18.wrap(11975133168707466);
  SD59x18 internal constant TIER_ODDS_1_5 = SD59x18.wrap(11975133168707465);
  ----------
  SD59x18 internal constant TIER_ODDS_2_5 = SD59x18.wrap(52342392259021369);
  SD59x18 internal constant TIER_ODDS_2_5 = SD59x18.wrap(52342392259021367);
  ----------
  SD59x18 internal constant TIER_ODDS_3_5 = SD59x18.wrap(228784597949733865);
  SD59x18 internal constant TIER_ODDS_3_5 = SD59x18.wrap(228784597949733862);

Tools Used

Recommended Mitigation Steps

Fix a tier odds

Assessed type

Error