0x73696d616f - `Claimer::claimPrizes()` fee is incorrect and incurs very signficant costs whenever it is frontrunned

[sherlock-admin3]

0x73696d616f

high

Claimer::claimPrizes() fee is incorrect and incurs very signficant costs whenever it is frontrunned

Summary

Claimer::claimPrizes() incorrectly computes the fee based on the number of prizes to claim, not considering if any of the indices were already claimed and calls Vault::claimPrize(), which spends up to a little more than 150_000 gas per claim that receives no fee.

Vulnerability Detail

Claimer::claimPrizes() computes the fee based on the auction mechanism and then divides the total fee by the number of prize claims. However, it fails to account for indices that were already claimed, dividing the fee by all indices. This will severely cut fees for claimers that were frontrunned in any amount of indices of prizes claimed. At the end of Claimer::_computeFeePerClaim(), it can be seen how the fee is divided by _claimCount:

function _computeFeePerClaim(
  uint8 _tier,
  uint256 _claimCount,
  uint256 _claimedCount
) internal view returns (uint256) {
  ...
  return fee / _claimCount;
}

And _claimCount is _countClaims(_winners, _prizeIndices), which sums all prizes, without considering that some of them were already claimed.

Additionally, by not checking the prizes that were already claimed, it still tries to claim in the try/catch statement in Claimer::_claim()

for (uint256 w = 0; w < _winners.length; w++) {
  prizeIndicesLength = _prizeIndices[w].length;
  for (uint256 p = 0; p < prizeIndicesLength; p++) {
    try
      _vault.claimPrize(_winners[w], _tier, _prizeIndices[w][p], _feePerClaim, _feeRecipient)
    returns (uint256 /* prizeSize */) {
      actualClaimCount++;
    } catch (bytes memory reason) {
      emit ClaimError(_vault, _tier, _winners[w], _prizeIndices[w][p], reason);
    }
  }
}

It calls Vault.claimPrize() regardless, which may trigger a hook in Claimable, if _hooks[_winner].useBeforeClaimPrize is set to true for this winner, and may spend up to 150k gas. Thus, if there are 100 winners whose prize has been claimed, it will spend approximately 15_000_000 total gas, which on Ethereum for example is very expensive (15_000_000 * 20*10^(-9) == 0.3 ETH). If it was checked beforehand if the prize has been already claimed, it would cost just a storage read 100 times by calling PrizePool::wasClaimed(), something like 2600*100 = 260000 gas, or 260_000 * 20*10^(-9) = 0.00026 ETH, 1150 times less.

Impact

Tremendous fee and gas costs if claimer bots are frontrunned.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-claimer/src/Claimer.sol#L113 https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-claimer/src/Claimer.sol#L162

Tool used

Manual Review

Vscode

Recommendation

For each index, fetch PrizePool::wasClaimed() and use the prizes that were not claimed to calculate the claim count. Additionally, skip calling Vault::claimPrize() for prizes that were claimed.

Duplicate of #124