code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L361 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L446-L448 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L781-L810

Vulnerability details

Impact

As mentioned in the docs, "The Canary Tier is a special prize tier that receives a smaller amount of prize liquidity. The Canary Tier informs the Prize Pool as to whether it's worth increasing the number of prize tiers. The canary tier's role is to let us know whether it's worth offering n+1 tiers of prizes".

The intended behaviour is that the number of tiers only increase if a high enough portion of both the highest non-canary tier prizes and the canary tier prizes are claimed, so that prizes scale with demand and liquidity. However, there is a bug that causes the number of tiers to increase if at least 1 canary prize is claimed. Therefore it is highly likely that it will take just 12 draws to reach the cap of 15 prize tiers (or an attacker could force the situation by claiming canary prizes), at which point the prize sizes will be broken based on the liquidity provided and the protocol will become almost unusable.

Proof of Concept

When a draw has finished/elapsed, the draw manager closes the draw by calling closeDraw. During this closing process the next number of tiers is computed:

_nextNumberOfTiers = _computeNextNumberOfTiers(_numTiers);

Before we dive into this computation, it's worth exploring the claimPrize method. Whenever a prize is claimed there is a largestTierClaimed state variable that is set to the highest tier that has been claimed for (which can include the canary tier):

    if (largestTierClaimed < _tier) {
      largestTierClaimed = _tier;
    }

Now, the issue titled in this report exists because of how the next number of tiers is calculated in the _computeNextNumberOfTiers function:

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;
    }

    return _nextNumberOfTiers;
  }

For the sake of argument let's say the prize pool had 3 tiers, so the canary tier is tier 2. If there is at least 1 canary tier prize that has been claimed for the previous draw then largestTierClaimed = 2 and therefore _nextNumberOfTiers = 4. What's interesting here is that even if the check to expand the number of tiers fails, we still return _nextNumberOfTiers which has just been set to 4. So we're expanding the number of tiers even if the claim count isn't higher than the claim expansion thresholds.

I have made a tiny change to the existing test suite to demonstrate that the number of tiers will increase with just 1 canary prize claim. It can be executed with forge test -vvv --match-path test/PrizePool.t.sol:

diff --git a/test/PrizePool.t.sol b/test/PrizePool.t.sol
index 45d8606..36063eb 100644
--- a/test/PrizePool.t.sol
+++ b/test/PrizePool.t.sol
@@ -561,25 +561,12 @@ contract PrizePoolTest is Test {
   function testCloseAndOpenNextDraw_expandingTiers() public {
     contribute(1e18);
     closeDraw(1234);
-    mockTwab(address(this), address(this), 0);
-    claimPrize(address(this), 0, 0);
-    mockTwab(address(this), sender1, 1);
-    claimPrize(sender1, 1, 0);
-    mockTwab(address(this), sender2, 1);
-    claimPrize(sender2, 1, 0);
-    mockTwab(address(this), sender3, 1);
-    claimPrize(sender3, 1, 0);
-    mockTwab(address(this), sender4, 1);
-    claimPrize(sender4, 1, 0);
+
+    // Just 1 canary prize tier claim increases the number of tiers

     // canary tiers
     mockTwab(address(this), sender5, 2);
     claimPrize(sender5, 2, 0);
-    mockTwab(address(this), sender6, 2);
-    claimPrize(sender6, 2, 0);
-
-    vm.expectEmit();
-    emit DrawClosed(2, 245, 3, 4, 7997159090909503, UD34x4.wrap(7357954545454530000), prizePool.lastClosedDrawEndedAt());

     closeDraw(245);
     assertEq(prizePool.numberOfTiers(), 4);

Tools Used

Manual review + foundry

Recommended Mitigation Steps

The _computeNextNumberOfTiers logic should be updated to the below:

diff --git a/src/PrizePool.sol b/src/PrizePool.sol
index a42a27e..016124a 100644
--- a/src/PrizePool.sol
+++ b/src/PrizePool.sol
@@ -791,19 +791,22 @@ contract PrizePool is TieredLiquidityDistributor {
     }

     // 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;
+    if (_nextNumberOfTiers > _numTiers) {
+      if (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
+        return _numTiers + 1;
+      }
+      else {
+        return _numTiers;
+      }
     }

     return _nextNumberOfTiers;