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

12 stars 7 forks source link

DrawNotFinished Revert Condition Issue in PrizePool Contract #287

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/PrizePool.sol#L353-L355

Vulnerability details

Impact

The impact of this finding is significant as it can lead to incorrect assumptions about the completion of the draw in the PrizePool contract. Without the equality check, the function may mistakenly proceed under the false assumption that the draw has finished when it has not. This can result in unexpected consequences such as premature execution of subsequent code or the generation of incorrect results. The issue undermines the reliability and accuracy of the contract's logic, potentially compromising its intended functionality.

Proof of Concept

The code snippet below illustrates the issue in the PrizePool contract:

353    if (block.timestamp < _openDrawEndsAt()) {
      revert DrawNotFinished(_openDrawEndsAt(), uint64(block.timestamp));
    }

You can find the code in context on GitHub here.

Tools Used

manual code review and write report with help of chatgpt

Recommended Mitigation Steps

To mitigate this issue, it is recommended to modify the if statement in the PrizePool contract to include an equality check for the block timestamp and the end time of the draw. The corrected code should appear as follows:

 if (block.timestamp <= _openDrawEndsAt()) {
    revert DrawNotFinished(_openDrawEndsAt(), uint64(block.timestamp));
  }

By including the equality check, the code will accurately handle cases where the block timestamp matches the end time of the draw, ensuring the correct behavior and preventing any erroneous assumptions about the draw's completion.

After implementing the recommended change, it is crucial to conduct thorough testing to verify that the draw's completion is accurately determined and the subsequent logic behaves as intended.

Assessed type

Other

asselstine commented 1 year ago

If we had an equality statement, then Draws would overlap, because draw ends at = draw starts at.

For example: the draw period runs for 10 seconds. The draw starts at time = 10.

The draw will be open between [10, 20). It includes '10' but does not include '20'.

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid