sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

volodya - _directlyContributedReserve doesn't work as expected #48

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

volodya

high

_directlyContributedReserve doesn't work as expected

Summary

DirectlyContributedReserve will always recycle into the next draw

Vulnerability Detail

Impact

Whenever prize is being claimed there is a chance that more prizes will be claimed than the amount of prize sizes. For this reason, to not leave users without a prize tokens are being taken from reserves.

  function _consumeLiquidity(Tier memory _tierStruct, uint8 _tier, uint104 _liquidity) internal {
    uint8 _tierShares = _numShares(_tier, numberOfTiers);
    uint104 remainingLiquidity = SafeCast.toUint104(
      _getTierRemainingLiquidity(
        _tierStruct.prizeTokenPerShare,
        prizeTokenPerShare,
        _tierShares
      )
    );

    if (_liquidity > remainingLiquidity) {
      uint96 excess = SafeCast.toUint96(_liquidity - remainingLiquidity);

      if (excess > _reserve) {
        revert InsufficientLiquidity(_liquidity);
      }

      unchecked {
        _reserve -= excess;
      }
...
    } else {

TieredLiquidityDistributor.sol#L381

For this reason _directlyContributedReserve exist, but the protocol team will need to call it every draw or create a bot that will do it every draw due to the fact that previously reserve is being recycled into the next draw here, so that code above will never is executed.

  function finishDraw(address _rewardRecipient) external returns (uint24) {
...
    uint256 remainingReserve = prizePool.reserve();

    if (remainingReserve != 0 && vaultBeneficiary != address(0)) {
      _reward(address(this), remainingReserve);
      prizePool.withdrawRewards(address(prizePool), remainingReserve);
      prizePool.contributePrizeTokens(vaultBeneficiary, remainingReserve);
    }

    return drawId;
  }

src/DrawManager.sol#L354

Code Snippet

Tool used

Manual Review

Recommendation

I think it should look like this, so users will get their prizes if too many winners happen

-    uint256 remainingReserve = prizePool.reserve();
+        uint256 remainingReserve = prizePool.reserve() - prizePool._directlyContributedReserve();

    if (remainingReserve != 0 && vaultBeneficiary != address(0)) {
      _reward(address(this), remainingReserve);
      prizePool.withdrawRewards(address(prizePool), remainingReserve);
      prizePool.contributePrizeTokens(vaultBeneficiary, remainingReserve);
    }

    return drawId;
  }
  function _computeAvailableRewards() internal view returns (uint256) {
-    uint256 totalReserve = prizePool.reserve() + prizePool.pendingReserveContributions();
+     uint256 totalReserve = prizePool.reserve() + prizePool.pendingReserveContributions() - prizePool._directlyContributedReserve();

    return totalReserve > maxRewards ? maxRewards : totalReserve;
  }
nevillehuang commented 4 months ago

Invalid, the reserves incerement the contributed rewards accordingly as seen here