no time restriction in setMarketTimeRestrictions()

[code423n4]

Handle

JMukesh

Vulnerability details

Impact

as mentioned in the comment , time must be at least this much second, but it lack those check that given time is atleast >= someTime , as a result minimumDuration, maximumDuration are directly initialized without any check

Proof of Concept

https://github.com/code-423n4/2021-08-realitycards/blob/39d711fdd762c32378abf50dc56ec51a21592917/contracts/RCFactory.sol#L431

Tools Used

manual review

Recommended Mitigation Steps

add require condition to check those value before setting it

[Splidge]

This is just poor commenting from when advancedWarning, minimumDuration and maximumDuration each had their own setter functions. They were combined to avoid the Factory going over the contract size limit. However the comments weren't combined correctly. 0 seconds is a valid advancedWarning if you want the market to open immediately. 0 seconds for maximumDuration may also be used if we don't want to set a maximum.

I'll not be adding in the checks but I will update the comments.

[Splidge]

On looking back through this I may have misunderstood what the warden was proposing here. I thought the warden wanted a zero value check, but they could have been asking for the minimumDuration < maximumDuration check. This would make more sense as if the maximum was less than the minimum it wouldn't be possible to create a new market. The same problem would happen if advancedWarning was set to be greater than the maximumDuration. Unfortunately the Factory is right on the size limit and adding these checks would require other changes to the contract which pose the risk of introducing more problems. Given how infrequently we expect to change these values, and the fact they can be changed back again should a mistake be made, I'll be marking this issue as acknowledged. Initial fix to add more comments was implemented here