Platform: Code4rena
Start Date: 19/08/2021
Pot Size: $30,000 USDC
Total HM: 5
Participants: 11
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 26
League: ETH
Rank: 5/11
Findings: 2
Award: $2,217.98
🌟 Selected for report: 4
🚀 Solo Findings: 0
JMukesh
Due to uninitialized-state-variables : marketWhitelist, marketWhitelistCheck() will always return true, since it will always satisfy if condition requiredRole == bytes32(0)), it will never go to else part
manual review
Initialize all the variables
#0 - Splidge
2021-08-25T14:08:02Z
Duplicate of #18
259.6738 USDC - $259.67
JMukesh
since the parameter in the constructor are used to initialize the state variable , proper check up should be done , other wise error in these state variable can lead to redeployment of contract
manual review
add zero address validation
#0 - Splidge
2021-08-26T11:12:36Z
RCOrderbook and RCTreasury both have setter functions where an incorrectly initialized variable could be set after deployment. It's unlikely for these to be set incorrectly because of the deployment script we use, however I'll add a setter to the RCLeaderboard so we can set the address afterwards.
#1 - Splidge
2021-09-02T19:05:53Z
Treasury setter in RCLeaderboard added here
🌟 Selected for report: JMukesh
577.0529 USDC - $577.05
JMukesh
since no limit is mentioned in batchWhitelist() for the input of _users array , it may run out of gas if array length become large
manual review
add a limitation for which , this number of address can be whitelisted at a time
#0 - Splidge
2021-08-26T11:16:10Z
This whitelist is a temporary measure to be used in the Beta and the run-up to launch, after launch it will be disabled. As such we will not be making changes to a feature that will not be used going forward.
🌟 Selected for report: JMukesh
577.0529 USDC - $577.05
JMukesh
referenceContractAddress is used in createMarket() to create newAddress for the market , a necessary check should be there that referenceContractAddress exist or not, because if createMarket() is called before setReferenceContractAddress() address(0) will be passed as referenceContractAddress , since addMarket() of treasury and nfthub does not have address validation for the market
manual review
add a condition to check the referenceContractAddress
#0 - Splidge
2021-09-07T10:59:14Z
Fixed here
🌟 Selected for report: JMukesh
577.0529 USDC - $577.05
JMukesh
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
manual review
add require condition to check those value before setting it
#0 - Splidge
2021-08-25T08:25:34Z
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.
#1 - Splidge
2021-09-07T10:57:43Z
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