Rigor Protocol contest - c3phas's results

QA

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: Community.sol Line 394 1000

            (_projectInstance.lenderFee() + 1000);

File: Community.sol Line 686 86400

            _communityProject.lastTimestamp) / 86400; // 24*60*60

File: Community.sol Line 694 365000

                365000;

File: Project.sol Line 907

unnecessary casting

The type of the variable is the same as the type to which the variable is being cast to File: Project.sol Line 199

        require(
            projectCost() >= uint256(_newTotalLent),
            "Project::value>required"
        );

The variable _newTotalLent is already of type uint256 so no need to convert it again

Lack of event emission after critical initialize() functions

File: Community.sol Line: 102-119

    function initialize(address _homeFi)
        external
        override
        initializer
        nonZero(_homeFi)
    {
        // Initialize pausable. Set pause to false;
        __Pausable_init();


        // Initialize variables
        homeFi = IHomeFi(_homeFi);
        tokenCurrency1 = homeFi.tokenCurrency1();
        tokenCurrency2 = homeFi.tokenCurrency2();
        tokenCurrency3 = homeFi.tokenCurrency3();


        // Community creation is paused for non admin by default paused
        restrictedToAdmin = true;
    }

File: Project.sol Line 94-105

    function initialize(
        address _currency,
        address _sender,
        address _homeFiAddress
    ) external override initializer {
        // Initialize variables
        homeFi = IHomeFi(_homeFiAddress);
        disputes = homeFi.disputesContract();
        lenderFee = homeFi.lenderFee();
        builder = _sender;
        currency = IDebtToken(_currency);
    }

File: DebtToken.sol Line 43-58

    function initialize(
        address _communityContract,
        string memory name_,
        string memory symbol_,
        uint8 decimals_
    ) external override initializer {
        // Revert if _communityContract is zero address. Invalid address.
        require(_communityContract != address(0), "DebtToken::0 address");
        // Initialize ERC20
        __ERC20_init(name_, symbol_);
        // Store details
        _decimals = decimals_;
        communityContract = _communityContract;
    }

File: ProjectFactory.sol Line 45-55

    function initialize(address _underlying, address _homeFi)
        external
        override
        initializer
        nonZero(_underlying)
        nonZero(_homeFi)
    {
        // Store details
        underlying = _underlying;
        homeFi = _homeFi;
    }

File: Disputes.sol Line 74-81

    function initialize(address _homeFi)
        external
        override
        initializer
        nonZero(_homeFi)
    {
        homeFi = IHomeFi(_homeFi);
    }

File: HomeFi.sol Line 92 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L92-L120

Unused named return

Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those.

File: Project.sol Line 716-723

    function getAlerts(uint256 _taskID)
        public
        view
        override
        returns (bool[3] memory _alerts)
    {
        return tasks[_taskID].getAlerts();
    }

FINDINGS

NB: Some functions have been truncated where neccessary to just show affected parts of the code

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3gas) Storage value should get cached in memory

File: HomeFi.sol Line 228

HomeFi.sol.createProject(): projectCount should be cached(Saves ~ 71 gas)

Average gas before caching = 339543 Average gas after caching = 339472

    function createProject(bytes memory _hash, address _currency)
        external
        override
        nonReentrant
    {
        // Revert if currency not supported by HomeFi
        validCurrency(_currency);
   // Update project related mappings
        projects[projectCount] = _project;  @audit: SLOAD 1
        projectTokenId[_project] = projectCount; @audit: SLOAD 2


        emit ProjectAdded(projectCount, _project, _sender, _currency, _hash); @audit: SLOAD 3
    }

projectCount is being read 3 times in the following lines

SLOAD 1 Line 228 SLOAD 2 Line 229 SLOAD 3 Line 231

File: HomeFi.sol Line 284-297

HomeFi.sol.mintNFT(): projectCount should be cached

    function mintNFT(address _to, string memory _tokenURI)
        internal
        returns (uint256)
    {
        // Project count starts from 1
        projectCount += 1;


        // Mints NFT and set token URI
        _mint(_to, projectCount);
        _setTokenURI(projectCount, _tokenURI);


        emit NftCreated(projectCount, _to);
        return projectCount;
    }

File: Project.sol Line 176&179

Project.sol.updateProjectHash(): hashChangeNonce should be cached (saves ~ 101 gas)

Average gas before caching = 54538 Average gas after caching = 54437

    function updateProjectHash(bytes calldata _data, bytes calldata _signature)
        external
        override
    {
        // Revert if decoded nonce is incorrect. This indicates wrong _data.
        require(_nonce == hashChangeNonce, "Project::!Nonce"); @audit - SLOAD 1
        // Increment to ensure a set of data and signature cannot be re-used.
        hashChangeNonce += 1;@audit - SLOAD 2 and 
        emit HashUpdated(_hash);
    }

In the above function, there are two SLOADS that can be replaced with a cached variable. SLOAD 1: Line 176 SLOAD 2: Line 179

File: Project.sol Line 277&290

Project.sol.updateTaskHash(): hashChangeNonce should be cached(saves ~ 98 gas)

Average gas before caching = 58185 Average gas after caching = 58087

    function updateTaskHash(bytes calldata _data, bytes calldata _signature)
        external
        override
    {
        // Decode params from _data
        (bytes memory _taskHash, uint256 _nonce, uint256 _taskID) = abi.decode(
            _data,
            (bytes, uint256, uint256)
        );
        // Revert if decoded nonce is incorrect. This indicates wrong data.
        require(_nonce == hashChangeNonce, "Project::!Nonce");
       // Increment to ensure a set of data and signature cannot be re-used.
        hashChangeNonce += 1;
        emit TaskHashUpdated(_taskID, _taskHash);
    }

In the above function, there are two SLOADS that can be replaced with a cached variable. SLOAD 1: Line 277 SLOAD 2: Line 290

File: Project.sol Line 591-604

Project.sol.allocateFunds():_changeOrderedTask.length should be cached(Saves ~ 118 gas)

Average gas before caching = 63493 Average gas after caching = 63295

        uint256[] memory _tasksAllocated = new uint256[](
            taskCount - j + _changeOrderedTask.length - i @audit - SLOAD 1
        );
   // Number of times a loop has run.
        uint256 _loopCount;
        /// CHANGE ORDERED TASK FUNDING ///
        // Any tasks added to _changeOrderedTask will be allocated first
        if (_changeOrderedTask.length > 0) { @audit - SLOAD 2
            // Loop from lastAllocatedChangeOrderTask to _changeOrderedTask length (until _maxLoop)
            for (; i < _changeOrderedTask.length; i++) { @audit - Another SLOAD ;read repeatedly inside the loop
                // Local instance of task cost. To save gas.
				
		//truncated a big chunk of code here
635:   if (i == _changeOrderedTask.length) { @audit - another SLOAD

SLOAD 1: Line 592 SLOAD 2: Line 610 SLOAD 3: Read inside a for loopLine 603 SLOAD 4: Line 635 In the above function, the gas estimate might be higher than indicated due the SLOAD inside the for loop

File: Community.sol Line 143 & 150

Community.sol.createCommunity():communityCount should be cached( Saves ~186 gas)

Average gas before caching = 176852 Average gas after caching = 176666

   140: communityCount++; //@audit - SLOAD 1 + SSTORE(communityCount)


        // Store community details
   143: CommunityStruct storage _community = _communities[communityCount]; @audit - SLOAD 2(communityCount)
        _community.owner = _sender;
        _community.currency = IDebtToken(_currency);
		      ...
         @ audit - Truncated some bit of code here
   150: emit CommunityAdded(communityCount, _sender, _currency, _hash);@audit - SLOAD 3(communityCount)
    }

SLOAD 1: Line 140 SLOAD 2: Line 143 SLOAD 3: Line 150 Note, after creating a temp variable in the above , for line 140, after incrementing the temp variable we need to assign the temp value to communityCount

Cache the length of arrays in loops

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack):

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: Project.sol Line 603

Project.sol.allocateFunds(): _changeOrderedTask.length should be cached - _changeOrderedTask is a storage array

            for (; i < _changeOrderedTask.length; i++) {

This optimization is especially important if it is a storage array as it's our case here.

The above should be modified to

	uint256 length = _changeOrderedTask.length;
            for (; i < length; i++) {

++i costs less gas compared to i++ or i += 1 in for loops (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

File: HomeFiProxy.sol line 87

        for (uint256 i = 0; i < _length; i++) {
            _generateProxy(allContractNames[i], _implementations[i]);
        }

File: HomeFiProxy.sol line 136

        for (uint256 i = 0; i < _length; i++) {
            _replaceImplementation(_contractNames[i], _contractAddresses[i]);
        }

File: Project.sol Line 248

        for (uint256 i = 0; i < _length; i++) {

File: Project.sol Line 311

        for (uint256 i = 0; i < _length; i++) {
            _inviteSC(_taskList[i], _scList[i], false);
        }

File: Project.sol Line 322

        for (uint256 i = 0; i < _length; i++) {

File: Tasks.sol Line 181

        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

++x is more efficient than x++(Saves ~6 gas)

File: Community.sol Line 140 Average gas when using communityCount++ : 176852 Average gas when using ++communityCount : 176846

        communityCount++;

Other Instances File: Disputes.sol Line 121

        emit DisputeRaised(disputeCount++, _reason);

Splitting require() statements that use && saves gas - (saves 8 gas per &&)

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File: Disputes.sol Line 61

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

The above should be modified to

        require( _disputeID < disputeCount,  "Disputes::!Resolvable");
        require(disputes[_disputeID].status == Status.Active, "Disputes::!Resolvable");

File: Disputes.sol Line 106

        require(
            _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
            "Disputes::!ActionType"
        );

File: Community.sol Line 353

        require(
            _lendingNeeded >= _communityProject.totalLent &&
                _lendingNeeded <= IProject(_project).projectCost(),
            "Community::invalid lending"
        );

Proof The following tests were carried out in remix with both optimization turned on and off

	require ( a > 1 && a < 5, "Initialized");
	return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

	require (a > 1 ,"Initialized");
	require (a < 5 , "Initialized");
	return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

Comparisons: != is more efficient than > in require (6 gas less)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

File: Project.sol Line 195

        require(_cost > 0, "Project::!value>0");

File: Community.sol Line 764

        require(_repayAmount > 0, "Community::!repay");

Emitting storage values instead of the memory one(saves ~101 gas)

Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead:

File: Project.sol Line 144 average gas while using the storage value - 69561 average gas while using the memory value - 69460

        // Store new contractor
        contractor = _contractor;
        contractorConfirmed = true;


        // Check signature for builder and contractor
        checkSignature(_data, _signature);


        emit ContractorInvited(contractor);@audit - should emit _contractor instead of contractor
    }

In the above we should emit _contractor

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

File: Project.sol Line 427

       uint256 _withdrawDifference = _taskCost - _newCost;

The above operation cannot underflow due to the check on Line 425 which ensures that _taskCost is greater than _newCost before the subtraction operation is peformed The above can be modified as follows

       uint256 _withdrawDifference;
	     unchecked {
	        _withdrawDifference = _taskCost - _newCost;
	     }

File: Project.sol Line 616

           _costToAllocate -= _taskCost;

The above line cannot underflow due to the check on Line 614 which ensures that the above operation would only be performed if the value of _costToAllocate is greater than the value of _taskCost

File: Project.sol Line 663

            _costToAllocate -= _taskCost;

The above line cannot underflow due to the check on Line 661 which ensures that the above operation would only be performed if the value of _costToAllocate is greater than the value of _taskCost

File: Community.sol Line 794

            _lentAmount = _lentAndInterest - _repayAmount;

The above line cannot underflow due to the check on Line 792 which ensures that the above operation would only be performed if the value of _lentAndInterest is greater than the value of _repayAmount

File: Community.sol Line 798

            _interest -= _repayAmount;

The above line cannot underflow as it would only be evalauted if _interest is not less than _repayAmount . See Line 785

see resource

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File: HomeFiProxy.sol line 87

        for (uint256 i = 0; i < _length; i++) {
            _generateProxy(allContractNames[i], _implementations[i]);
        }

The above should be modified to:

        for (uint256 i = 0; i < _length;) {
            _generateProxy(allContractNames[i], _implementations[i]);
		unchecked {
			++i;
		}
        }

Other Instances to modify File: Project.sol Line 248

        for (uint256 i = 0; i < _length; i++) {

File: Project.sol Line 311

        for (uint256 i = 0; i < _length; i++) {
            _inviteSC(_taskList[i], _scList[i], false);
        }

File: Project.sol Line 322

        for (uint256 i = 0; i < _length; i++) {

File: Tasks.sol Line 181

        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

see resource

Use 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) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

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

File: DebtToken.sol line 31

        require(
            communityContract == _msgSender(),
            "DebtToken::!CommunityContract"
        );

File: DebtToken.sol line 50

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

File: DebtToken.sol line 96

        revert("DebtToken::blocked");

File: DebtToken.sol line 104

        revert("DebtToken::blocked");

File: ProjectFactory.sol line 36

        require(_address != address(0), "PF::0 address");

File: ProjectFactory.sol line 64

        require( _msgSender() == IHomeFi(homeFi).admin(), "ProjectFactory::!Owner" );

File: ProjectFactory.sol line 84

        require(_msgSender() == homeFi, "PF::!HomeFiContract");

File: HomeFiProxy.sol line 41 File: HomeFiProxy.sol line 81 File: HomeFiProxy.sol line 105 File: HomeFiProxy.sol line 133 File: Disputes.sol Line 39 File: Disputes.sol Line 46 File: Disputes.sol Line 52 File: Disputes.sol Line 61 File: Disputes.sol Line 106 File: Disputes.sol Line 183 File: Tasks.sol Line 124 File: Community.sol Line 69 File: Community.sol Line 75 File: Community.sol Line 81 File: Community.sol Line 90 File: Community.sol Line 131 File: Community.sol Line 159 File: Community.sol Line 191 File: Community.sol Line 235 File: Community.sol Line 241 File: Community.sol Line 248 File: Community.sol Line 251 File: Community.sol Line 312 File: Community.sol Line 353 File: Community.sol Line 886 File: Project.sol Line 123 File: Project.sol Line 132 File: Project.sol Line 135 File: Project.sol Line 176 File: Project.sol Line 238 File: Project.sol Line 241 File: Project.sol Line 245

x += y costs more gas than x = x + y for state variables

File: Project.sol Line 179

Project.sol.updateProjectHash() - (Saves ~19 gas)

Average gas before modification: 54538 Average gas after modification: 54519

        hashChangeNonce += 1;
    

The above should be modified to

        hashChangeNonce = hashChangeNonce + 1;

File: Project.sol Line 290

Project.sol.updateTaskHash() - (Saves ~19 gas)

Average gas before modification: 58185 Average gas after modification: 58166

        hashChangeNonce += 1;

File: HomeFi.sol Line 289

HomeFi.sol.mintNFT()

        projectCount += 1;

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Instances affected include File: HomeFiProxy.sol line 30

    mapping(address => bool) internal contractsActive;

File: Disputes.sol Line 144

        bool _ratify

File: HomeFi.sol Line 50

    bool public override addrSet;

File: Project.sol Line 68

    bool public override contractorConfirmed;

File: Project.sol Line 84

    mapping(address => mapping(bytes32 => bool)) public override approvedHashes;

File: Project.sol Line 412

        bool _unapproved = false;

File: Project.sol Line 582

        bool _exceedLimit;

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

File: Project.sol Line 60

    uint256 public constant override VERSION = 25000;

Not using the named return variables when a function returns, wastes deployment gas

File: Project.sol Line 716-723

    function getAlerts(uint256 _taskID)
        public
        view
        override
        returns (bool[3] memory _alerts)
    {
        return tasks[_taskID].getAlerts();
    }

File: Project.sol
438:                 else if (totalLent - _totalAllocated >= _newCost - _taskCost) {
439:                     // Increase the difference of new cost and old cost to total allocated.
440:                     totalAllocated += _newCost - _taskCost; //@audit should be unchecked due to L438