A Malicious Prize Winner Can Steal the ClaimReward From the Claimer Using the BeforeClaimPrize Hook

[c4-bot-1]

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

[raymondfam]

Claimable::claimPrize has the visibility of onlyClaimer denying the winner's hook reentrancy.

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c

[InfectedIsm]

Hi @hansfriese, I think there was a misunderstanding here and I'd really value a second look from you.

In my submission, I've demonstrated how a malicious winner could exploit the beforeClaimPrize hook to get the claim reward for himself, basically stealing it from the rightful claimer.

@raymondfam in his analysis mentioned the onlyClaimer modifier as a denial of the reentrancy, which is in fact not correct.
In my description, for simplicity's sake, I decided to not mention the existing Claimer contract (the one allowed to call the onlyClaimer functions), that must be used to call the Claimable.claimPrize function (which is public and not role protected), but obviously the hook will have to call the Claimer.claimPrizes function, which itself calls the claimPrize function L188 of Claimer.
I ask you to read again the "Proof of Concept" section which shows how the hook will get the reward, while the claimer will simply return early and get 0 reward, with no revert

It has also be shown by another researcher (@Al-Qa-qa) here that the gas limit implemented as a defensive mechanism wouldn't prevent such exploit. And He also provided a runnable PoC for that : #345

Regarding the impact, I still think it should be categorized a Medium as there is a clear theft of value from a malicious user to a honest user + endangering/getting around the incentive mechanism as this could easily be implemented by many users, making the claiming mechanism less financially interesting.

Finally, if this finding gets re-evaluated to satisfactory, I would like to point that the submissions I will list afterward either do not show the impact, or show something different and shouldn't be grouped under #345 :

• no impact shown: HookManager.sol allows users to set arbitrary hooks #324, QA Report #250,
• different impact and root cause: Prizes cannot be claimed with Hooks in Arbitrum due to low gas limit #166, Potential DoS in claimPrize. #210, Winners call on hook may exceed the hardcoded gas limit #276, QA Report #230, QA Report #48, Insufficient Gas Limit for External Calls Leads to Potential Denial of Service (DoS) Attack (in Claimable.sol) #40, Winners can frontrun the claimer with malicious hooks #78
• false positive impact: Using hooks can block everyone from claiming prizes #189, Lack of Access Control in setHooks Function #106

And thanks a lot for your time!

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese marked the issue as not a duplicate

[hansfriese]

Thank you for your detailed comment.
I acknowledge you've shown a valid attack path and mitigation. However, I see the original report overlooks the Claimer contract as the starting point for a reentrancy attack.
As you've said, I will consider this an omission for simplicity and mark it as a duplicate of #345. Btw I believe it's fair to apply a partial credit.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #345

[c4-judge]

hansfriese marked the issue as partial-75

[InfectedIsm]

Hi @hansfriese,thank you very much for taking the time to re-evaluate my submission.
I just want to highlight that the issue is still marked as "insufficient quality report", doesn't that mean it's discarded for reward?

Also, as you confirmed, I clearly shown the vulnerability and exploit path, and the onlyClaimer modifier was as far as I understand never set to protect against reentrancy, but to force participants to go through the Claimer contract as the claimReward is calculated base on a Dutch auction mechanism in Claimer.
That is why I haven't considered it important to mention, as it does not add any constraints to the exploit.

Regards

[hansfriese]

It won't affect the reward calculation.