code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

prizes are probabilistic and the late-claimer may not be able to claim his/her price when some more than the expected number of winners are generated and reserve is not enough to cover the excess #17

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L36-L43

Vulnerability details

Impact

prizes are probabilistic and the late-claimer would not be able to claim his/her price when some more than the expected number of winners are generated in a draw and reserve is not enough to cover the excess.

In the protocol, at a particular tier count, each tier prize has a probability of being hit. All user of any vault would collectively shares a slice of this probability of winning. Let's say at tier count of 3, tier 1(1-level below grand) has a probability of 5.23% of being hit (TIER_ODDS_1_3).

Since there would be 4 ** 1 priceCount, each user can check against his/her random number at prieIndex(0 1 2 3); therefore the collectively probabilities of having tier 1 prize is 5.23 * 4 = 21%.

With this configuration, prizes 1 is expected to have 1 winner every 5 rounds, expectedDuration is 100/5.23 = 19.12 days. If a winner occurs within 5days after the previous winner, he/she simply gets less prizes due to a short period of accumulation. However, if 2 winners occurs at the same draw, the 1st claimer can of course collect the full/remaining prize; however the 2nd winner might not be able to claim anything if the reserve is not enough to cover the deficit, this is probably true for some bigger prize, or at the early stage of the protocol.

The impact is that at low-probability occurrence, a winner could be rejected the prize due to low reserve, and he/she could never claim it back once the new draw has also passes, leading to principle loss. (claimable that belonged to him/her but unable to claim)

  function prizeCount(uint8 _tier) internal pure returns (uint256) {
    uint256 _numberOfPrizes = 4 ** _tier;

    return _numberOfPrizes;
  }

During consumeLiquidity, the function would first check against the required prizeAmount against the remaining liquidity, if the remaining liquidity is not enough then it tries to deduce from the reserve; if even the reserve is not enough to cover the gap; revert would happen.

``solidity Tier memory _tierStruct, uint8 _tier, uint104 _liquidity ) internal returns (Tier memory) { uint8 _shares = _computeShares(_tier, numberOfTiers); uint104 remainingLiquidity = uint104( fromUD60x18( _getTierRemainingLiquidity( _shares, fromUD34x4toUD60x18(_tierStruct.prizeTokenPerShare), fromUD34x4toUD60x18(prizeTokenPerShare) ) ) ); if (_liquidity > remainingLiquidity) { uint104 excess = _liquidity - remainingLiquidity; if (excess > _reserve) { revert InsufficientLiquidity(_liquidity); } _reserve -= excess; emit ReserveConsumed(excess); _tierStruct.prizeTokenPerShare = prizeTokenPerShare; } else { UD34x4 delta = fromUD60x18toUD34x4(toUD60x18(_liquidity).div(toUD60x18(_shares))); _tierStruct.prizeTokenPerShare = UD34x4.wrap( UD34x4.unwrap(_tierStruct.prizeTokenPerShare) + UD34x4.unwrap(delta) ); } _tiers[_tier] = _tierStruct; return _tierStruct; }



## Proof of Concept
NA
## Tools Used

## Recommended Mitigation Steps
This is a non-trivial problem, since the core design is to allow a statistical representation; even a low probability prize could happen to more than the expected number of winners; it is advisable to cache the winners and the "should-have-won amount` at that draw, so that he/she could come back and claim when the reserve is big enough to cover the claimable.

## Assessed type

Token-Transfer
c4-sponsor commented 1 year ago

asselstine marked the issue as disagree with severity

asselstine commented 1 year ago

This is by design. The number of missed prizes can be reduced to an extremely small number depending on the values of the reserve shares and prize tier shares.

We have a simulator project that runs a monte carlo simulation, and can tune the system to achieve comfortable padding in the reserve.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

Picodes commented 1 year ago

This works as intended and is a known small edge case of the system. Downgrading to Low as with proper configuration this happens with very small chances.