Claimer:claimPrizes does a naive feePerClaim calculation

Summary

The claimPrizes external function wrongly assumes all caller-defined claims will be successful and does not handle the case were claims may fail - leading to wrong feePerClaim calculations.

Vulnerability Detail

When the claimPrizes external function calculates the feePerClaim uint96 variable, it accounts all claims and assumes every single one will be successful. This is not entirely true, as the claim internal function does claimPrize calls inside a try/catch code block:

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);
        }

What that means is if a claimPrize call fails, it will silently error without reversion and not add anything to the actualClaimCount variable - which is correct behavior. The issue with that is the feePerClaim variable becomes incorrect if any claim fails, as it was based on a fee per claim computation that assumed all claims would be successful:

feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, _countClaims(_winners, _prizeIndices), prizePool.claimCount()));

Impact

ClaimPrizes call will incorrectly price fees per claim in case any of the claims revert.

ClaimPrizes can have its fees per claim manipulated by a malicious party. The individual would have to call claimPrizes with certain amount of claims that intentionally revert in order to increase artificially increase the count of total claims and alter the computed fee per claim. As the try/catch block will not revert on wrong claims.

Code Snippet

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

Tool used

Manual Review

Recommendation

Make sure to calculate the fee per claim every time a claim is made, otherwise wrong fees will be returned as the current logic currently assumes all claims are necessarily successful. The loop should not happen at the claim internal function, but at the Claimer:claimprizes logic, in order to calculate the next feePerClaim if the call is successful. Here's a suggestion of refactor. First we remove the following lines of the claimPrizes function:

 if (!feeRecipientZeroAddress) {
      feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, _countClaims(_winners, _prizeIndices), prizePool.claimCount()));
      if (feePerClaim < _minFeePerClaim) {
        revert VrgdaClaimFeeBelowMin(_minFeePerClaim, feePerClaim);
      }
    }
    return feePerClaim * _claim(_vault, _tier, _winners, _prizeIndices, _feeRecipient, feePerClaim);

Then we substitute it with the following snippet:

 function claimPrizes(
    IClaimable _vault,
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    address _feeRecipient,
    uint256 _minFeePerClaim
  ) external returns (uint256 totalFees) {
...
uint256 acumulatedFee = 0;
for (uint256 w = 0; w < _winners.length; w++) {
      prizeIndicesLength = _prizeIndices[w].length;

      for (uint256 p = 0; p < prizeIndicesLength; p++) {
      // @audit the prizePool's claim count is updated when
      // @audit the claimPrize call is successful. By calculating
      // @audit it claim by claim, we ensure its value is correct.
      feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, 1, prizePool.claimCount()));
        try
            _vault.claimPrize(_winners[w], _tier, _prizeIndices[w][p], _feePerClaim, _feeRecipient)
            returns (uint256 /* prizeSize */) {
          acumulatedFee += feePerClaim;
        } catch (bytes memory reason) {
          emit ClaimError(_vault, _tier, _winners[w], _prizeIndices[w][p], reason);
        }
...
return acumulatedFee;
}
...

Duplicate of #124

sherlock-audit / 2024-05-pooltogether-judging

jovi - Claimer:claimPrizes does a naive feePerClaim calculation #86