hash - Users can setup hooks to control the expannsion of tiers

[sherlock-admin3]

hash

medium

Users can setup hooks to control the expannsion of tiers

Summary

Reentrancy allows to not claim rewards and update tiers

Vulnerability Detail

The fix for the issue still allows user's claim the rewards. But as mitigation, the contract will revert. But this still allows the user's to specifically revert on just the last canary tiers in order to always prevent the expansion of the tiers

Impact

User's have control over the expansion of tiers

Code Snippet

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

Tool used

Manual Review

Recommendation

[sherlock-admin4]

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

infect3d commented:

expected design

[nevillehuang]

request poc

[sherlock-admin4]

PoC requested from @10xhash

Requests remaining: 8

[10xhash]

The issue that I wanted to point out is that since a user is not going to be benefited if a canary tier prize is claimed (the claiming bot would keep the fee as the entire prize amount. The canary prize claiming is kept inorder to determine if the number of tiers should be increased / decreased) a user can revert in the hook if the tier is a canary tier. If it is misunderstood as a single user can disallow the entire tier expansion, it is not so

diff --git a/pt-v5-vault/test/unit/Claimable.t.sol b/pt-v5-vault/test/unit/Claimable.t.sol
index dfbb351..44b6568 100644
--- a/pt-v5-vault/test/unit/Claimable.t.sol
+++ b/pt-v5-vault/test/unit/Claimable.t.sol
@@ -131,6 +131,22 @@ contract ClaimableTest is Test, IPrizeHooks {
         assertEq(logs.length, 1);
     }

+
+
+    function testHash_RevertIfCanaryTiers() public {
+        PrizeHooks memory beforeHookOnly = PrizeHooks(true, false, hooks);
+
+        vm.startPrank(alice);
+        claimable.setHooks(beforeHookOnly);
+        vm.stopPrank();
+
+        mockClaimPrize(alice, 1, 2, prizeRedirectionAddress, 1e17, bob);
+
+        vm.expectRevert();
+        
+        claimable.claimPrize(alice, 1, 2, 1e17, bob);
+    }
+
     function testClaimPrize_afterClaimPrizeHook() public {
         PrizeHooks memory afterHookOnly = PrizeHooks(false, true, hooks);

@@ -259,6 +275,13 @@ contract ClaimableTest is Test, IPrizeHooks {
         uint96 reward,
         address rewardRecipient
     ) external returns (address, bytes memory) {
+        // prizePool.isCanarayTier(tier)
+        bool isCanaryTier = true;
+
+        if(isCanaryTier){
+            revert();
+        }
+
         if (useTooMuchGasBefore) {
             for (uint i = 0; i < 1000; i++) {
                 someBytes = keccak256(abi.encode(someBytes));

[trmid]

Coincidentally, about 2 weeks ago I started writing and sharing an article about how depositors can use this as a way to "vote" on prize sizes. Recently published the article showing how this can be used to democratize the prize algorithm.

User's have control over the expansion of tiers

This is permissionless and powerful!

---

One dev's bug is another dev's feature.

[0xjuaan]

Escalate

The following is the criteria for medium severity issues

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
2. Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

This issue is informational

The impact of this issue as described by the watson is

User's have control over the expansion of tiers

This does not lead to loss of funds AND does not break core contract functionality. Therefore the issue does not satisfy sherlock's requirements for medium severity

[sherlock-admin3]

Escalate

The following is the criteria for medium severity issues

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
2. Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

This issue is informational

The impact of this issue as described by the watson is

User's have control over the expansion of tiers

This does not lead to loss of funds AND does not break core contract functionality. Therefore the issue does not satisfy sherlock's requirements for medium severity

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.

[nevillehuang]

@10xhash What is the impact of your PoC mentioned here?

[10xhash]

@10xhash What is the impact of your PoC mentioned here?

User's can revert specifically on prize claims occurring for canary tiers. The expansion of tiers (number of tiers determine the size and the number of prizes) is based on whether the canary tier prizes are claimed or not and hence this allows user's to influence it without any loss to them (since they gain nothing from a canary tier claim)

[WangSecurity]

To clarify, the attack is pretty much the same as for the issue from C4 audit, but here it doesn't allow to bypass the fees but to prevent the expansion of tiers?

And about the impact, this allows users to increase their chance of winning, or just the size and number of prizes are smaller (i.e. what is the benefit for the attacker to control expansion of tiers or loss to others?).

[10xhash]

To clarify, the attack is pretty much the same as for the issue from C4 audit, but here it doesn't allow to bypass the fees but to prevent the expansion of tiers?

And about the impact, this allows users to increase their chance of winning, or just the size and number of prizes are smaller (i.e. what is the benefit for the attacker to control expansion of tiers or loss to others?).

Yes

It doesn't improve a user's chance of winning directly. If some user's want to have a specific distribution of prizes (eg: instead of distributing prizes more frequently, they want to have a large yearly prize), they can influence to achieve this

[Hash01011122]

Attack Vector: But as mitigation, the contract will revert. But this still allows the user's to specifically revert on just the last canary tiers in order to always prevent the expansion of the tiers

Impact: User's have control over the expansion of tiers

As pointed out by @0xjuaan this issue has no impact on protocol whatsoever. This should be considered as low/info issue.

Another point to note from the sponsors comment:

One dev's bug is another dev's feature.

This is a design choice of protocol and can be invalidated.

@nevillehuang @WangSecurity Do correct me if I am wrong

[WangSecurity]

Firstly, as I understand, it's not a design decision, but an acceptable risk, which was not documented (this is confirmed). Secondly, I believe it allows the users to control tiers to their needs, an examples is given by the submitter here. Since, it wasn't documented as acceptable, and gives users more control than they should have, which I believe breaks core contract functionality. But if it's incorrect please correct me.

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

[Hash01011122]

@WangSecurity I guess you are getting the escalation wrong, even if we overlook the fact of design choice then there isn't any direct impact nor breakage of core functionality.

If some user's want to have a specific distribution of prizes (eg: instead of distributing prizes more frequently, they want to have a large yearly prize), they can influence to achieve this.

Users can influence it in some cases to benefit from it increasing their odds, this doesn't qualify as direct impact.

This is low/info severity issue.

[WangSecurity]

As I understand it indeed has impact, the user is able to control distribution of prizes (i.e. get a large yearly prize instead of frequent prizes), when they shouldn't be able to. And it wasn't the design, hence, I see it as a valid issue.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[Hash01011122]

@10xhash @trmid What large yearly prize instead of frequent prizes will account to? In terms of loss incurred? Also given the timeframe wouldn't protocol notice it and can redeploy tier contracts? And why this isn't design choice and acceptable risk @WangSecurity, because codebase is clearly designed the way @trmid pointed out.

[WangSecurity]

I still believe that this is a vulnerability because the users gain control that they shouldn't have and can prevent the expansion of tiers. It will have rather a long-term effect and can lead to this user getting more wins than they should've if they hadn't this control.

I can confirm it's not a design decision and it's still the same issue as it was in the previous audit, but the impact is lower. It's not an acceptable risk because it wasn't put in a special field in the README and given the fact that the sponsors view it as good, rather than bad, doesn't mean we have to straight invalidate it.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: rejected