Non-inclusive checks currently break contract's logic in a few instances

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L95-L117

Summary

Checks are used in protocol to set some limitations/deadlines, but some instances lack inclusivity at the ends where as they should be

Vulnerability Detail

Take the setContest() function as an example

Click to see code reference

```solidity /** * @notice Only owner can set contest's properties * @notice close time must be less than 28 days from now * @dev Set contest close time, implementation address, organizer, contest id * @dev only owner can call this function * @param organizer The owner of the contest * @param contestId The contest id * @param closeTime The contest close time * @param implementation The implementation address */ function setContest(address organizer, bytes32 contestId, uint256 closeTime, address implementation) public onlyOwner { if (organizer == address(0) || implementation == address(0)) revert ProxyFactory__NoZeroAddress(); if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) { revert ProxyFactory__CloseTimeNotInRange(); } bytes32 salt = _calculateSalt(organizer, contestId, implementation); if (saltToCloseTime[salt] != 0) revert ProxyFactory__ContestIsAlreadyRegistered(); saltToCloseTime[salt] = closeTime; emit SetContest(organizer, contestId, closeTime, implementation); } } ```

As seen it's been explicitly stated that the close time must be less than 28 days from now but this is not applied in code, since in the edge case where close time == 28 days + block.timestamp, i.e not less than 28 days the line below does not revert

    if (closeTime > block.timestamp + MAX_CONTEST_PERIOD || closeTime < block.timestamp) { revert ProxyFactory__CloseTimeNotInRange(); }
Also note that the same line does not revert if closeTime == block.timestamp which doesn't seem like the right way to operate a contest

Additinonally do note that a few other references still exist in the ProxyFactory.sol contract but for brevity reasons only the setContest() instance has been discussed in report

Impact

A break in contract's logic in multiple instances

Tool used

Manual Audit

Recommendation

Check if a check should be inclusive and make it so if yes, additionally would be nice to introduce a minimum contest period as it doesn't make sense to have a contest just last for say 5 minutes or that has closeTime == block.timestamp.

Cyfrin / 2023-08-sparkn