`PrizePool` will generate a partial DoS due to underflow

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L745

Vulnerability details

Impact

The PrizePool will generate a partial DoS over time due to underflow when _totalWithdrawn grow significantly.

Proof of Concept

Note: There is no way to decrease _totalWithdrawn variable

The _totalWithdrawn variable represents the total amount of prize tokens that have been claimed over time. It is updated in the _transfer function as follows:

  function _transfer(address _to, uint256 _amount) internal {
    _totalWithdrawn += _amount;
    prizeToken.safeTransfer(_to, _amount);
  }

and the _transfer() function is triggered in this three functions:

• withdrawReserve()
• claimPrize()
• withdrawClaimRewards()

As a result, the _totalWithdrawn variable will grow significantly over time, and it is used computes how many tokens have been accounted for in the _accountedBalance function:

  function _accountedBalance() internal view returns (uint256) {
    Observation memory obs = DrawAccumulatorLib.newestObservation(totalAccumulator);
    return (obs.available + obs.disbursed) - _totalWithdrawn; //@audit underflow
  }

• obs.available: This field represents the total amount available for a particular draw as of the current observation.

• obs.disbursed: This field tracks the total amount disbursed or distributed in previous draws up to the current observation.

Both of these variables will not be more than _totalWithdrawn over time, resulting in an underflow in the _accountedBalance function, which causes a DoS to the contributePrizeTokens() function:

  function contributePrizeTokens(address _prizeVault, uint256 _amount) external returns (uint256) {
    uint256 _deltaBalance = prizeToken.balanceOf(address(this)) - _accountedBalance();
    if (_deltaBalance < _amount) {
      revert ContributionGTDeltaBalance(_amount, _deltaBalance);
    }
    DrawAccumulatorLib.add(
      vaultAccumulator[_prizeVault],
      _amount,
      lastClosedDrawId + 1,
      smoothing.intoSD59x18()
    );
    DrawAccumulatorLib.add(
      totalAccumulator,
      _amount,
      lastClosedDrawId + 1,
      smoothing.intoSD59x18()
    );
    emit ContributePrizeTokens(_prizeVault, lastClosedDrawId + 1, _amount);
    return _deltaBalance;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a diffrent mechanism to compute the accounted balance.

Assessed type

Under/Overflow

[Picodes]

Do you have an estimation for this? 2**256 is > 1e71 so assuming 1e30 transfer size (which is large) there is still no chances than it happens

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[0xbtk]

Hey @Picodes,

Thank you for your feedback on this matter. I'd like to clarify a key aspect of the issue, as it seems there might be some misunderstanding.

The concern with _totalWithdrawn isn't tied to a single massive transaction of 1e30, as you mentioned.

Instead, it arises due to the cumulative effect of repeated increments over time. With each transfer, the _totalWithdrawn value grows significantly as it accumulates claimed or withdrawn amounts.

  function _transfer(address _to, uint256 _amount) internal {
    _totalWithdrawn += _amount;
    prizeToken.safeTransfer(_to, _amount);
  }

And the _transfer() function is called in:

• withdrawReserve()
• claimPrize()
• withdrawClaimRewards()

While it's true that the magnitude of 2**256 is immense and an assumed transfer size of 1e30 would be substantial, the crux of the issue lies in the gradual growth of _totalWithdrawn across multiple transfers. This can result in an unintended underflow scenario in the _accountedBalance() function.

note: Keep in mind that _totalWithdrawn is never decremented

I would greatly appreciate any further insights or clarifications you can provide regarding the estimation and the likelihood you mentioned:

Do you have an estimation for this? 2**256 is > 1e71 so assume 1e30 transfer size (which is large) there is still no chances than it happens

Thank you for your time and consideration!

[Picodes]

@btk yes this was actually my point: assuming 1e30 average transfer size, it'd require more than 1e40 transfers to overflow, which is just impossible considering for example the gas price it'd require.

[0xbtk]

Hey @Picodes, I did undrestand you're point but I am not sure that you undrestand mine sir.

The overflow issue isn't the focus here, as I understand that 2**256 is indeed a substantial value.

The critical point is that as time goes on, _totalWithdrawn will continue to increase. In this context, both obs.available and obs.disbursed will become smaller compared to _totalWithdrawn.

Here is a simple scenario to illustrate:

Let's assume that over time _totalWithdrawn increases to be:

• _totalWithdrawn = 50e18

Meanwhile, the values for this Observation are:

• obs.available = 10e18
• obs.disbursed = 30e18

In this scenario, when _accountedBalance is called, an underflow occurs. This is because _totalWithdrawn surpasses the combined value of both obs.available and obs.disbursed.

I do respect your decision. I just wanted to provide an additional explanation to ensure that you undrestand the issue.

[Picodes]

@btkayoub my bad, you are right and I had totally misunderstood the report, thinking it was about an overflow of _totalWithdrawn. Let me check this.

[Picodes]

So you are discussing the possibility of underflow in (obs.available + obs.disbursed) - _totalWithdrawn. I think an underflow is indeed possible here because of other issues in the code like https://github.com/code-423n4/2023-07-pooltogether-findings/issues/147. The reason is that totalWithdrawn is increased in withdrawReserve() and reserve accountability is broken. But obs.available + obs.disbursed should remain bigger than the amounts transferred out in claimPrize() and withdrawClaimRewards().

Overall I don't think this is because what you explain above (_totalWithdrawn isn't decreasing): obs.disbursed is not decreasing as well and has the same properties as totalWithdrawn: see https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/dfdc651705a92ed713d9f87e3c013382c9b7ea7d/src/libraries/DrawAccumulatorLib.sol#L93 and https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/dfdc651705a92ed713d9f87e3c013382c9b7ea7d/src/libraries/DrawAccumulatorLib.sol#L108

Overall my view is that this report is invalid in itself as there is no specific issue related to _totalWithdrawn being only increasing. I'll ping the sponsor about this to make sure they voice their opinion.

@btkayoub feel free to point out any fact that I may be missing here

[0xbtk]

The reason is that totalWithdrawn is increased in withdrawReserve() and reserve accountability is broken. But obs.available + obs.disbursed should remain bigger than the amounts transferred out in claimPrize() and withdrawClaimRewards().

Indeed, I shared a similar thought. Yet, when you observe that the reserve acts as a second reservoir of liquidity,, it's reasonable to anticipate substantial token transfers during increaseReserve. Keep in mind, this function is public, meaning anyone can contribute tokens to the reserve.

This insight underscores the potential for significant token inflows to the reserve, which might not conform to the initial expectations in obs.available + obs.disbursed.

I just wanted to pin this fact for the sponsor to review.

[asselstine]

I've been asked to comment here, so I'll add my thoughts. I'll summarize the issue here for clarity:

The PrizePool will generate a partial DoS over time due to underflow when _totalWithdrawn grow significantly. Note: There is no way to decrease _totalWithdrawn variable

The issue is referencing this line: accountedBalance = (obs.available + obs.disbursed) - _totalWithdrawn

Both of these variables will not be more than _totalWithdrawn over time, resulting in an underflow in the _accountedBalance function, which causes a DoS to the contributePrizeTokens() function

*The issue is saying that it's problematic that totalWithdrawn grows indefinitely, because it will become larger than the available + disbursed**

This is not the case because, as the judge mentions, the available also grows indefinitely. I'll add some more background for clarity:

disbursed: the total number of tokens that have been contributed and allocated to prizes up to the last contributed draw. This number is uint168 and always growing available: the total number of tokens available since the last contributed draw and subsequent draws. This number is uint96 and grows and shrinks. totalWithdrawn: the total prizes withdrawn from the prize pool, all time. This number is uint256 and is always growing.

Let's think about these values wrt liquidity flows:

1. new prize contributions are added to the available.
2. after a draw is closed, the draw's available tokens are added to the disbursed tokens.
3. when users claim prizes from the disbursed, they increase the totalWithdrawn field.

From a liquidity perspective, available + disbursed will always be larger than totalWithdrawn. The scenario you describe above is not possible.

Based on the original rationale, I don't think this is a valid issue.

[0xbtk]

Hey @asselstine @Picodes ,

From a liquidity perspective, available + disbursed will always be larger than totalWithdrawn. The https://github.com/code-423n4/2023-07-pooltogether-findings/issues/205#issuecomment-1676046795 is not possible.

I can rely that all of what you said is true only if withdrawReserve() won't increase _totalWithdrawn

1. new prize contributions are added to the available.
2. after a draw is closed, the draw's available tokens are added to the disbursed tokens.
3. when users claim prizes from the disbursed, they increase the totalWithdrawn field.

The assessment you've presented presupposes the absence of calls to both increaseReserve() and withdrawReserve(). This assumption, I believe, might be challenging to maintain as it appears somewhat impractical, because the reserve act as a second reservoir of liquidity and as I said above:

it's reasonable to anticipate substantial token transfers during increaseReserve. Keep in mind, this function is public, meaning anyone can contribute tokens to the reserve.

Furthermore, this presents a potential vulnerability—safeguarding against this requires cautiousness when withdrawing from the reserve in the future, as even seemingly trivial amounts could result in a DoS scenario within the contributePrizeTokens() function.

The restriction on withdrawals from the reserve fundamentally opposes the core purpose of maintaining a reserve. Consequently, this protocol faces heightened risks of insolvency, loosing user trust, and could result in a series of unfavorable outcomes.

I recognize that the initial report lack substantiating evidence (due to a time constraint during its submission) and I do beleive that issue falls bellow:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[Picodes]

@btkayoub I disagree with your view. This issue is the one I described in my comment above, but the original report doesn't describe this. Maybe the intuition was correct, but the report is invalid as it focuses on _totalWithdrawn being strictly increasing. So the original report is invalid. I can't accept it just because it lead us to find another issue during the judging process.

[0xbtk]

I agree with you @Picodes, but I think the unsatisfactory label is a bit too strict. For a valid submission, I think its value comes first from its ability to identify the root cause of the vulnerability, and then a clear proof. The ratio might be 50/50 Not higher than does not mean 0

Submissions which judges deem insufficiently proven will not be eligible for anything higher than a satisfactory score.

Marking a valid issue unsatisfactory for the lack of proof dosn't seem fair.

And to rely on this:

So the original report is invalid. I can't accept it just because it lead us to find another issue during the judging process.

It didn't lead as to find another issue, I just provided more proof on why i think the issue is valid because as a warden I prefer to spend time on new findings rather than writing POC.

I omitted mentioning the increaseReserve function or a PoC due time constraints. Nonetheless, I have clearly described the following aspects:

• The links:

  withdrawReserve() claimPrize() withdrawClaimRewards()

• The impact

The PrizePool will generate a partial DoS over time due to underflow when _totalWithdrawn grow significantly.

• The root cause:

Both of these variables will not be more than _totalWithdrawn over time, resulting in an underflow in the _accountedBalance function, which causes a DoS to the contributePrizeTokens() function.

Even if a subsequent vulnerability fix addresses the concerns raised in #205, it doesn't warrant the invalidation of issue #205

https://github.com/GenerationSoftware/pt-v5-prize-pool/pull/18/files/21bc88a53fb1a9f0babaf293045a2dd67ba8b688