Gas Optimizations

[code423n4]

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();
    }

[JustDravee]

Overall a high quality gas report IMHO. The warden starts with the most manual and interesting findings: storage reading optimizations. There are also the unchecked blocks. The gas savings are almost always mentioned too.

Analysis:

• Cache storage values in memory to minimize SLOADs
  • HomeFi.sol.createProject(): projectCount should be cached(Saves ~ 71 gas)
  • HomeFi.sol.mintNFT(): projectCount should be cached
  • Project.sol.updateProjectHash(): hashChangeNonce should be cached (saves ~ 101 gas)
  • Project.sol.updateTaskHash(): hashChangeNonce should be cached(saves ~ 98 gas)
  • Project.sol.allocateFunds():_changeOrderedTask.length should be cached(Saves ~ 118 gas)
  • Community.sol.createCommunity():communityCount should be cached( Saves ~186 gas)

Valid

• Cache the length of arrays in loops
  • Project.sol.allocateFunds(): _changeOrderedTask.length should be cached - _changeOrderedTask is a storage array

Valid

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

Valid

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

Valid, kinda same as above (pre-increments)

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

Valid on Optimizer with 200 runs

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

Valid with Solidity 0.8.6 < 0.8.13

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

Valid

• Using unchecked blocks to save gas

Valid and well explained. I believe only 1 instance is missing in the solution:

• Project.sol#L440

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

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

Valid

• Use Custom Errors instead of Revert Strings to save Gas

Valid

• x += y costs more gas than x = x + y for state variables
  • Project.sol.updateProjectHash() - (Saves ~19 gas)
  • Project.sol.updateTaskHash() - (Saves ~19 gas)
  • HomeFi.sol.mintNFT()

Valid, but could've saved more gas with ++x instead of x += 1

• Using bools for storage incurs overhead

Valid but partially true as not all mentioned booleans are state booleans (some are memory ones or function arguments).

• Using private rather than public for constants, saves gas

I believe it's invalid here as this specific constant needs to be public.

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

From memory, this has actually been debunked (the optimizer takes care of it). So, invalid, but could be NC.

[jack-the-pug]

This is 💯!