sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

berndartmueller - Entire prize pool reserve is used up after a draw is awarded, preventing building up a larger reserve over time #128

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

berndartmueller

medium

Entire prize pool reserve is used up after a draw is awarded, preventing building up a larger reserve over time

Summary

The finishDraw function caps the incentives with the maxRewards upper bound but donates the entire remaining reserve to the prize pool, which prevents building up a larger reserve over time.

Vulnerability Detail

A portion of the vault contributions are captured as reserve, used to fund the incentives to award the draw (request a random number via DrawManager.startDraw and submit the random number and award the draw via DrawManager.finishDraw). The collected prize pool reserve is kept track of via the TieredLiquidityDistributor._reserve variable.

The eligible rewards (availableRewards) for starting and finishing the draw are calculated via the _computeAvailableRewards function in line 333 of the finishDraw function.

503: /// @notice Computes the available rewards for the auction (limited by max).
504: /// @return The amount of rewards available for the auction
505: function _computeAvailableRewards() internal view returns (uint256) {
506:   uint256 totalReserve = prizePool.reserve() + prizePool.pendingReserveContributions();
507:   return totalReserve > maxRewards ? maxRewards : totalReserve;
508: }

Those rewards are capped by the maxRewards upper bound to ensure that not the entire reserve is used.

After the draw got rewarded, the remaining reserve (remainingReserve) is donated to the prize pool.

However, recalling that the rewards are capped by maxRewards, donating the remaining reserve causes the reserve to be used up entirely, which prevents building up a larger reserve over time to act as a cushion when there is insufficient tier liquidity.

Impact

Whenever a draw is awarded, the entire prize pool reserve is used up (as incentives and donations), which prevents building up a larger reserve over time.

Code Snippet

DrawManager.finishDraw#L354-L369

314: function finishDraw(address _rewardRecipient) external returns (uint24) {
...    // [...]
353:
354:   uint256 remainingReserve = prizePool.reserve();
355:
356:   emit DrawFinished(
357:     msg.sender,
358:     _rewardRecipient,
359:     drawId,
360:     _computeElapsedTime(startDrawAuction.closedAt, block.timestamp),
361:     _finishDrawReward,
362:     remainingReserve
363:   );
364:
365:   if (remainingReserve != 0 && vaultBeneficiary != address(0)) {
366:     _reward(address(this), remainingReserve);
367:     prizePool.withdrawRewards(address(prizePool), remainingReserve);
368:     prizePool.contributePrizeTokens(vaultBeneficiary, remainingReserve);
369:   }

Tool used

Manual Review

Recommendation

Consider calculating the remaining reserve as the difference between the availableRewards (which incorporates the maxRewards cap) and the rewards actually used as incentives.

nevillehuang commented 5 months ago

Request poc

Likely Invalid per sponsor comments:

  • This is intended in deployments that set a vaultBeneficiary on the draw manager
  • The prize pool works on a statistical model to ensure that the probability of over-awarding a prize tier is within reason. There are 2 tools a prize pool deployer can use for this purpose. The first is the reserve, which can be built up to provide backstop liquidity. The second is the tierUtilizationRatio which can be set to ensure prizes are calculated using a specific portion of available tier liquidity. By setting the utilization ratio low enough, the deployer can ensure that the chance of a prize being over-awarded is below their desired threshold. The utilization ratio is used when the vaultBeneficiary is provided so that the reserve does not need to be relied on for prize backstops.
  • It's up to the prize pool deployer to configure these parameters correctly for any given network environment
sherlock-admin4 commented 5 months ago

PoC requested from @berndartmueller

Requests remaining: 4

berndartmueller commented 5 months ago

Hey @nevillehuang!

Given the sponsor's comments, I have to agree that this is a design decision, and it's up to the prize pool deployer (and draw manager deployer, i.e., likely the same actor) to correctly configure the parameters.

Thus, my submission is invalid and can be closed. Thanks!