code-423n4 / 2023-03-wenwin-findings

1 stars 1 forks source link

DRAWS_PER_YEAR may be incorrect #321

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotteryMath.sol#L24 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L164 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L272

Vulnerability details

Impact

The error assumes that there are only 52 draws in a year, resulting in less time than expected for the user to claim.

Proof of Concept

LotteryMath.DRAWS_PER_YEAR(52) represents how many draws there are in a year. It is mainly used in returnUnclaimedJackpotToThePot()/claimable() to determine whether the claim is more than one year, if more than can no longer claim But the current protocol can define the drawPeriod, not a fixed (one week per draw)

struct LotteryDrawSchedule {
    /// @dev First draw is scheduled to take place at this timestamp
    uint256 firstDrawScheduledAt;
    /// @dev Period for running lottery
    uint256 drawPeriod;   //<-------------Period
    /// @dev Cooldown period when users cannot register tickets for draw anymore
    uint256 drawCoolDownPeriod;
}

contract LotterySetup is ILotterySetup {
    uint256 public immutable override drawPeriod;
...    

So one year is not necessarily only 52 draws should be calculated, not fixed 52, such as: DRAWS_PER_YEAR = 365 days / drawPeriod

At present, it may lead to a failure to claim very early than the user expects, such as drawPeriod=1 day

Tools Used

Recommended Mitigation Steps

DRAWS_PER_YEAR is calculated by drawPeriod

DRAWS_PER_YEAR = 365 days / drawPeriod

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #408

c4-judge commented 1 year ago

thereksfour changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

thereksfour marked the issue as grade-b