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

12 stars 7 forks source link

Tier odds in TieredLiquidityDistributor are incorrect #352

Open code423n4 opened 11 months ago

code423n4 commented 11 months ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L83-L200

Vulnerability details

Impact

The following points are stated in the docs:

  1. The highest standard prize tier is the most common prize: occurring every single draw.
  2. The canary tier has odds of occurring daily (as if it were the highest tier)

Therefore, both the highest standard tier and the canary tier should have odds of 1, however at the moment only the canary tier has odds of 1. The result is that the protocol is not distributing prizes as intended.

Proof of Concept

The issue with the odds calculations can be seen in the TieredLiquidityDistributor.sol contract. Below is a short snippet for the tier odds when there are 3 tiers:

  SD59x18 internal constant TIER_ODDS_0_3 = SD59x18.wrap(2739726027397260);
  SD59x18 internal constant TIER_ODDS_1_3 = SD59x18.wrap(52342392259021369);
  SD59x18 internal constant TIER_ODDS_2_3 = SD59x18.wrap(1000000000000000000);

The canary tier odds are 1 as intended, however the highest normal prize tier odds are incorrect. If the tier odds were being calculated correctly, we should see the last two odds for each number of tiers being 1 (i.e. 1000000000000000000).

Tools Used

Manual review

Recommended Mitigation Steps

Since the TierCalculationLib.getTierOdds only seems to be used by the GenerateConstants script, I suggest modifying the script used to generate the constants that are placed in TieredLiquidityDistributor.sol:

diff --git a/script/generateConstants.s.sol b/script/generateConstants.s.sol
index fe1d4ed..fb9f90a 100644
--- a/script/generateConstants.s.sol
+++ b/script/generateConstants.s.sol
@@ -38,16 +38,24 @@ contract GenerateConstants is Script {
     MAX_TIERS = 15;
     // Precompute the odds for each tier
     for (uint8 numTiers = MIN_TIERS; numTiers <= MAX_TIERS; numTiers++) {
-      for (uint8 tier = 0; tier < numTiers; tier++) {
+      for (uint8 tier = 0; tier < numTiers - 1; tier++) {
         console.log(
           "SD59x18 internal constant TIER_ODDS_%d_%d = SD59x18.wrap(%d);",
           uint256(tier),
           uint256(numTiers),
           uint256(
-            SD59x18.unwrap(TierCalculationLib.getTierOdds(tier, numTiers, GRAND_PRIZE_PERIOD_DRAWS))
+            SD59x18.unwrap(TierCalculationLib.getTierOdds(tier, numTiers - 1, GRAND_PRIZE_PERIOD_DRAWS))
           )
         );
       }
+      console.log(
+          "SD59x18 internal constant TIER_ODDS_%d_%d = SD59x18.wrap(%d);",
+          uint256(numTiers - 1),
+          uint256(numTiers),
+          uint256(
+            SD59x18.unwrap(TierCalculationLib.getTierOdds(numTiers - 1, numTiers, GRAND_PRIZE_PERIOD_DRAWS))
+          )
+        );
     }
   }
 }

Assessed type

Math

c4-sponsor commented 11 months ago

asselstine marked the issue as sponsor confirmed

c4-judge commented 11 months ago

Picodes marked the issue as satisfactory

asselstine commented 10 months ago

Fixed here https://github.com/GenerationSoftware/pt-v5-prize-pool/pull/24