hash - User's might be able to claim their prizes even after shutdown

[sherlock-admin4]

hash

high

User's might be able to claim their prizes even after shutdown

Summary

User's might be able to claim their prizes even after shutdown due to lastObservationAt and draw period difference

Vulnerability Detail

There is no shutdown check kept on the claimPrize function. Hence user's can calim their prizes even after the pool has been shutdown if the draw has not been finalized

link

  function claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    address _prizeRecipient,
    uint96 _claimReward,
    address _claimRewardRecipient
  ) external returns (uint256) {
    /// @dev Claims cannot occur after a draw has been finalized (1 period after a draw closes). This prevents
    /// the reserve from changing while the following draw is being awarded.
    uint24 lastAwardedDrawId_ = _lastAwardedDrawId;
    if (isDrawFinalized(lastAwardedDrawId_)) {
      revert ClaimPeriodExpired();
    }

The remaining balance after a shutdown is supposed to be allocated to user's based on their (vault prize contribution + twab contribution) via the withdrawShutdownBalance and is not supposed to be based on a random number ie. via claimPrize

link

  function withdrawShutdownBalance(address _vault, address _recipient) external returns (uint256) {
    if (!isShutdown()) {
      revert PrizePoolNotShutdown();
    }

In case the draw period is different from the TWAB's period length, it is not necessary that the shutdown due to the lastObservationAt occurs at the end of a draw. In such a case, it will allow user's who are winners of the draw to claim their prizes and also withdraw their share of the shutdown balance hence stealing funds from others

POC

diff --git a/pt-v5-prize-pool/test/PrizePool.t.sol b/pt-v5-prize-pool/test/PrizePool.t.sol
index 99fe6b5..5ce7ad6 100644
--- a/pt-v5-prize-pool/test/PrizePool.t.sol
+++ b/pt-v5-prize-pool/test/PrizePool.t.sol
@@ -75,6 +75,7 @@ contract PrizePoolTest is Test {
   uint256 RESERVE_SHARES = 10;

   uint24 grandPrizePeriodDraws = 365;
+  uint periodLength;
   uint48 drawPeriodSeconds = 1 days;
   uint24 drawTimeout; // = grandPrizePeriodDraws * drawPeriodSeconds; // 1000 days;
   uint48 firstDrawOpensAt;
@@ -112,27 +113,26 @@ contract PrizePoolTest is Test {

   ConstructorParams params;

-  function setUp() public {
-    drawTimeout = 30; //grandPrizePeriodDraws;
-    vm.warp(startTimestamp);
+    function setUp() public {
+      // at end drawPeriod == 2 day, and period length in twab = 1 day

+    periodLength = 1 days;
+    drawPeriodSeconds = 2 days;
+    
+    // the last draw should be ending at lastObservation timestamp + 1 day
+    startTimestamp = 1000 days;
+    firstDrawOpensAt =  uint48((type(uint32).max / periodLength ) % 2 == 0 ? startTimestamp + 1 days : startTimestamp + 2 days);
+
+    
+
+    drawTimeout = 25854; // to avoid shutdown by drawTimeout when warping
+
+    vm.warp(startTimestamp + 1);
     prizeToken = new ERC20Mintable("PoolTogether POOL token", "POOL");
-    twabController = new TwabController(uint32(drawPeriodSeconds), uint32(startTimestamp - 1 days));
+    twabController = new TwabController(uint32(periodLength), uint32(startTimestamp));

-    firstDrawOpensAt = uint48(startTimestamp + 1 days); // set draw start 1 day into future
     initialNumberOfTiers = MINIMUM_NUMBER_OF_TIERS;

-    vm.mockCall(
-      address(twabController),
-      abi.encodeCall(twabController.PERIOD_OFFSET, ()),
-      abi.encode(firstDrawOpensAt)
-    );
-    vm.mockCall(
-      address(twabController),
-      abi.encodeCall(twabController.PERIOD_LENGTH, ()),
-      abi.encode(drawPeriodSeconds)
-    );
-
     drawManager = address(this);
     vault = address(this);
     vault2 = address(0x1234);
@@ -142,7 +142,7 @@ contract PrizePoolTest is Test {
       twabController,
       drawManager,
       tierLiquidityUtilizationRate,
-      drawPeriodSeconds,
+      uint48(drawPeriodSeconds),
       firstDrawOpensAt,
       grandPrizePeriodDraws,
       initialNumberOfTiers, // minimum number of tiers
@@ -155,6 +155,51 @@ contract PrizePoolTest is Test {
     prizePool = newPrizePool();
   }

+  function testHash_CanClaimPrizeAfterShutdown() public {
+    uint secondLastDrawStart = startTimestamp + (type(uint32).max / periodLength ) * periodLength - 3 days;
+
+    vm.warp(secondLastDrawStart + 1);
+    address user1 = address(100);
+    //address user2 = address(200);
+
+    // mint tokens to the user in twab
+    twabController.mint(user1, 10e18);
+    //twabController.mint(user2, 5e18);
+
+    // contribute prize tokens to the vault
+    prizeToken.mint(address(prizePool),100e18);
+    prizePool.contributePrizeTokens(address(this),100e18);
+
+    // move to the next draw and award this one
+    vm.warp(secondLastDrawStart + drawPeriodSeconds + 1);
+    prizePool.awardDraw(100);
+    uint drawId = prizePool.getOpenDrawId();
+
+    //currently not shutdown. but shutdown will occur in the middle of this draw allowing both prize claiming and the shutdown withdrawal
+    uint shutdownTimestamp = prizePool.shutdownAt();
+    assert(shutdownTimestamp == secondLastDrawStart + drawPeriodSeconds + periodLength);
+
+    vm.warp(shutdownTimestamp);
+
+    // call to store the shutdown data before the prize is claimed
+    prizePool.shutdownBalanceOf(address(this),user1);
+
+    /**
+     * address _winner,
+    uint8 _tier,
+    uint32 _prizeIndex,
+    address _prizeRecipient,
+    uint96 _claimReward,
+    address _claimRewardRecipient
+     */
+    prizePool.claimPrize(user1,1,0,user1,0,address(0));
+
+    // no withdrawing shutdown balance will revert due to the amount being withdrawn earlier via the claimPrize function
+    vm.prank(user1);
+    vm.expectRevert();
+    prizePool.withdrawShutdownBalance(address(this),user1);
+  }
+
   function testConstructor() public {
     assertEq(prizePool.firstDrawOpensAt(), firstDrawOpensAt);
     assertEq(prizePool.drawPeriodSeconds(), drawPeriodSeconds);

Impact

User's can double claim assets when vault shutdowns eventually

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-prize-pool/src/PrizePool.sol#L517-L524

Tool used

Manual Review

Recommendation

Add a notShutdown modifier to the claimPrize function

[0xjuaan]

Escalate

This issue is informational

This issue requires the following to be true as stated by the watson

In case the draw period is different from the TWAB's period length.....

The draw period and TWAB's period length are both set by the protocol themselves so setting this incorrectly will be admin error and considered invalid

By looking at the test suite we can clearly see that draw period = TWAB's period length == 1 day

[sherlock-admin3]

Escalate

This issue is informational

This issue requires the following to be true as stated by the watson

In case the draw period is different from the TWAB's period length.....

The draw period and TWAB's period length are both set by the protocol themselves so setting this incorrectly will be admin error and considered invalid

By looking at the test suite we can clearly see that draw period = TWAB's period length == 1 day

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.

[10xhash]

Escalate

The issue allows the winners to claim the rewards twice which causes other user's to loose their prizes which I think should warrant high severity

[sherlock-admin3]

Escalate

The issue allows the winners to claim the rewards twice which causes other user's to loose their prizes which I think should warrant high 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]

@trmid @10xhash Might want to take a look at this comment. If not I rated it medium given the unlikely likelihood of shutdown mentioned in comments here.

[0x73696d616f]

I believe for this to happen, 2 things must be considered:

• The draw length must be different than the twab length.
• The shutdown is caused by the end of the twab controller, in 82 years.

Given these extensive limitations, medium is appropriate. If the shutdown happens due to the rng service, it ends at the end of the draw so prizes can not be claimed and this issue does not exist.

[10xhash]

@trmid @10xhash Might want to take a look at this comment. If not I rated it medium given the unlikely likelihood of shutdown mentioned in comments here.

It is a not an incorrect setting. The requirements for the draw period are that:

    if (
      params.drawPeriodSeconds < twabPeriodLength ||
      params.drawPeriodSeconds % twabPeriodLength != 0
    ) {
      revert IncompatibleTwabPeriodLength();
    }

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-prize-pool/src/PrizePool.sol#L357-L362

In the sponsors comment, it is mentioned that the case of the TWAB timestamps have reached their max limit is being explicitly considered which is the scenario discussed here

The second is guaranteed to happen at the end of life for a TWAB controller and was a major consideration in the design of the shutdown logic

[WangSecurity]

Since there's a set of inputs that doesn't result in an issue, we should consider this set will be used by the protocol team. Planning to accept the escalation and invalidate the issue, cause as I understand there won't be an issue when TWAB and draw periods are the same.

[10xhash]

Since there's a set of inputs that doesn't result in an issue, we should consider this set will be used by the protocol team. Planning to accept the escalation and invalidate the issue, cause as I understand there won't be an issue when TWAB and draw periods are the same.

How is it considered that the protocol will be using drawPeriod == TWABperiod when the code clearly has other rules to determine whether a drawPeriod is compatible or not?

    if (
      params.drawPeriodSeconds < twabPeriodLength ||
      params.drawPeriodSeconds % twabPeriodLength != 0
    ) {
      revert IncompatibleTwabPeriodLength();
    }

[WangSecurity]

If there are values set by admins and only specific set causes issues, then it's considered an admin mistake to set values to cause issues. But, looking at this issue and the code again, I agree it wouldn't be a mistake, cause draw period can be any multiple of TWAB period length. Hence, I agree this scenario is indeed possible.

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

[10xhash]

If there are values set by admins and only specific set causes issues, then it's considered an admin mistake to set values to cause issues. But, looking at this issue and the code again, I agree it wouldn't be a mistake, cause draw period can be any multiple of TWAB period length. Hence, I agree this scenario is indeed possible.

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

I had raised an escalation to reconsider the severity of the issue as high which I don't think have been addressed/considered. Can you please look into it

[WangSecurity]

In the escalation message you say the user is able to claim the prize twice, you mean claim the prize before the shutdown and after the shutdown if the current draw haven't been finalised. Or by that you mean claim the prize and withdraw their share of the shutdown balance?

[10xhash]

mean claim the prize and withdraw their share of the shutdown balance?

"claim the prize and withdraw their share of the shutdown balance?" this. Sorry for the incorrectness there

[WangSecurity]

Then I agree it should be high. Yes, only the winners can do it, but any winner can do it without any extensive limitations. Planning to reject @0xjuaan escalation since the issue should remain valid. Planning to accept @10xhash's escalation and upgrade severity to high.

[0x73696d616f]

@WangSecurity what are your thoughts on these 2 points? The parameter being a specific one and the issue happening in 82 years are extensive limitations (we will never witness it, or there is a 0.0000001% chance). Agree that the sponsor saying they care about the shutdown makes the issue in scope, but high for a issue that will not happen seems too much.

[WangSecurity]

Thank you and excuse me, TWAB limit is always 82 years, correct? And if the shutdown happens due to draw timeout has been reached, then this issue won't happen?

[0x73696d616f]

@WangSecurity I think it does not happen due to the draw timeout, look at the following links to confirm.

Shutdown check

shutdownAt() returns drawTimeoutAt

drawTimeoutAt

drawClosesAt

On the other hand, the twab controller timeout, lastObservationAt(), returns approx 82 years in the future (type(uint32).max).

[WangSecurity]

And if it can't happen at the draw timeout, because in that case the last draw would have been finalised, and this issue requires the opposite, correct?

Moreover, is it guaranteed that when TWAB limit is reached, there would be a not finalised draw, or it depends on configuration?

[0x73696d616f]

And if it can't happen at the draw timeout, because in that case the last draw would have been finalised, and this issue requires the opposite, correct?

Yes

Moreover, is it guaranteed that when TWAB limit is reached, there would be a not finalised draw, or it depends on configuration?

Depends on config, the period length of the twab controller needs to be different than that of the prize pool for them to end at different timestamps and cause this issue

[WangSecurity]

Thank you for this clarifications, then I would agree this is a high constraint, and even though any winner in this case can steal funds, the TWAB period length of 82 years has to be reached, which may never happen due to draw timeout shutdown. Hence, I agree indeed medium severity should remain.

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

[10xhash]

Thank you for this clarifications, then I would agree this is a high constraint, and even though any winner in this case can steal funds, the TWAB period length of 82 years has to be reached, which may never happen due to draw timeout shutdown. Hence, I agree indeed medium severity should remain.

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

Here the project is especially considering this scenario of the max time reaching ie.type(uint32).max. And the other constraint of draw period length being not equal to the twab period length is also not a constraint as such. Both of these are values/situations that are supposed to be handled by the contract

[WangSecurity]

I agree that the draw period being not equal to the TWAB period length is not an extensive constraint. But I believe reaching the TWAB period length of 82 years without a draw timeout, is quite a high constraint. Moreover, this scenario may never occur. Hence, I believe medium is more appropriate.

The decision remains the same, reject both escalations and leave the issue as it is.

[10xhash]

I agree that the draw period being not equal to the TWAB period length is not an extensive constraint. But I believe reaching the TWAB period length of 82 years without a draw timeout, is quite a high constraint. Moreover, this scenario may never occur. Hence, I believe medium is more appropriate.

The decision remains the same, reject both escalations and leave the issue as it is.

A draw timeout would occur when the protocol is non-operational ie. the bots refuse to award draws. The team wants to consider this project to be work correctly even after 82 years. I am reiterating the comment of the sponsor:

The second is guaranteed to happen at the end of life for a TWAB controller and was a major consideration in the design of the shutdown logic

It is different if the project simply decides to not handle the scenario after type(uint32).max and if the project explicitly wants to handle the scenario after this time period. It is upto the team to decide which timeframe they want to provide support for and if they want the timeframe to be even 1000 years, it shouldn't be considered as a limitation.

[WangSecurity]

I understand that this is the scenario the protocol wants to cover, but this doesn't change the fact that 82 years is quite long and there are others reasons why the vault may shutdown before this 82 years expire. Hence, I believe the medium is more appropriate. I don't mean to say this is invalid, or not of importance, but I see it as an extensive limitation, because of which this issue may never arise.

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

[10xhash]

From sherlocks docs, criteria for high:

Definite loss of funds without (extensive) limitations of external conditions

If the protocol plans to operate past 82 years, is that timeframe still considered under extensive limitation of external condition?

[sherlock-admin2]

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

[WangSecurity]

I see that the protocol plans to operate for 82 years, but given the fact, that it's a very long timeframe and other reasons can cause shutdown before it and won't cause this issue, medium is more appropriate. It's not based only on the fact that the TWAB limit is 82 years, but on the combination of factors.

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

[10xhash]

I see that the protocol plans to operate for 82 years, but given the fact, that it's a very long timeframe and other reasons can cause shutdown before it and won't cause this issue, medium is more appropriate. It's not based only on the fact that the TWAB limit is 82 years, but on the combination of factors.

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

There is one other reason due to which the protocol can shutdown and that is if the draw is not awarded for a time period of drawTimeOut. If the protocol is assumed to operate 82 yrs, isn't it trivial that it is also assumed that the bots will continue to draw awards during this period

[WangSecurity]

I still believe that medium severity is more appropriate here, based on the same reasons as here.

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

[10xhash]

I still believe that medium severity is more appropriate here, based on the same reasons as here.

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

ok. I have no more points to make

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: rejected
• 10xhash: rejected

[10xhash]

Fixed Now the shutdownTime is moved to the start of the corresponding draw

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.