claimability of a winning ticket is not checked correctly

[code423n4]

Lines of code

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/LotteryMath.sol#L24

Vulnerability details

Impact

A winning ticket must be claimed before its expiration time that should be fixed at a least one year time period.

From the documentation : Winning tickets have a 1-year deadline from the draw time to claim the prize by redeeming the winning NFT ticket.

But the code is using a fixed value when checking for the maximal claimable time.

This value is calculated based on a 1 day per week lottery draw. Because the nature of the protocol allows a instance of the Lottery where can be more/less then 1 per week draw the claimability of a winning ticket is not correctly checked.

Proof of Concept

The value DRAW_PER_YEAR is used to check if a ticket is claimable for a year period

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

And is fixed to the value of 52 (which is the number of weeks in an average year .. so this value is best suitable in the 1 draw per week scenario)

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

This will only hold true if a Lottery contract instance will be created with the LotteryDrawSchedule.drawPeriod of 7 days in seconds.

Any other value will result in the ticket being claimable for less/more then a year because there will be more/less drawings then the 52 fixed.

Tools Used

Manual review

Recommended Mitigation Steps

Consider dynamically calculating the DRAW_PER_YEAR field. Something like:

// define an immutable uint256 field
// best place would be inside the LotterySetup/ILotterySetup contract 
uint256 public immutable override drawPerYear;

// inside LotterySetup constructor after validating that drawPeriod value is OK ..calculate
drawPerYear=365/lotterySetupParams.drawSchedule.drawPeriod;
// from here on we can also validate for a minimal draw per years value... 

Now when checking if ticket is still valid use the newly defined drawPerYear field.

[c4-judge]

thereksfour marked the issue as duplicate of #408

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-b