Lines of code

Vulnerability details

Original Issue

M-24 - Claimer.claimPrizes can be frontrunned in order to make losses for the claim bot

Details

In the previous implementation, when claiming prizes, if a winner had already claimed its prize, any new tx attempting to claim the prize for that winner was reverted.

That logic could've caused Claimer bots to get their tx to claim the prizes for all the winners to be frontrunned by a malicious actor or another bot wanting to get all the claim fees for himself.

Mitigation

The fix for this issue was to update the logic in the PrizePool:claimPrize() to instead of reverting the tx if the winner has already claimed its prize, now, it will return a 0, and in the Claimer:_claim(), when claiming prizes from the vault, if the returned value is a 0, a counter will be increased to keep track of all the actual claims that were made in that tx, and finally, the Claimer:_claim() will return the total number of claims that were made, and that number will be multiplied by the feePerClaim value, which that will compute the total amount of fees that the bot collected.

// @audit-info => PrizePool:claimPrize() function
  function claimPrize(
    ...
  ) external returns (uint256) {
    ...
    //@audit-ok => Instead of reverting the tx, now it returns a 0 to indicate that this winner has already claimed its prize
    if (claimedPrizes[msg.sender][_winner][lastClosedDrawId][_tier][_prizeIndex]) {
      return 0;
    }
    ...
  }

// @audit-info => Claimer:_claim() function
  function _claim(
    ...
  ) internal returns (uint256) {
    uint256 actualClaimCount;
    uint256 winnersLength = _winners.length;
    for (uint256 w = 0; w < winnersLength; w++) {
      uint256 prizeIndicesLength = _prizeIndices[w].length;
      for (uint256 p = 0; p < prizeIndicesLength; p++) {
        //@audit-ok => If the prize was claimed, increments the counter of total claims
        if (0 != _vault.claimPrize(
          _winners[w],
          _tier,
          _prizeIndices[w][p],
          _feePerClaim,
          _feeRecipient
        )) {
          actualClaimCount++;
        } else {
          emit AlreadyClaimed(_winners[w], _tier, _prizeIndices[w][p]);
        }
      }
    }
    return actualClaimCount;
  }

// @audit-info => Claimer:claimPrizes() function
  function claimPrizes(
    ...
  ) external returns (uint256 totalFees) {

    ...

    //@audit-ok => Computes the total amount of fees that were claimed based on the total number of prizes that were successfully claimed
    return feePerClaim * _claim(
      _vault,
      _tier,
      _winners,
      _prizeIndices,
      _feeRecipient,
      feePerClaim
    );
  }

Conclusion

The implemented mitigation solves the original issue.

code-423n4 / 2023-08-pooltogether-mitigation-findings