Audit Round Manager contract

[michellema1208]

[gravityblast]

here are some comments, I think no major issues have been found so far.

[gravityblast]

All these functions should be set as external:

- ProgramFactory.initialize() (contracts/program/ProgramFactory.sol#24-27)  
- ProgramFactory.updateProgramContract(address) (contracts/program/ProgramFactory.sol#37-41)  
- ProgramImplementation.initialize(bytes) (contracts/program/ProgramImplementation.sol#46-75)  
- ProgramImplementation.updateMetaPtr(MetaPtr) (contracts/program/ProgramImplementation.sol#79-82)  
- RoundFactory.initialize() (contracts/round/RoundFactory.sol#37-40)  
- RoundFactory.updateRoundContract(address) (contracts/round/RoundFactory.sol#50-54)  
- RoundImplementation.initialize(bytes) (contracts/round/RoundImplementation.sol#104-167)  
- RoundImplementation.updateRoundMetaPtr(MetaPtr) (contracts/round/RoundImplementation.sol#171-176)  
- RoundImplementation.updateApplicationMetaPtr(MetaPtr) (contracts/round/RoundImplementation.sol#180-185)  
- RoundImplementation.updateRoundStartTime(uint256) (contracts/round/RoundImplementation.sol#189-198)  
- RoundImplementation.updateRoundEndTime(uint256) (contracts/round/RoundImplementation.sol#202-211)  
- RoundImplementation.updateApplicationsStartTime(uint256) (contracts/round/RoundImplementation.sol#215-224)  
- RoundImplementation.updateApplicationsEndTime(uint256) (contracts/round/RoundImplementation.sol#228-237)  
- RoundImplementation.updateProjectsMetaPtr(MetaPtr) (contracts/round/RoundImplementation.sol#241-246)  
- RoundImplementation.applyToRound(bytes32,MetaPtr) (contracts/round/RoundImplementation.sol#251-253)  
- RoundImplementation.vote(bytes[]) (contracts/round/RoundImplementation.sol#257-260)  

[gravityblast]

slither has some warning on dangerous comparisons with timestamps, which can be manipulated by miners. but in our case I don't think it's a problem because all these functions are only executable by owners.

• RoundImplementation.initialize(bytes) (contracts/round/RoundImplementation.sol#104-167) uses timestamp for comparisons
  Dangerous comparisons:
  • require(bool,string)(_applicationsStartTime >= block.timestamp,initialize: applications start time has already passed) (contracts/round/RoundImplementation.sol#134)
• RoundImplementation.updateRoundStartTime(uint256) (contracts/round/RoundImplementation.sol#189-198) uses timestamp for comparisons
  Dangerous comparisons:
  • require(bool,string)(_newRoundStartTime >= block.timestamp,updateRoundStartTime: start time has already passed) (contracts/round/RoundImplementation.sol#191)
    RoundImplementation.updateRoundEndTime(uint256) (contracts/round/RoundImplementation.sol#202-211) uses timestamp for comparisons
    Dangerous comparisons:
  • require(bool,string)(_newRoundEndTime >= block.timestamp,updateRoundEndTime: end time has already passed) (contracts/round/RoundImplementation.sol#204)
    RoundImplementation.updateApplicationsStartTime(uint256) (contracts/round/RoundImplementation.sol#215-224) uses timestamp for comparisons
    Dangerous comparisons:
  • require(bool,string)(_newApplicationsStartTime >= block.timestamp,updateApplicationsStartTime: application start time has already passed) (contracts/round/RoundImplementation.sol#217)
    RoundImplementation.updateApplicationsEndTime(uint256) (contracts/round/RoundImplementation.sol#228-237) uses timestamp for comparisons
    Dangerous comparisons:
  • require(bool,string)(_newApplicationsEndTime >= block.timestamp,updateApplicationsEndTime: application end time has already passed) (contracts/round/RoundImplementation.sol#230)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp

[gravityblast]

Naming convention

slither has many warnings on function arguments starting with _ but I think it's used in many contracts in the same way so it shouldn't be a problem.

[gravityblast]

Lack of zero check. I don't think it's a problem since they are all functions callable by admins.

• ProgramFactory.updateProgramContract(address)._programContract (contracts/program/ProgramFactory.sol#37) lacks a zero-check on :
  • programContract = _programContract (contracts/program/ProgramFactory.sol#38)
    RoundFactory.updateRoundContract(address)._RoundContract (contracts/round/RoundFactory.sol#50) lacks a zero-check on :
• RoundFactory.updateRoundContract(address)._RoundContract (contracts/round/RoundFactory.sol#50) lacks a zero-check on :
  • RoundContract = _RoundContract (contracts/round/RoundFactory.sol#51)

[gravityblast]

Empty lines.

• blank line at contracts/program/ProgramFactory.sol#22 can be removed

[thelostone-mc]

Ah perfect 🙌🏼 https://github.com/gitcoinco/grants-round/pull/228 should fix all of these <3 thanks for the review man