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

12 stars 7 forks source link

Tiers can be mantained active to give unfair advantage to user through DoS #331

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L784 https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L173

Vulnerability details

Impact

Tiers can be maintained active by claiming a single prize of the last tier at a loss. The protocol will close tiers of the next draw based on the largest tier claimed of the draw being closed, which is registered during the claiming of the prizes.

This is a symptom that liquidity is being spread too thin on the last tier, and its no longer profitable for bots to claim prizes for users since their gas costs are over the claiming fees they earn.

The closing of a tier is meant to increase maxFee, which is the highest price a claiming fee can achieve in the reverse Dutch auction mechanism, which in the current draw isn't enough for the bot to be profitable.

The maxFee is computed based on the last active tier's prize size, and by lowering the amount of tiers, the liquidity is going to get concentrated in less tiers and in less prizes(since the number of prizes is 4^tierNumber), making the bots profitable again and incentivizing them to claim prizes for users.

During the draw where there are no incentives for bots to claim prizes, prizes won't get claimed, which means that if someone won a high prize from a low tier, they would have to manually claim it or the liquidity of that prize will continue onto the next draw, leaving the user without a possibility of claiming his prize once the draw has passed.

A malicious actor that has deposited liquidity in the vaults that contribute to the prize pool could take advantage of this mechanic. By having a bot that in this closing tier state claims his prizes when available, and claims a single prize from the highest tier, he is able to maintain the tier active, which will cause following draws to fall again into the state where bots don't claim prizes. This hugely decreases the probability of winning for users that don't have a claiming bot while giving the malicious actor an unfair advantage.

Proof of Concept

Draw 0: closeDraw has just been executed and the remaining liquidity from the previous draw with the newly added liquidity for this draw computes a maxFee that isn't enough to cover gas costs for bots to claim prizes, therefore the last tier is supposed to be closed. A malicious actor can claim a random single prize from the last tier at a loss to maintain the tier active, also claiming any prizes he might have won.

Draw 1: new liquidity is added to the system, which will make the last tier's prize size increase, and subsequently, the maxFee will increase. Depending on how much liquidity is added, maxFee will or won't be enough for bots to start claiming prizes since the last tier has accumulated the liquidity of two draws.

It could be the case that the added liquidity isn't enough and creates a draw state similar to draw 0, but to continue with the example let's assume that the accumulated liquidity of two draws computes a maxFee that is enough to incentivize bots to claim prizes.

Draw 1 behaves as expected, which means that the last tier's liquidity would get claimed, and the following draw would rely mostly on the newly added liquidity. "Mostly" since there will be a remanent part of liquidity from the prizes that haven't been won.

Draw 2: The situation of draw 0 repeats itself, the added liquidity on this draw is not concentrated enough to incentivize bots to claim prizes. The malicious actor claims a single prize at a loss again, to keep the tier active, also claiming any prizes he might have won.

This results in a situation where, depending on how much liquidity is added to each draw, many draws could result inactive, meaning that unless users claim their own prizes no one is going to claim them, while the malicious actor has an advantage since his bot will claim his prizes, apart from maintaining the highest tier active.

Tools Used

Manual review

Recommended Mitigation Steps

Changing the mechanism by which largestTierClaimed is set so that it's resistant to a single user performing a claim.

Assessed type

DoS

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by Picodes

Picodes commented 1 year ago

It seems to me that Medium severity is more appropriate here, as the profitability and the damage done by this attack scenario are hypothetical. It assumes that it is worth claiming prizes at a loss of say maxFeePortionOfPrize * prize to increase the attacker's chances of winning, and the damage is that users not claiming their prize (which requires multiple winners as the attacker is claiming one price) lose it.

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

asselstine commented 1 year ago

Fixed in https://github.com/GenerationSoftware/pt-v5-prize-pool/pull/17