code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L598-L600 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L542-L545

Vulnerability details

Impact

The first winner who claims a prize in a tier gets more tokens than subsequent claimants, even though the prize is expected to be split equally. Since the order of winners during claiming can be arbitrary specified by the claimer, winners can be deliberately sorted in a way to benefit some of them.

Proof of Concept

In the prize pool, each tier has a fixed number of prizes: the accumulated liquidity of a tier is split into a fixed number of equal prizes so that multiple winners could win prizes from one tier. However, after the first winner has claimed their prize, the remaining liquidity of the tier, while being reduced after the first claim, is again split into the same amount of prizes. Thus, after the first claim, each prize of a tier becomes smaller, and the subsequent claimants receive less tokens than the first winner. The later you claim your prize, the smaller its size is.

The PrizePool.claimPrize function is used to claim prizes from the prize pool. The prize size is determined in the very beginning of the function, by loading the info about the tier–it's stored in tierLiquidity.prizeSize. Looking at _getTier, we can see that the prize size is computed in the _computePrizeSize function: the function computes the liquidity of the tier by multiplying the amount of prize tokens per share by the amount of shares of the tier; the liquidity is then divided by _fractionalPrizeCount, which is set earlier to the number of prizes of the tier.

When a prize is claimed, the amount of liquidity equal to the size of the prize is removed from the tier (PrizePool.sol#L451): the _consumeLiquidity function the prize tokens per share amount of the tier (TieredLiquidityDistributor.sol#L542-L545), which reduces the liquidity of the tier. However, the number of prizes remains unchanged (in fact, the number of prizes of a tier is pre-defined and cannot be changed). Thus, for subsequent winners, the prize will be split into smaller amounts.

Consider this example scenario:

Suppose that a tier has 4 prizes, the prizeTokenPerShare of the tier is 0, the global prizeTokenPerShare is 1, and the tier has 100 shares. The liquidity of the tier is thus: (1 - 0) * 100 = 100 (as per the _getTierRemainingLiquidity function).
First winner claims their prize. The size of the prize is: (1 - 0) * 100 / 4 = 25 (as per the _computePrizeSize function). The _consumeLiquidity function increases the prizeTokenPerShare of the tier by: 25 / 100 = 0.25. The prizeTokenPerShare of the tier after the claim is 0.25.
Next winner claims their prize from the same tier, the prize size is: (1 - 0.25) * 100 / 4 = 18.75. Notice that the remaining liquidity is still divided by 4, even though one of the prizes has already been claimed. Consuming the prize increases the prizeTokenPerShare of the tier by: 18.75 / 100 = 0.1875, bringing it to 0.4375.
Next winner claims their prize from the same tier, the prize size is even smaller: (1 - 0.4375) * 100 / 4 = 14.0625. The final winner's prize size will be even smaller.

Tools Used

Manual review

Recommended Mitigation Steps

It seems like the only solution is to track the number of claimed prizes per tier and per draw. The number can then be subtracted from the _fractionalPrizeCount argument value of the _computePrizeSize function, thus reducing the number of shares the liquidity of a tier is split into.