recoverTimelock can have a value shorter than the winner's deadline to claim

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L159

Vulnerability details

Impact

The value of recoverTimelock is checked to be greater than a week and less than a year, but it should never be allowed to be shorter than block.timestamp + settings.drawBufferTime, which is the time given to the winner to claim the NFT. Otherwise it could happen that while in the winner claim period, the owner could call lastResortTimelockOwnerClaimNFT and get it first, preventing the winner from claiming it.

Proof of Concept

When initializing a new draw, function initialize does the following checks on settings.recoverTimelock and settings.drawBufferTime:

if (_settings.drawBufferTime < HOUR_IN_SECONDS) {
    revert REDRAW_TIMELOCK_NEEDS_TO_BE_MORE_THAN_AN_HOUR();
}

^- drawBufferTime must be at least 1 hour

if (_settings.drawBufferTime > MONTH_IN_SECONDS) {
    revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();
}

^- drawBufferTime must be at most 1 month

if (_settings.recoverTimelock < block.timestamp + WEEK_IN_SECONDS) {
    revert RECOVER_TIMELOCK_NEEDS_TO_BE_AT_LEAST_A_WEEK();
}

^- recoverTimelock be at least 1 week ahead in time

if (
    _settings.recoverTimelock >
    block.timestamp + (MONTH_IN_SECONDS * 12)
) {
    revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
}

^- recoverTimelock be at most 1 year ahead in time drawBufferTime is used to set request.drawTimelock which is the time limit for the winner to withdraw the NFT. This is set when the user calls startDraw or redraw, and is set as drawBufferTime seconds in the future.

request.drawTimelock = block.timestamp + settings.drawBufferTime;

So we see that recoverTimelock is a fixed time point in the future when the owner will be available to withdraw the NFT, but it could happen that this time comes before the end of the current request.drawTimelock. And this should never be possible to ever happen because the owner should always wait until the claim period expires to be able to call lastResortTimelockOwnerClaimNFT, otherwise it would be unfair for the winner.

Tools Used

Manual analysis.

Recommended Mitigation Steps

I recommend here the following fix: After this line of function _requestRoll:

request.drawTimelock = block.timestamp + settings.drawBufferTime;

Check if recoverTimelock is less than request.drawTimelock and in that case set recoverTimelock = request.drawTimelock:

if(settings.recoverTimelock < request.drawTimelock){
    settings.recoverTimelock = request.drawTimelock;
}

[c4-judge]

gzeon-c4 marked the issue as duplicate of #146

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)