PoolTogether - Infect3d's results

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L76-L119

Vulnerability details

Impact

Claimer will lose part of their profit. If too many winners set such a hook, this can disencentivize claimers to do their job.

Vulnerability details

Pool Together prize distribution is based on incentivization of 3rd parties to activate its functionality, rather than depending on a governance or centralized actor. For example in this case, everytime a draw to award prizes is closed, any 3rd party can call the claimable::claimPrize function to claim the winner's prize on his behalf. By doing so, the prize will be distributed to the winner, minus a small fee rewarded to the claimer.

As said above, to claim prizes 3rd party must call the Claimable::claimPrize:

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L76-L119

File: pt-v5-vault\src\abstract\Claimable.sol
076:     function claimPrize(
077:         address _winner,
078:         uint8 _tier,
079:         uint32 _prizeIndex,
080:         uint96 _reward,
081:         address _rewardRecipient
082:     ) external onlyClaimer returns (uint256) {
083:         address recipient;
084: 
085:         if (_hooks[_winner].useBeforeClaimPrize) {
086:             recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }(
087:                 _winner,
088:                 _tier,
089:                 _prizeIndex,
090:                 _reward,
091:                 _rewardRecipient
092:             );
093:         } else {
094:             recipient = _winner;
095:         }
096: 
097:         if (recipient == address(0)) revert ClaimRecipientZeroAddress();
098: 
099:         uint256 prizeTotal = prizePool.claimPrize(
100:             _winner,
101:             _tier,
102:             _prizeIndex,
103:             recipient,
104:             _reward,
105:             _rewardRecipient
106:         );

And as we can see here, before the prize is claimed (L99), a hook is called (L86). We can observe that this hook receives the exact same parameters as the claimPrize call: _winner, _tier, _prizeIndex, reward and rewardRecipient.

The vulnerability lies in the fact that nothing prevent a malicious winner to configure a hook that will claim the prize before the original claimer. By developping a hook that reuses these parameters to call claimable::claimPrize, but replacing the _rewardRecipient address with one of its own, the winner can then receive its legitimate prize + the claimer reward. And as a bonus, we can also note that the winner will not even have to pay the gas fee for the call, as it uses the claimer call to execute this action.

Proof of Concept

By claiming right before the original claimer thanks to the hook, the winner sets the _claimedPrizes[msg.sender][_winner][lastAwardedDrawId_][_tier][_prizeIndex]) entry to true (L516), after that the award will be credited to him (L525) Then, when the claimer will enter PrizePool::claimPrize himself after the hook has been executed, the entry will already be set to true, making if statement at L512 pass and the call return early.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4380f1ab65e39109ac6a36f8935137a2ef685860/src/PrizePool.sol#L512-L516

File: pt-v5-vault\lib\pt-v5-prize-pool\src\PrizePool.sol
476:   function claimPrize(
477:     address _winner,
478:     uint8 _tier,
479:     uint32 _prizeIndex,
480:     address _prizeRecipient,
481:     uint96 _claimReward,
482:     address _claimRewardRecipient
483:   ) external returns (uint256) {
484:     
...: /* removed some code for readability */
511: 
512:     if (_claimedPrizes[msg.sender][_winner][lastAwardedDrawId_][_tier][_prizeIndex]) { 
513:       return 0; //<@ original claimer will return here because of the hook that set it first to true L516
514:     }
515: 
516:     _claimedPrizes[msg.sender][_winner][lastAwardedDrawId_][_tier][_prizeIndex] = true;
517: 
518:     // `amount` is a snapshot of the reserve before consuming liquidity
519:     _consumeLiquidity(tierLiquidity, _tier, tierLiquidity.prizeSize);
520: 
521:     // `amount` is now the payout amount
522:     uint256 amount;
523:     if (_claimReward != 0) {
524:       emit IncreaseClaimRewards(_claimRewardRecipient, _claimReward);
525:       _rewards[_claimRewardRecipient] += _claimReward;
526: 
527:       unchecked {
528:         amount = tierLiquidity.prizeSize - _claimReward;
529:       }
530:     } else {
531:       amount = tierLiquidity.prizeSize;
532:     }
533: 
534:     // co-locate to save gas
535:     claimCount++;
536:     _totalWithdrawn = SafeCast.toUint128(_totalWithdrawn + amount);
537:     _totalRewardsToBeClaimed = SafeCast.toUint104(_totalRewardsToBeClaimed + _claimReward);
538: 
...: /* removed the emited event */
550: 
551:     prizeToken.safeTransfer(_prizeRecipient, amount);
552: 
553:     return tierLiquidity.prizeSize;
554:   }

Tools Used

Manual review

Recommended Mitigation Steps

Hopefully, the solution is pretty easy to set-up: just add a nonReentrant modifier (can use the ReentrancyGuard lib from OZ) to the Claimable::claimPrize This way, the hook will not be allowed to reenter to function.

Assessed type

Reentrancy