search

sherlock-audit / 2024-05-pooltogether-judging

11 stars 6 forks source link

evmboi32 - The number of tiers is incorrectly used when claiming rewards. #76

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

evmboi32

medium

The number of tiers is incorrectly used when claiming rewards.

Summary

The number of tiers is incorrectly used when claiming rewards.

Vulnerability Detail

When a draw is closed it can be awarded. It is done so by calling the awardDraw function. When the function is called it will update the number of tiers for the next draw.

The number of tiers can change each draw such that the number of tiers is increased or decreased by 1, depending on how many tiers have been claimed.

After the draw is awarded the prizes can be claimed for the awarded draw (keep in mind that the number of tiers has already been changed when awarding a draw). When claiming prizes the winner is checked by invoking the isWinner function.

The isWinner function will check if the tier that a user wants to claim a prize for is valid

The issue is that it compares the tier to the new already updated number of tiers meant for the next draw. Instead, it should use the number of tiers that were present in the awarded draw. This can cause the last tier not to be claimed if the number of tiers decreases or will let users claim a tier that should not be claimed in the given draw if the number of tiers increases.

Imagine the following scenario:

Impact

Incorrect use of a number of tiers will prevent all tiers from being claimed.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-prize-pool/src/PrizePool.sol#L476-L480

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-prize-pool/src/PrizePool.sol#L472

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-prize-pool/src/abstract/TieredLiquidityDistributor.sol#L248

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-prize-pool/src/PrizePool.sol#L547

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-prize-pool/src/PrizePool.sol#L1009-L1011

Tool used

Manual Review

Recommendation

Use the number of tiers that were awarded when claiming prizes.

sherlock-admin4 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

natspec of numberOfTiers : The number of tiers for the >>last awarded draw<<

nevillehuang commented 4 months ago

request poc

sherlock-admin4 commented 4 months ago

PoC requested from @evmboi32

Requests remaining: 5

evmboi32 commented 4 months ago

Add this to PrizePool.t.sol and run with forge test --match-path ./test/PrizePool.t.sol -vvv


function testCannotClaimIfGrow() public {
  params.numberOfTiers = 4;
  prizePool = newPrizePool();

  contribute(100e18);
  // claimCount 0 so no increase in tiers
  awardDraw(1234);
  _claimAllPrizes(prizePool.numberOfTiers());

  // Now that we claimed all the prizes, the number of tiers should increase by 1
  assertEq(prizePool.estimateNextNumberOfTiers(), 5);
  contribute(100e18);
  // Number of tiers is still 4, this means the pool should award 4 tiers
  assertEq(prizePool.numberOfTiers(), 4);

  awardDraw(1234);
  // awardDraw will increase the number of tiers by 1
  assertEq(prizePool.numberOfTiers(), 5);

  // 4 tiers are awarded but 5 tiers can be claimed
  _claimAllPrizes(prizePool.numberOfTiers());
  assertEq(prizePool.estimateNextNumberOfTiers(), 6);
  }```
trmid commented 4 months ago

The numberOfTiers indicates the number of tiers for the last awarded draw. This means it is updated every time awardDraw is called. This is accurately illustrated in the example code given and is expected behaviour: