cu5t0mPe0 - The claimer's fee will be stolen by the winner

[sherlock-admin4]

cu5t0mPe0

medium

The claimer's fee will be stolen by the winner

Summary

The user calls claimPrizes to collect prizes for the winner, earning some rewards in the process. However, during this process, the winner can steal the fees collected by the user without paying any gas fees.

Vulnerability Detail

The user calls claimPrizes to collect prizes for the winner, thereby earning a reward fee.

Throughout the process, the winner can set hooks before and after calling prizePool.claimPrize. The winner can set an afterClaimPrize hook and then call claimer.claimPrizes again within this afterClaimPrize hook, thereby earning reward fees without using any gas. As a result, the user can only collect the reward fee for one winner, while the remaining reward fees for other winners will all be collected by the first winner.

Consider the following scenario:

1. User A calls claimer.claimPrizes to claim prizes, setting the winners as [B, C, D, E, F] (B, C, D, E, F are all winners).
2. User B notices in the mempool that User A is about to call claimer.claimPrizes to claim prizes for others. User B then calls setHooks to set the afterClaimPrize function, which implements a logic to loop call claimer.claimPrizes with winners set as [C, D, E, F].
3. User A's transaction is executed, but since User B has already claimed the rewards for C, D, E, and F, User A's attempts to claim prizes for C, D, E, and F will revert. However, due to the try-catch, User A's transaction will continue executing. In the end, User A will only receive the reward fee for User B.

This is essentially the same attack method as the previous PoolTogether vulnerability. The last audit did not completely fix it. The difference this time is that the hook uses afterClaimPrize.

https://code4rena.com/reports/2024-03-pooltogether#m-01-the-winner-can-steal-claimer-fees-and-force-him-to-pay-for-the-gas

Impact

The user will lose the reward fee

Code Snippet

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

Tool used

Manual Review

Recommendation

If the hook's purpose is for managing whitelists and blacklists, PoolTogether can create a template contract that users can manage. This way, the hook can only be set to the template contract and cannot perform additional operations.

If users are allowed to perform any operation, there is no good solution to this problem. The only options are to limit the gas, use reentrancy locks, or remove the after hook altogether.

[sherlock-admin2]

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

infect3d commented:

if the attacker can front-run one prize then he is better off front-running the whole claimPrizes call and earn even more reward

[trmid]

The risks of front-running claims is a known issue from previous audits: https://code4rena.com/reports/2023-07-pooltogether#m-24-claimerclaimprizes-can-be-front-runned-in-order-to-make-losses-for-the-claim-bot

Although the method described here is slightly more complex, the end result is still very similar to the more straightforward approach of frontrunning the entire tx.

It is also worth noting that the prize hooks have a gas limit of 150k gas, which would cause this strategy to fail when trying to frontrun a large number of prizes.

[nevillehuang]

I believe medium severity is appropriate here, this issue wasn't highlighted as a known issue/risk, and watsons has highlighted another vector that allows stealing of claimer fees

[InfectedIsm]

Escalate

The reason I think it shouldn't be valid is because its a front-run, and any profitable action can be front-run and nothing can be done against this. We can also show that a simple copy-paste front-run of the tx is always a better solution than that one.

Here the attacker need (1) to detect the user A call (2) front-run it to update its hooks

But if the user is able to do do this, then why wouldn't it simply copy-paste the whole user A call, and put his own claiming call before user A? He would get even more reward and more disturbance.

Following the described attack path of this submission, the user will only be able to claim 2 prizes (before and after hooks), which will be less beneficial than front-running the whole claiming array with a standard claim call.

There is no way to defend against a front-run unless using the proposed remediation (using whitelist/blacklist), but this totally goes against the way the protocol is designed as a permissionnes protocol

[sherlock-admin3]

Escalate

The reason I think it shouldn't be valid is because its a front-run, and any profitable action can be front-run and nothing can be done against this. We can also show that a simple copy-paste front-run of the tx is always a better solution than that one.

Here the attacker need (1) to detect the user A call (2) front-run it to update its hooks

But if the user is able to do do this, then why wouldn't it simply copy-paste the whole user A call, and put his own claiming call before user A? He would get even more reward and more disturbance.

Following the described attack path of this submission, the user will only be able to claim 2 prizes (before and after hooks), which will be less beneficial than front-running the whole claiming array with a standard claim call.

There is no way to defend against a front-run unless using the proposed remediation (using whitelist/blacklist), but this totally goes against the way the protocol is designed as a permissionnes protocol

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0jovi0]

Escalate

The reason I think it shouldn't be valid is because its a front-run, and any profitable action can be front-run and nothing can be done against this. We can also show that a simple copy-paste front-run of the tx is always a better solution than that one.

Here the attacker need (1) to detect the user A call (2) front-run it to update its hooks

But if the user is able to do do this, then why wouldn't it simply copy-paste the whole user A call, and put his own claiming call before user A? He would get even more reward and more disturbance.

Following the described attack path of this submission, the user will only be able to claim 2 prizes (before and after hooks), which will be less beneficial than front-running the whole claiming array with a standard claim call.

There is no way to defend against a front-run unless using the proposed remediation (using whitelist/blacklist), but this totally goes against the way the protocol is designed as a permissionnes protocol

The other dupes are not about the front-run but about the hooks allowing gas or prize fees to be stolen. Issue #82 and #161 describe a beforeClaimPrize hook that effectively steals prize fees without any mempool manipulation whatsoever.

[0x73696d616f]

It is not true that it would always be better to frontrun the tx and execute the same transaction because there is a bigger upfront cost and there is the risk that some claims have been made and a lot of gas is wasted. This allows gaming the claiming mechanism and could be fixed by simply adding a reentrancy lock. The bot would take a loss from this. The bot gives 300k free gas to the user per prize claimed. As the bots will collect a lot of prizes for each user, the gas given to users is 300k*n, where n is the number of prizes claimed. Thus, a simple frontrun to change the logic of a hook, will cost something like 30k gas, gives the user 300k * n gas to claim prizes, at no risk, which is obviously problematic.

[0x73696d616f]

Also, is it possible to trick the simulation of the transaction so no frontrunning is needed? I am really curious about this. I was thinking about using block.timestamp, but there is a chance that the simulation fails.

[WangSecurity]

I agree with the comments above, the fact that there may be a "better" attack path (which is explained not to be better in the comment above) doesn't mean this attack path won't be taken or it's negligible. Moreover, if the mitigation is not the best, it doesn't mean the issue is invalid.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• InfectedIsm: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-claimer/pull/32

[10xhash]

Fixed. Now re-entrancy guard is added to to the Claimer contract which disallows claiming prizes by reentering through the same claimer contract. If in future vaults are launched with a different claimer contract and bots attempt to claim prizes for both these vaults in the same tx, then the same issue would arise again

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.