Rigor Protocol contest - Rohan16's results

1. Multiple initialization of contract

Description

The attacker can initialize the contract, take malicious actions, and allow it to be re-initialized by the project without any error being noticed.

Instances

// Link to github function initialize(Task storage _self, uint256 _cost) public {

//actual codes used in program 

    function initialize(Task storage _self, uint256 _cost) public {
        _self.cost = _cost;
        _self.state = TaskStatus.Inactive;
        _self.alerts[uint256(Lifecycle.None)] = true;
    }

2.USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Instances

// Link to github using Tasks for Task; https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L28-L29

//actual codes used 
contracts/Project.sol:30:    using Tasks for Task;
contracts/Project.sol:33:    using SafeERC20Upgradeable for IDebtToken
contracts/Community.sol:28:    using SafeERC20Upgradeable for IDebtToken;

1. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Instances:

Project.sol:248 Project.sol:311 Project.sol:322 libraries/Tasks.sol:181 Community.sol:624 HomeFiProxy.sol:87

// uint
Project.sol:248:        for (uint256 i = 0; i < _length; i++) {
Project.sol:311:        for (uint256 i = 0; i < _length; i++) {
Project.sol:322:        for (uint256 i = 0; i < _length; i++) {
libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
Community.sol:624:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) 

Recommendation:

I suggest removing explicit initializations for default values.

2.Use !=0 instead of > 0 for UINT

Instances

Project.sol:195 Community.sol:764

Project.sol:195:        require(_cost > 0, "Project::!value>0");
Community.sol:764:        require(_repayAmount > 0, "Community::!repay");

3. Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances:

HomeFi.sol

HomeFi.sol:73:        require(admin == _msgSender(), "HomeFi::!Admin");
HomeFi.sol:78:        require(_address != address(0), "HomeFi::0 address");
HomeFi.sol:84:        require(_oldAddress != _newAddress, "HomeFi::!Change");
HomeFi.sol:142:        require(!addrSet, "HomeFi::Set");
HomeFi.sol:191:        require(lenderFee != _newLenderFee, "HomeFi::!Change");

Project.sol

Project.sol:123:        require(!contractorConfirmed, "Project::GC accepted");
Project.sol:132:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:135:        require(_contractor != address(0), "Project::0 address");
Project.sol:150:        require(_msgSender() == builder, "Project::!B");
Project.sol:153:        require(contractor != address(0), "Project::0 address");
Project.sol:176:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:195:        require(_cost > 0, "Project::!value>0");
Project.sol:238:        require(_taskCount == taskCount, "Project::!taskCount");
Project.sol:241:        require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol:245:        require(_length == _taskCosts.length, "Project::Lengths !match");
Project.sol:277:        require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol:308:        require(_length == _scList.length, "Project::Lengths !match");
Project.sol:341:        require(_projectAddress == address(this), "Project::!Project");
Project.sol:369:        require(tasks[_taskID].getState() == 3, "Project::!Complete");
Project.sol:406:        require(_project == address(this), "Project::!projectAddress");
Project.sol:511:        require(_project == address(this), "Project::!projectAddress");
Project.sol:530:        require(getAlerts(_task)[2], "Project::!SCConfirmed");
Project.sol:753:        require(_sc != address(0), "Project::0 address");

Disputes.sol

Disputes.sol:39:        require(_address != address(0), "Disputes::0 address");
Disputes.sol:46:        require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Disputes.sol:52:        require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Disputes.sol:183:        require(_result, "Disputes::!Member");

DebtToken.sol:50

DebtToken.sol:50:        require(_communityContract != address(0), "DebtToken::0 address");

ProjectFactory.sol

ProjectFactory.sol:36:        require(_address != address(0), "PF::0 address");
ProjectFactory.sol:84:        require(_msgSender() == homeFi, "PF::!HomeFiContract");

libraries/Tasks.sol

libraries/Tasks.sol:44:        require(_self.state == TaskStatus.Inactive, "Task::active");
libraries/Tasks.sol:50:        require(_self.state == TaskStatus.Active, "Task::!Active");
libraries/Tasks.sol:124:        require(_self.subcontractor == _sc, "Task::!SC");

Community.sol

Community.sol:69:        require(_address != address(0), "Community::0 address");
Community.sol:75:        require(_msgSender() == homeFi.admin(), "Community::!admin");
Community.sol:241:        require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Community.sol:248:        require(_community.isMember[_builder], "Community::!Member");
Community.sol:536:        require(_builder == _projectInstance.builder(), "Community::!Builder");
Community.sol:557:        require(!restrictedToAdmin, "Community::restricted");
Community.sol:568:        require(restrictedToAdmin, "Community::!restricted");
Community.sol:764:        require(_repayAmount > 0, "Community::!repay");
Community.sol:792:        require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
HomeFiProxy.sol:41:       require(_address != address(0), "Proxy::0 address");

HomeFiProxy.sol

HomeFiProxy.sol:81:        require(_length == _implementations.length, "Proxy::Lengths !match");
HomeFiProxy.sol:133:        require(_length == _contractAddresses.length, "Proxy::Lengths !match");

4.++I COSTS LESS GAS THAN I++

Instances:

Project.sol:625 Project.sol:672 Community.sol:140

Project.sol:625:                    _loopCount++;
Project.sol:672:                    _loopCount++;
Community.sol:140:        communityCount++;