sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

MiloTruck - `try/catch` in `Claimer._claim()` allows users to steal gas from claimer bots #37

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

MiloTruck

medium

try/catch in Claimer._claim() allows users to steal gas from claimer bots

Summary

Claimer.claimPrizes() does not revert when an attempt to claim a prize fails, making it possible for claimer bots to pay gas but not receive any fees in return.

Vulnerability Detail

When claimer bots call Claimer.claimPrizes(), they provide a _minFeePerClaim parameter, which determines the minimum fee they should receive for each successful claim:

Claimer.sol#L113-L116

      feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, _countClaims(_winners, _prizeIndices), prizePool.claimCount()));
      if (feePerClaim < _minFeePerClaim) {
        revert VrgdaClaimFeeBelowMin(_minFeePerClaim, feePerClaim);
      }

If the fee received per claim is less than _minFeePerClaim, Claimer.claimPrizes() reverts to ensure that claimer bots never lose funds from gas costs.

However, this is not sufficient to protect against the scenarios where calling PrizeVault.claimPrize() fails. In Claimer._claim(), calling PrizeVault.claimPrize() is wrapped in a try/catch:

Claimer.sol#L161-L167

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

This means that Claimer._claim() will not revert when attempting to claim a prize fails. The issue is that calling PrizeVault.claimPrize() costs gas even when it reverts. As such, Claimer bots are not reimbursed for gas costs due to failed calls to PrizeVault.claimPrize().

The logic of PrizeVault.claimPrize() is as shown:

Claimable.sol#L86-L118

        if (_hooks[_winner].useBeforeClaimPrize) {
            (_prizeRecipient, _hookData) = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }(
                // ...
            );
        } else {
            _prizeRecipient = _winner;
        }

        if (_prizeRecipient == address(0)) revert ClaimRecipientZeroAddress();

        uint256 _prizeTotal = prizePool.claimPrize(
            // ...
        );

        if (_hooks[_winner].useAfterClaimPrize) {
            _hooks[_winner].implementation.afterClaimPrize{ gas: HOOK_GAS }(
                // ...
            );
        }

As seen from above, the function could revert for a number of reasons, such as:

To maximize the claimer bot's loss from calling PrizeVault.claimPrize(), a winner could do the following:

Assume a claimer bot calls Claimer.claimPrizes() to claim a prize for such a winner:

As seen from above, the claimer bot pays for more than 300,000 gas, but receives no fees in return.

Note that on Blast L2 (which is one of the supported chains in the contest's README), contracts can claim a portion of the gas that was spent. This incentivizes winners to perform the attack described above, since they can claim part of the gas spent in the beforeClaimPrize and afterClaimPrize hook.

Impact

Since claimer bots pay gas but do not receive any fees in return when a call to PrizeVault.claimPrize() fails, they can lose funds. On Blast L2, winners can steal gas from claimer bots by intentionally spending gas in custom hooks, which maximizes the loss for claimer bots.

Code Snippet

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

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-claimer/src/Claimer.sol#L161-L167

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L86-L118

Tool used

Manual Review

Recommendation

Whenever _vault.claimPrize() is called in Claimer.claim(), PrizePool.claimPrize() must be executed successfully to guarantee that the claimer bot will receive fees in return for paying gas. This can be achieved by doing the following:

  1. In Claimer.claim(), check PrizePool.wasClaimed() before calling _vault.claimPrize(). This ensures gas is never spent to try to claim a prize that was already claimed (eg. another bot front-runs and claims the prize).
  2. In _vault.claimPrize(), wrap the beforeClaimPrize and afterClaimPrize hooks in a try/catch. This ensures the _winner address cannot intentionally revert the call.

Both of these mitigations will always ensure that _vault.claimPrize() and PrizePool.claimPrize() will never revert when called, provided there is sufficient liquidity in the pool to payout prizes.

Duplicate of #163