code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The protocol utilizes Gradual Dutch Auctions to determine the price paid to claimants. In the original paper, the number of assets owned is known in advance, enabling the auction to precisely adjust prices based on demand. However, the number of prices in the PoolTogether pool is stochastic, which can lead to pricing discrepancies when the actual number deviates significantly from the expected number.

Proof of Concept

The formula to derive the reward the claimer receives for getting an auction is

auction prize = p * e^(t - n / r)

, where t is the time passed, n is the number of claims and r is the expected number of claims per second. When the number of claims is right on target then t = n / r, so we sell at the target price p. When we have claimed a larger number of units n then expected, then t << n / r, so the prize will significantly drop below the target prize. The auction price drops below the gas fees, so many prizes will remain unclaimed.

Tools Used

Manual review, VS Code

Recommended Mitigation Steps

With a large number of tiers, users and vault the probability of a large deviation is small.

For a small number of tiers, we should calculate the maximum number of prizes that will appear with a high probability (i.e. a number of draws that is not exceeded in 99.9% of the cases). We call this number the limitPrizeCount. We use the limit to estimate the maximum number of units drawn per second instead of the average number of units drawn:

SD59x18 perTimeUnit = LinearVRGDALib.getPerTimeUnit(
-    prizePool.estimatedPrizeCount(),
+    prizePool.limitPrizeCount()
     timeToReachMaxFee
    );

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

asselstine commented 1 year ago

Our simulator mitigates against this by having the VRGDA on a much more aggressive timeline; the duration over which prizes are auctioned is half the draw period. However, it's worth doing some additional statistical analysis to better qualify this choice.

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor confirmed

c4-judge commented 11 months ago

Picodes changed the severity to QA (Quality Assurance)

Picodes commented 11 months ago

Downgrading to Low as despite this being a possibility, it should remain exceptional with a proper configuration, and if n >> r there may be other issues like the reserves not being enough