Rigor Protocol contest - ElKu's results

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>Zero-check is not performed for address</ins>

In the HomeFi.sol, general address changes are performed only after a zero check. Which is done with the nonZero modifier. But in the setTrustedForwarder function, the new address, _newForwarder is not checked for zero value.

Mitigation would be to add the nonZero modifier in the function as shown below:

    function setTrustedForwarder(address _newForwarder)
        external
        override
        onlyAdmin  
        nonZero(_newForwarder)   //mitigation applied
        noChange(trustedForwarder, _newForwarder)
    {
        trustedForwarder = _newForwarder;
    }

2. <ins>Anyone can withdraw funds for Builder on his behalf (without permission)</ins>

In Project.sol, the recoverTokens function, sends remaining funds to the builder. But this can be initiated by anyone on his behalf without prior permission. This can be confusing from the Builder's perspective.

Mitigation would be to check if the msg.sender is the builder himself.

3. <ins> Value of APR isnt checked to be within range</ins>

In Community contract, the value of APR isnt checked to be within a certain range in the publishProject function. This can lead to a large interest to be calculated on the debt amount.

Mitigation would be to check if APR is within a particular range before publishing the project. At least a maximum value should be specified.

Non-Critical Issues

Summary Of Findings:

Details on Findings:

1. <ins>Variable name should indicate what it represents</ins>

In Project.sol, in the raiseDispute function, the input data is decoded as shown below:

       (address _project, uint256 _task, , , ) = abi.decode(  // @audit QA could be renamed to taskID
            _data,
            (address, uint256, uint8, bytes, bytes)
        );

The variable _task here represents the taskID. And it should be named as such for better readability. In other parts of the code its done well.

2. <ins>Use a newer version of Solidity</ins>

The codebase uses Solidity version 0.8.6 which was released in June 2021. Though its not possible to keep up with the version changes of Solidity, its advisable to use a relatively newer version. The current Solidity version is 0.8.15 which was released in June 2022 (one year later than the current version used by the codebase).

Newer versions like 0.8.10 will skip contract existence checks for external calls, if the external call has a return value. This will reduce gas costs.

Gas Optimizations

Summary Of Gas Savings:

Reduction In Deployment Costs:

Detailed Report on Gas Optimization Findings:

1. <ins>HomeFi.sol</ins>

1. In the createProject function state variable projectCount was read 3 times. By using a cached copy, we could save at least 200 gas.

//Original Code:
        // Update project related mappings
        projects[projectCount] = _project;
        projectTokenId[_project] = projectCount;
        emit ProjectAdded(projectCount, _project, _sender, _currency, _hash);

//Can be optimized into:
        uint256 projectCountCached = projectCount;  // cache the state variable.
        projects[projectCountCached] = _project;
        projectTokenId[_project] = projectCountCached;

2. The createProject calls an internal function called mintNFT, which again uses projectCount 5 times. This could be optimized as follows as well to save around 400 gas.

//Original Code:
        projectCount += 1;
        // Mints NFT and set token URI
        _mint(_to, projectCount);
        _setTokenURI(projectCount, _tokenURI);
        emit NftCreated(projectCount, _to);
        return projectCount;
//Can be optimized into:
        uint256 projectCountCached = ++projectCount;  // @audit gas saving
        // Mints NFT and set token URI
        _mint(_to, projectCountCached);
        _setTokenURI(projectCountCached, _tokenURI);
        emit NftCreated(projectCountCached, _to);
        return projectCountCached;

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 345 gas.

2. <ins>Community.sol</ins>

1. In the createCommunity function state variable communityCount was read 3 times. By using a cached copy, we could save at least 200 gas.

//Original Code:
        communityCount++;
        CommunityStruct storage _community = _communities[communityCount];
        emit CommunityAdded(communityCount, _sender, _currency, _hash);

//Can be optimized into:
        uint256 communityCountCached = ++communityCount;  // cache the state variable.
        CommunityStruct storage _community = _communities[communityCountCached];
        emit CommunityAdded(communityCountCached, _sender, _currency, _hash);

Comparison of Hardhat gas reports, before and after the above changes, showed a gas saving of 194 gas.

2. The lendToProject function reads the projectDetails from the community mapping 6 times. If a value inside a mapping/array is accessed more than once, gas can be saved by caching the mapping's value in local storage. The gas saving comes from not needing to recalculate the key's keccak256.

See the mitigation below:

//Cache in local storage to avoid hash calculations
      ProjectDetails storage _communityProject = _communities[_communityID].projectDetails[
            _project];

//The following lines are modified to use the above variable. 
        require(                                            // Line 400
            _amountToProject <=
                _communityProject
                    .lendingNeeded -
                    _communityProject
                        .totalLent,
            "Community::lending>needed"
        );

        // Update total lent by lender
        _communityProject                                    // Line 421
            .totalLent += _amountToProject;

        // First claim interest if principal lent > 0
            _communityProject.lentAmount > 0                 // Line 427


        // Increment lent principal
        _communityProject                                     // Line 433
            .lentAmount += _lendingAmount;

        // Update lastTimestamp
        _communityProject                                     // Line 438
            .lastTimestamp = block.timestamp;

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 813 gas.

3. <ins>Project.sol</ins>

1. In the allocateFunds function: a. The length of state variable _changeOrderedTask is read multiple times. This can be cached to save gas. b. The state variable taskCount was being read inside a for loop. This should be cached.

See the mitigation below and affected lines:

//Mitigation:
        uint256 len = _changeOrderedTask.length;  // cache the length of state variable 
        
        uint256 taskCountCached = taskCount;    // cache the state variable 

//The following lines change as follows:
        taskCount - j + len - i                 //Line 592

        if (len > 0) {                          //Line 601

        for (; i < len; i++) {                  //Line 603

        if (i == len) {                         //Line 635

        if (j < taskCountCached) {              //Line 648
            
        for (++j; j <= taskCountCached; j++) {  //Line 650
   
        if (j > taskCountCached) {              //Line 681

        lastAllocatedTask = taskCountCached;    //Line 682

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 293 gas.

2. Emit memory variables instead of state variables whenever possible:

In the inviteContractor function, the emit statement is emitting a state variable called contractor, which was just assigned from a memory variable called _contractor. The following mitigation will save around 100 gas.

// change
emit ContractorInvited(contractor);
// to
emit ContractorInvited(_contractor);

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 102 gas.

4. <ins>Use custom errors instead of revert strings</ins>

Another major area in which we could save deployment cost would be in converting the revert strings into custom errors. If the function does revert, you can also save on dynamic gas cost. See this example implementation to understand the dynamics of custom errors. It shows that each require string converted into a custom error saves you around 11000 gas.

To make sure that it is indeed the case, I changed all the require error messages in the Disputes.sol file into custom errors.

There were 6 of them. And it reduced the deployment cost from 1,340,217 to 1,284,195 ( a total saving 56,022 gas). So we can give it a conservative estimate of 9000 saved gas per custom error.

See below how I changed the existing errors into Custom errors. You can see that, custom errors doesn't always look ugly as many developers feel.

    error Disputes_NotResolvable();
    error Disputes_NotActionType();
    error Disputes_NotMember();
    error Disputes_0address();
    error Disputes_NotAdmin();
    error Disputes_NotProject();

 	require(_address != address(0), "Disputes::0 address");
	//became...
	if(_address == address(0))
            revert Disputes_0address();


  	require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
	//became...
	if(homeFi.admin() != _msgSender())
            revert Disputes_NotAdmin();


	require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
	//became...
	if(!homeFi.isProjectExist(_msgSender()))
            revert Disputes_NotProject();



	require(
            _disputeID < disputeCount &&
                disputes[_disputeID].status == Status.Active,
            "Disputes::!Resolvable"
        );
 	//became...
	if(!(_disputeID < disputeCount &&
                disputes[_disputeID].status == Status.Active))
             revert Disputes_NotResolvable();


        require(
            _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
            "Disputes::!ActionType"
        );
	//became...
	if(!(_actionType > 0 && _actionType <= uint8(ActionType.TaskPay)))
                revert Disputes_NotActionType();


	require(_result, "Disputes::!Member");
     	//became...
     	if(!_result)
            revert Disputes_NotMember();

Listing out all the require strings which could be converted into custom errors:

File:     contracts/Disputes.sol
---------------------------------

Line 039:       require(_address != address(0), "Disputes::0 address");
Line 046:       require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Line 052:       require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Line 061:       require(
                      _disputeID < disputeCount &&
                          disputes[_disputeID].status == Status.Active,
                      "Disputes::!Resolvable"
                  );
Line 106:       require(
                      _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
                      "Disputes::!ActionType"
                  );
Line 183:       require(_result, "Disputes::!Member"); 
 

File:     contracts/Project.sol
---------------------------------

Line 123:       require(!contractorConfirmed, "Project::GC accepted");
Line 132:       require(_projectAddress == address(this), "Project::!projectAddress");
Line 135:       require(_contractor != address(0), "Project::0 address");
Line 150:       require(_msgSender() == builder, "Project::!B");
Line 153:       require(contractor != address(0), "Project::0 address");
Line 176:       require(_nonce == hashChangeNonce, "Project::!Nonce");
Line 189:       require(
                      _sender == builder || _sender == homeFi.communityContract(),
                      "Project::!Builder&&!Community"
                  );
Line 195:       require(_cost > 0, "Project::!value>0");
Line 199:       require(
                    projectCost() >= uint256(_newTotalLent),
                    "Project::value>required"
                );
Line 238:       require(_taskCount == taskCount, "Project::!taskCount");
Line 241:       require(_projectAddress == address(this), "Project::!projectAddress");
Line 245:       require(_length == _taskCosts.length, "Project::Lengths !match");
Line 277:       require(_nonce == hashChangeNonce, "Project::!Nonce");
Line 301:       require(
                    _msgSender() == builder || _msgSender() == contractor,
                    "Project::!Builder||!GC"
                );
Line 308:       require(_length == _scList.length, "Project::Lengths !match");
Line 341:       require(_projectAddress == address(this), "Project::!Project");
Line 369:       require(tasks[_taskID].getState() == 3, "Project::!Complete");
Line 406:       require(_project == address(this), "Project::!projectAddress");
Line 511:       require(_project == address(this), "Project::!projectAddress");
Line 515:       require(
                      signer == builder || signer == contractor,
                      "Project::!(GC||Builder)"
                  );
Line 521:       require(
                    signer == builder ||
                        signer == contractor ||
                        signer == tasks[_task].subcontractor,
                    "Project::!(GC||Builder||SC)"
                );
Line 530:       require(getAlerts(_task)[2], "Project::!SCConfirmed");
Line 753:       require(_sc != address(0), "Project::0 address");
Line 886:       require(
                      _recoveredSignature == _address || approvedHashes[_address][_hash],
                      "Project::invalid signature"
                  );
Line 906:       require(
                      ((_amount / 1000) * 1000) == _amount,
                      "Project::Precision>=1000"
                  );


File:     contracts/Community.sol
-----------------------------------
   
Line 069:       require(_address != address(0), "Community::0 address");
Line 075:       require(_msgSender() == homeFi.admin(), "Community::!admin");
Line 081:       require(
                    projectPublished[_project] == _communityID,
                    "Community::!published"
                );
Line 090:       require(
                      _msgSender() == IProject(_project).builder(),
                      "Community::!Builder"
                  );
Line 131:       require(
                    !restrictedToAdmin || _sender == homeFi.admin(),
                    "Community::!admin"
                );
Line 159:       require(
                  _communities[_communityID].owner == _msgSender(),
                  "Community::!owner"
              );
Line 191:       require(
                  !_community.isMember[_newMemberAddr],
                  "Community::Member Exists"
              );
Line 235:       require(
                    _publishNonce == _community.publishNonce,
                    "Community::invalid publishNonce"
                ); 
Line 241:       require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Line 248:       require(_community.isMember[_builder], "Community::!Member");
Line 251:       require(
                    _projectInstance.currency() == _community.currency,
                    "Community::!Currency"
                );
Line 312:       require(
                    !_communityProject.publishFeePaid,
                    "Community::publish fee paid"
                );
Line 347:       require(
                    _communityProject.publishFeePaid,
                    "Community::publish fee !paid"
                );
Line 353:       require(
                    _lendingNeeded >= _communityProject.totalLent &&
                        _lendingNeeded <= IProject(_project).projectCost(),
                    "Community::invalid lending"
                );
Line 384:       require(
                    _sender == _communities[_communityID].owner,
                    "Community::!owner"
                );
Line 400:       require(
                    _amountToProject <=
                        _communities[_communityID]
                            .projectDetails[_project]
                            .lendingNeeded -
                            _communities[_communityID]
                                .projectDetails[_project]
                                .totalLent,
                    "Community::lending>needed"
                );
Line 491:       require(
                    _msgSender() == _communities[_communityID].owner,
                    "Community::!Owner"
                );
Line 536:       require(_builder == _projectInstance.builder(), "Community::!Builder");
Line 539:       require(
                    _lender == _communities[_communityID].owner,
                    "Community::!Owner"
                );
Line 557:       require(!restrictedToAdmin, "Community::restricted");
Line 568:       require(restrictedToAdmin, "Community::!restricted");
Line 764:       require(_repayAmount > 0, "Community::!repay");
Line 792:       require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
Line 886:       require(
                      _recoveredSignature == _address || approvedHashes[_address][_hash],
                      "Community::invalid signature"
                  );    


File:     contracts/HomeFi.sol
--------------------------------

Line 073:       require(admin == _msgSender(), "HomeFi::!Admin"); 
Line 078:       require(_address != address(0), "HomeFi::0 address");
Line 084:       require(_oldAddress != _newAddress, "HomeFi::!Change");
Line 142:       require(!addrSet, "HomeFi::Set");
Line 191:       require(lenderFee != _newLenderFee, "HomeFi::!Change");
Line 255:       require(
                      _currency == tokenCurrency1 ||
                          _currency == tokenCurrency2 ||
                          _currency == tokenCurrency3,
                      "HomeFi::!Currency"
                  );


File:     contracts/ProjectFactory.sol
---------------------------------------

Line 036:       require(_address != address(0), "PF::0 address");               
Line 064:       require(
                    _msgSender() == IHomeFi(homeFi).admin(),
                    "ProjectFactory::!Owner"
                ); 
Line 084:       require(_msgSender() == homeFi, "PF::!HomeFiContract");  
     

File:     contracts/DebtToken.sol
-----------------------------------

Line 031:       require(
                    communityContract == _msgSender(),
                    "DebtToken::!CommunityContract"
                );            
Line 050:       require(_communityContract != address(0), "DebtToken::0 address");
            

File:     contracts/HomeFiProxy.sol
------------------------------------

Line 041:       require(_address != address(0), "Proxy::0 address");
Line 081:       require(_length == _implementations.length, "Proxy::Lengths !match");
Line 105:       require(
                    contractAddress[_contractName] == address(0),
                    "Proxy::Name !OK"
                );
Line 133:       require(_length == _contractAddresses.length, "Proxy::Lengths !match");

There are a total of 6 + 25 + 24 + 6 + 3 + 2 + 4 = 70 require strings spread out in 7 files. Converting all of them into custom errors can save us around 70 * 9000 = 630,000 gas when deploying.

Conclusions:

The above changes reduced the deployment cost by 691,860. And Dynamic cost was reduced by ‭1,747‬.