Gas Optimizations #176

code423n4 · 2022-08-06T09:42:41Z

Summary

Gas Optimizations

	Issue	Instances
[G‑01]	State variables can be packed into fewer storage slots	1
[G‑02]	Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas	4
[G‑03]	Avoid contract existence checks by using solidity version 0.8.10 or later	4
[G‑04]	State variables should be cached in stack variables rather than re-reading them from storage	25
[G‑05]	Multiple accesses of a mapping/array should use a local variable cache	26
[G‑06]	The result of external function calls should be cached rather than re-calling the function	1
[G‑07]	`<x> += <y>` costs more gas than `<x> = <x> + <y>` for state variables	7
[G‑08]	`internal` functions only called once can be inlined to save gas	7
[G‑09]	Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement	2
[G‑10]	`<array>.length` should not be looked up in every loop of a `for`-loop	1
[G‑11]	`++i`/`i++` should be `unchecked{++i}`/`unchecked{i++}` when it is not possible for them to overflow, as is the case when used in `for`- and `while`-loops	11
[G‑12]	Optimize names to save gas	2
[G‑13]	Using `bool`s for storage incurs overhead	7
[G‑14]	Using `> 0` costs more gas than `!= 0` when used on a `uint` in a `require()` statement	2
[G‑15]	`++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)	14
[G‑16]	Splitting `require()` statements that use `&&` saves gas	3
[G‑17]	Stack variable used as a cheaper cache for a state variable is only used once	1
[G‑18]	`require()` or `revert()` statements that check input arguments should be at the top of the function	1
[G‑19]	Use custom errors rather than `revert()`/`require()` strings to save gas	74
[G‑20]	Functions guaranteed to revert when called by normal users can be marked `payable`	23
[G‑21]	Don't use `_msgSender()` if not supporting EIP-2771	1

Total: 217 instances over 21 issues

Gas Optimizations

[G‑01] State variables can be packed into fewer storage slots

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper

There is 1 instance of this issue:

File: contracts/Project.sol

/// @audit Variable ordering with 15 slots instead of the current 16:
///           mapping(32):tasks, uint256[](32):_changeOrderedTask, uint256(32):lenderFee, uint256(32):hashChangeNonce, uint256(32):totalLent, uint256(32):totalAllocated, uint256(32):taskCount, uint256(32):lastAllocatedTask, uint256(32):lastAllocatedChangeOrderTask, mapping(32):approvedHashes, address(20):disputes, bool(1):contractorConfirmed, bool(1):contractorDelegated, user-defined(20):homeFi, user-defined(20):currency, address(20):builder, address(20):contractor
40:       address internal disputes;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L40

[G‑02] Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 4 instances of this issue:

File: contracts/Community.sol

/// @audit _details
484       function reduceDebt(
485           uint256 _communityID,
486           address _project,
487           uint256 _repayAmount,
488           bytes memory _details
489:      ) external virtual override whenNotPaused {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L484-L489

File: contracts/DebtToken.sol

/// @audit name_
/// @audit symbol_
43        function initialize(
44            address _communityContract,
45            string memory name_,
46            string memory symbol_,
47            uint8 decimals_
48:       ) external override initializer {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L43-L48

File: contracts/HomeFi.sol

/// @audit _hash
210       function createProject(bytes memory _hash, address _currency)
211           external
212           override
213:          nonReentrant

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L213

[G‑03] Avoid contract existence checks by using solidity version 0.8.10 or later

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

There are 4 instances of this issue:

File: contracts/Community.sol

/// @audit builder()
91:               _msgSender() == IProject(_project).builder(),

/// @audit projectCost()
355:                  _lendingNeeded <= IProject(_project).projectCost(),

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L91

File: contracts/ProjectFactory.sol

/// @audit admin()
65:               _msgSender() == IHomeFi(homeFi).admin(),

/// @audit isTrustedForwarder()
104:          return IHomeFi(homeFi).isTrustedForwarder(_forwarder);

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L65

[G‑04] State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 25 instances of this issue:

File: contracts/Community.sol

/// @audit communityCount on line 140
143:          CommunityStruct storage _community = _communities[communityCount];

/// @audit communityCount on line 143
150:          emit CommunityAdded(communityCount, _sender, _currency, _hash);

/// @audit _communities[_communityID].memberCount on line 620
624:          for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L143

File: contracts/Disputes.sol

/// @audit disputeCount on line 112
121:          emit DisputeRaised(disputeCount++, _reason);

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L121

File: contracts/HomeFi.sol

/// @audit projectCount on line 228
229:          projectTokenId[_project] = projectCount;

/// @audit projectCount on line 229
231:          emit ProjectAdded(projectCount, _project, _sender, _currency, _hash);

/// @audit projectCount on line 289
292:          _mint(_to, projectCount);

/// @audit projectCount on line 292
293:          _setTokenURI(projectCount, _tokenURI);

/// @audit projectCount on line 293
295:          emit NftCreated(projectCount, _to);

/// @audit projectCount on line 295
296:          return projectCount;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L229

File: contracts/Project.sol

/// @audit _changeOrderedTask on line 601
603:              for (; i < _changeOrderedTask.length; i++) {

/// @audit _changeOrderedTask on line 622
635:              if (i == _changeOrderedTask.length) {

/// @audit builder on line 190
204:          if (_sender == builder) {

/// @audit builder on line 516
522:                  signer == builder ||

/// @audit contractor on line 138
144:          emit ContractorInvited(contractor);

/// @audit contractor on line 798
807:                  checkSignatureValidity(contractor, _hash, _signature, 0);

/// @audit contractor on line 807
813:                  checkSignatureValidity(contractor, _hash, _signature, 1);

/// @audit contractor on line 842
852:                  checkSignatureValidity(contractor, _hash, _signature, 0);

/// @audit contractor on line 852
859:                  checkSignatureValidity(contractor, _hash, _signature, 1);

/// @audit totalLent on line 579
697:          totalAllocated = totalLent - _costToAllocate;

/// @audit taskCount on line 592
648:          if (j < taskCount) {

/// @audit taskCount on line 648
650:              for (++j; j <= taskCount; j++) {

/// @audit taskCount on line 650
681:              if (j > taskCount) {

/// @audit taskCount on line 681
682:                  lastAllocatedTask = taskCount;

/// @audit tasks[_task].subcontractor on line 524
528:              if (signer == tasks[_task].subcontractor) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L603

[G‑05] Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata

There are 26 instances of this issue:

File: contracts/Community.sol

/// @audit _communities[_communityID] on line 385
402                   _communities[_communityID]
403:                      .projectDetails[_project]

/// @audit _communities[_communityID] on line 402
405                       _communities[_communityID]
406:                          .projectDetails[_project]

/// @audit _communities[_communityID] on line 405
412:          IDebtToken _currency = _communities[_communityID].currency;

/// @audit _communities[_communityID] on line 412
421           _communities[_communityID]
422:              .projectDetails[_project]

/// @audit _communities[_communityID] on line 421
427:              _communities[_communityID].projectDetails[_project].lentAmount > 0

/// @audit _communities[_communityID] on line 427
433           _communities[_communityID]
434:              .projectDetails[_project]

/// @audit _communities[_communityID] on line 433
438           _communities[_communityID]
439:              .projectDetails[_project]

/// @audit _communities[_communityID] on line 471
474:          _communities[_communityID].currency.safeTransferFrom(

/// @audit _communities[_communityID] on line 620
624:          for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

/// @audit _communities[_communityID] on line 624
625:              _members[i] = _communities[_communityID].members[i];

/// @audit _communities[_communityID] on line 680
692:                  _communities[_communityID].projectDetails[_project].apr *

/// @audit _communities[_communityID] on line 836
837           ProjectDetails storage _communityProject = _communities[_communityID]
838:              .projectDetails[_project];

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L402-L403

File: contracts/Project.sol

/// @audit tasks[_taskID] on line 350
354:              tasks[_taskID].subcontractor,

/// @audit tasks[_taskID] on line 354
355:              tasks[_taskID].cost

/// @audit tasks[_taskID] on line 409
423:              if (tasks[_taskID].alerts[1]) {

/// @audit tasks[_taskID] on line 423
449:                      tasks[_taskID].unApprove();

/// @audit tasks[_taskID] on line 449
452:                      tasks[_taskID].unAllocateFunds();

/// @audit tasks[_taskID] on line 452
464:              tasks[_taskID].cost = _newCost;

/// @audit tasks[_taskID] on line 464
470:          if (_newSC != tasks[_taskID].subcontractor) {

/// @audit tasks[_taskID] on line 470
474:                  tasks[_taskID].unApprove();

/// @audit tasks[_taskID] on line 474
485:                  tasks[_taskID].subcontractor = address(0);

/// @audit tasks[_task] on line 524
528:              if (signer == tasks[_task].subcontractor) {

/// @audit tasks[id] on line 553
554:          subcontractor = tasks[id].subcontractor;

/// @audit tasks[id] on line 554
555:          state = tasks[id].state;

/// @audit tasks[<etc>] on line 605
619:                      tasks[_changeOrderedTask[i]].fundTask();

/// @audit tasks[j] on line 652
666:                      tasks[j].fundTask();

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L354

[G‑06] The result of external function calls should be cached rather than re-calling the function

The instances below point to the second+ call of the function within a single function

There is 1 instance of this issue:

File: contracts/Community.sol

/// @audit _projectInstance.lenderFee() on line 393
394:              (_projectInstance.lenderFee() + 1000);

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L394

[G‑07] `<x> += <y>` costs more gas than `<x> = <x> + <y>` for state variables

Using the addition operator instead of plus-equals saves 113 gas

There are 7 instances of this issue:

File: contracts/HomeFi.sol

289:          projectCount += 1;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L289

File: contracts/Project.sol

179:          hashChangeNonce += 1;

290:          hashChangeNonce += 1;

431:                      totalAllocated -= _withdrawDifference;

440:                      totalAllocated += _newCost - _taskCost;

456:                      totalAllocated -= _taskCost;

772:          totalLent -= _amount;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L179

[G‑08] `internal` functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 7 instances of this issue:

File: contracts/Disputes.sol

207:      function resolveHandler(uint256 _disputeID) internal {

236:      function executeTaskAdd(address _project, bytes memory _actionData)

254:      function executeTaskChange(address _project, bytes memory _actionData)

268:      function executeTaskPay(address _project, bytes memory _actionData)

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L207

File: contracts/HomeFiProxy.sol

197       function _replaceImplementation(
198           bytes2 _contractName,
199           address _contractAddress
200:      ) internal nonZero(_contractAddress) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L197-L200

File: contracts/HomeFi.sol

284       function mintNFT(address _to, string memory _tokenURI)
285           internal
286:          returns (uint256)

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L284-L286

File: contracts/Project.sol

770:      function autoWithdraw(uint256 _amount) internal {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L770

[G‑09] Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There are 2 instances of this issue:

File: contracts/Community.sol

/// @audit require() on line 792
794:              _lentAmount = _lentAndInterest - _repayAmount;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L794

File: contracts/Project.sol

/// @audit if-condition on line 425
427:                      uint256 _withdrawDifference = _taskCost - _newCost;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L427

[G‑10] `<array>.length` should not be looked up in every loop of a `for`-loop

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas)
memory arrays use MLOAD (3 gas)
calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There is 1 instance of this issue:

File: contracts/Project.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L603

[G‑11] `++i`/`i++` should be `unchecked{++i}`/`unchecked{i++}` when it is not possible for them to overflow, as is the case when used in `for`- and `while`-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 11 instances of this issue:

File: contracts/Community.sol

624:          for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L624

File: contracts/HomeFiProxy.sol

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

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L87

File: contracts/libraries/Tasks.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L181

File: contracts/Project.sol

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

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

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

368:              for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

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

650:              for (++j; j <= taskCount; j++) {

710:          for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L248

[G‑12] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There are 2 instances of this issue:

File: contracts/HomeFiProxy.sol

/// @audit initiateHomeFi(), addNewContract(), upgradeMultipleImplementations(), changeProxyAdminOwner(), isActive(), getLatestAddress()
14:   contract HomeFiProxy is OwnableUpgradeable {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L14

File: contracts/libraries/Tasks.sol

/// @audit initialize()
41:   library Tasks {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L41

[G‑13] Using `bool`s 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 7 instances of this issue:

File: contracts/Community.sol

55:       bool public override restrictedToAdmin;

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L55

File: contracts/HomeFiProxy.sol

30:       mapping(address => bool) internal contractsActive;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L30

File: contracts/HomeFi.sol

50:       bool public override addrSet;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L50

File: contracts/Project.sol

68:       bool public override contractorConfirmed;

78:       bool public override contractorDelegated;

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L68

[G‑14] Using `> 0` costs more gas than `!= 0` when used on a `uint` in a `require()` statement

This change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.

There are 2 instances of this issue:

File: contracts/Community.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L764

File: contracts/Project.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L195

[G‑15] `++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)

Saves 5 gas per loop

There are 14 instances of this issue:

File: contracts/Community.sol

140:          communityCount++;

624:          for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L140

File: contracts/HomeFiProxy.sol

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

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L87

File: contracts/libraries/Tasks.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L181

File: contracts/Project.sol

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

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

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

368:              for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

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

625:                      _loopCount++;

650:              for (++j; j <= taskCount; j++) {

672:                      _loopCount++;

710:          for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L248

[G‑16] Splitting `require()` statements that use `&&` saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas

There are 3 instances of this issue:

File: contracts/Community.sol

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L353-L357

File: contracts/Disputes.sol

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

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L61-L65

[G‑17] Stack variable used as a cheaper cache for a state variable is only used once

If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend

There is 1 instance of this issue:

File: contracts/Project.sol

420:              uint256 _totalAllocated = totalAllocated;

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L420

[G‑18] `require()` or `revert()` statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.

There is 1 instance of this issue:

File: contracts/Project.sol

/// @audit expensive op on line 190
195:          require(_cost > 0, "Project::!value>0");

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L195

[G‑19] Use custom errors rather than `revert()`/`require()` strings to save gas

Custom errors are available from solidity version 0.8.4. 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

There are 74 instances of this issue:

File: contracts/Community.sol

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

75:           require(_msgSender() == homeFi.admin(), "Community::!admin");

81            require(
82                projectPublished[_project] == _communityID,
83                "Community::!published"
84:           );

90            require(
91                _msgSender() == IProject(_project).builder(),
92                "Community::!Builder"
93:           );

131           require(
132               !restrictedToAdmin || _sender == homeFi.admin(),
133               "Community::!admin"
134:          );

159           require(
160               _communities[_communityID].owner == _msgSender(),
161               "Community::!owner"
162:          );

191           require(
192               !_community.isMember[_newMemberAddr],
193               "Community::Member Exists"
194:          );

235           require(
236               _publishNonce == _community.publishNonce,
237               "Community::invalid publishNonce"
238:          );

241:          require(homeFi.isProjectExist(_project), "Community::Project !Exists");

248:          require(_community.isMember[_builder], "Community::!Member");

251           require(
252               _projectInstance.currency() == _community.currency,
253               "Community::!Currency"
254:          );

312           require(
313               !_communityProject.publishFeePaid,
314               "Community::publish fee paid"
315:          );

347           require(
348               _communityProject.publishFeePaid,
349               "Community::publish fee !paid"
350:          );

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

384           require(
385               _sender == _communities[_communityID].owner,
386               "Community::!owner"
387:          );

400           require(
401               _amountToProject <=
402                   _communities[_communityID]
403                       .projectDetails[_project]
404                       .lendingNeeded -
405                       _communities[_communityID]
406                           .projectDetails[_project]
407                           .totalLent,
408               "Community::lending>needed"
409:          );

491           require(
492               _msgSender() == _communities[_communityID].owner,
493               "Community::!Owner"
494:          );

536:          require(_builder == _projectInstance.builder(), "Community::!Builder");

539           require(
540               _lender == _communities[_communityID].owner,
541               "Community::!Owner"
542:          );

557:          require(!restrictedToAdmin, "Community::restricted");

568:          require(restrictedToAdmin, "Community::!restricted");

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

792:              require(_lentAndInterest >= _repayAmount, "Community::!Liquid");

886           require(
887               _recoveredSignature == _address || approvedHashes[_address][_hash],
888               "Community::invalid signature"
889:          );

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L69

File: contracts/DebtToken.sol

31            require(
32                communityContract == _msgSender(),
33                "DebtToken::!CommunityContract"
34:           );

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L31-L34

File: contracts/Disputes.sol

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

46:           require(homeFi.admin() == _msgSender(), "Disputes::!Admin");

52:           require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");

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

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

183:          require(_result, "Disputes::!Member");

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L39

File: contracts/HomeFiProxy.sol

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

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

105           require(
106               contractAddress[_contractName] == address(0),
107               "Proxy::Name !OK"
108:          );

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L41

File: contracts/HomeFi.sol

73:           require(admin == _msgSender(), "HomeFi::!Admin");

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

84:           require(_oldAddress != _newAddress, "HomeFi::!Change");

142:          require(!addrSet, "HomeFi::Set");

191:          require(lenderFee != _newLenderFee, "HomeFi::!Change");

255           require(
256               _currency == tokenCurrency1 ||
257                   _currency == tokenCurrency2 ||
258                   _currency == tokenCurrency3,
259               "HomeFi::!Currency"
260:          );

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L73

File: contracts/libraries/Tasks.sol

44:           require(_self.state == TaskStatus.Inactive, "Task::active");

50:           require(_self.state == TaskStatus.Active, "Task::!Active");

56            require(
57                _self.alerts[uint256(Lifecycle.TaskAllocated)],
58                "Task::!funded"
59:           );

124:          require(_self.subcontractor == _sc, "Task::!SC");

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L44

File: contracts/ProjectFactory.sol

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

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

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

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L36

File: contracts/Project.sol

123:          require(!contractorConfirmed, "Project::GC accepted");

132:          require(_projectAddress == address(this), "Project::!projectAddress");

135:          require(_contractor != address(0), "Project::0 address");

150:          require(_msgSender() == builder, "Project::!B");

153:          require(contractor != address(0), "Project::0 address");

176:          require(_nonce == hashChangeNonce, "Project::!Nonce");

189           require(
190               _sender == builder || _sender == homeFi.communityContract(),
191               "Project::!Builder&&!Community"
192:          );

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

199           require(
200               projectCost() >= uint256(_newTotalLent),
201               "Project::value>required"
202:          );

238:          require(_taskCount == taskCount, "Project::!taskCount");

241:          require(_projectAddress == address(this), "Project::!projectAddress");

245:          require(_length == _taskCosts.length, "Project::Lengths !match");

277:          require(_nonce == hashChangeNonce, "Project::!Nonce");

301           require(
302               _msgSender() == builder || _msgSender() == contractor,
303               "Project::!Builder||!GC"
304:          );

308:          require(_length == _scList.length, "Project::Lengths !match");

341:          require(_projectAddress == address(this), "Project::!Project");

369:                  require(tasks[_taskID].getState() == 3, "Project::!Complete");

406:          require(_project == address(this), "Project::!projectAddress");

511:          require(_project == address(this), "Project::!projectAddress");

515               require(
516                   signer == builder || signer == contractor,
517                   "Project::!(GC||Builder)"
518:              );

521               require(
522                   signer == builder ||
523                       signer == contractor ||
524                       signer == tasks[_task].subcontractor,
525                   "Project::!(GC||Builder||SC)"
526:              );

530:                  require(getAlerts(_task)[2], "Project::!SCConfirmed");

753:          require(_sc != address(0), "Project::0 address");

886           require(
887               _recoveredSignature == _address || approvedHashes[_address][_hash],
888               "Project::invalid signature"
889:          );

906           require(
907               ((_amount / 1000) * 1000) == _amount,
908               "Project::Precision>=1000"
909:          );

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L123

[G‑20] Functions guaranteed to revert when called by normal users can be marked `payable`

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 23 instances of this issue:

File: contracts/Community.sol

285       function unpublishProject(uint256 _communityID, address _project)
286           external
287           override
288           whenNotPaused
289           isPublishedToCommunity(_communityID, _project)
290:          onlyProjectBuilder(_project)

297       function payPublishFee(uint256 _communityID, address _project)
298           external
299           override
300           nonReentrant
301           whenNotPaused
302           isPublishedToCommunity(_communityID, _project)
303:          onlyProjectBuilder(_project)

331       function toggleLendingNeeded(
332           uint256 _communityID,
333           address _project,
334           uint256 _lendingNeeded
335       )
336           external
337           override
338           whenNotPaused
339           isPublishedToCommunity(_communityID, _project)
340:          onlyProjectBuilder(_project)

455       function repayLender(
456           uint256 _communityID,
457           address _project,
458           uint256 _repayAmount
459       )
460           external
461           virtual
462           override
463           nonReentrant
464           whenNotPaused
465:          onlyProjectBuilder(_project)

555:      function restrictToAdmin() external override onlyHomeFiAdmin {

566:      function unrestrictToAdmin() external override onlyHomeFiAdmin {

577:      function pause() external override onlyHomeFiAdmin {

582:      function unpause() external override onlyHomeFiAdmin {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L285-L290

File: contracts/DebtToken.sol

61        function mint(address _to, uint256 _total)
62            external
63            override
64:           onlyCommunityContract

70        function burn(address _to, uint256 _total)
71            external
72            override
73:           onlyCommunityContract

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L61-L64

File: contracts/Disputes.sol

84        function raiseDispute(bytes calldata _data, bytes calldata _signature)
85            external
86            override
87:           onlyProject

141       function resolveDispute(
142           uint256 _disputeID,
143           bytes calldata _judgement,
144           bool _ratify
145:      ) external override onlyAdmin nonReentrant resolvable(_disputeID) {

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L84-L87

File: contracts/HomeFiProxy.sol

100       function addNewContract(bytes2 _contractName, address _contractAddress)
101           external
102:          onlyOwner

125       function upgradeMultipleImplementations(
126           bytes2[] calldata _contractNames,
127           address[] calldata _contractAddresses
128:      ) external onlyOwner {

150       function changeProxyAdminOwner(address _newAdmin)
151           external
152           onlyOwner
153:          nonZero(_newAdmin)

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L100-L102

File: contracts/HomeFi.sol

123       function setAddr(
124           address _projectFactory,
125           address _communityContract,
126           address _disputesContract,
127           address _hTokenCurrency1,
128           address _hTokenCurrency2,
129           address _hTokenCurrency3
130       )
131           external
132           override
133           onlyAdmin
134           nonZero(_projectFactory)
135           nonZero(_communityContract)
136           nonZero(_disputesContract)
137           nonZero(_hTokenCurrency1)
138           nonZero(_hTokenCurrency2)
139:          nonZero(_hTokenCurrency3)

157       function replaceAdmin(address _newAdmin)
158           external
159           override
160           onlyAdmin
161           nonZero(_newAdmin)
162:          noChange(admin, _newAdmin)

171       function replaceTreasury(address _newTreasury)
172           external
173           override
174           onlyAdmin
175           nonZero(_newTreasury)
176:          noChange(treasury, _newTreasury)

185       function replaceLenderFee(uint256 _newLenderFee)
186           external
187           override
188:          onlyAdmin

200       function setTrustedForwarder(address _newForwarder)
201           external
202           override
203           onlyAdmin
204:          noChange(trustedForwarder, _newForwarder)

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L123-L139

File: contracts/libraries/Tasks.sol

88        function setComplete(Task storage _self)
89            internal
90            onlyActive(_self)
91:           onlyFunded(_self)

106       function inviteSubcontractor(Task storage _self, address _sc)
107           internal
108:          onlyInactive(_self)

119       function acceptInvitation(Task storage _self, address _sc)
120           internal
121:          onlyInactive(_self)

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L88-L91

[G‑21] Don't use `_msgSender()` if not supporting EIP-2771

Use msg.sender if the code does not implement EIP-2771 trusted forwarder support

There is 1 instance of this issue:

File: contracts/DebtToken.sol

32:               communityContract == _msgSender(),

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L32

The text was updated successfully, but these errors were encountered:

IllIllI000 · 2022-08-31T21:07:54Z

@jack-the-pug Can you let me know why this report was ranked so low? Without any feedback I can't improve. Also, it feels like I'm being penalized for other people copying my work. I've spent months learning gas optimizations and figuring out rules, and this report #260 is ranked higher, even though it has fewer findings and copies my text word for word, including links to my gists (see Multiple accesses of a mapping/array should use a local variable cache in both reports, along with many others).

jack-the-pug · 2022-08-31T23:12:57Z

So, first thing first, there is nothing wrong with copying. This is the ethos of the whole Ethereum and the greater OSS community, and probably also the entire human civilization.

I recommend you to watch this great documentary Everything is a Remix which explained why and how copying is great.

And also this article Copying is the way design works, it's about design, but I think it also applies to everything else that involves human creativity.

Thank you for your work and I appreciate your dedication to standardizing the gas optimization know-hows.

You have the best formatting, solid proof, short but concrete description of the findings, and many other merits in your QA and Gas reports. That's some great work being done!

If I remember correctly, I have scored your reports quite a few times as the top QA/gas report among the contests I judged.

However, as you can see from #260 and many other reports, once a gas optimization rule/know-how is known to the community, there will be people (or robots) that submit them many times in duplication.

So I wonder what kind of scoring rules can better incentive good, practical, down-to-earth, REAL helpful gas optimization reports to the sponsor, instead of the ones that only work theoretically, and should not even be considered in most cases as they can hurt the readability.

My rules can be explained like this:

Robot-like/pattern-matching reports will be scored from 60 as the starting points;
If there are some non-practical/wrong/invalid findings, I'll subtract from there;
If there are some good/first-of-its-kind/practical findings, I'll add a few points;
I'll also consider the length of the report (short is better when it's pattern-matching findings), what kind of gas optimization the report includes, formatting, etc.

This whole scoring process is done in a quick-decision mode, it can be wrong by a little bit sometimes, but generally, that's what I'm trying to do.

So, for this report being scored 60 and #260 being scored 65 (not a super big difference), I believe it is because of this: [G‑21] Don't use _msgSender() if not supporting EIP-2771

As we know, EIP-2771 is being used across many contracts in this project, so this may just be invalid.

All in all, I have no problem with copying. Creative and original work is welcomed, copying is okay. But I do prefer the findings that considered the context of the project much more than the pattern-matching findings, because they are more practical and helpful to the sponsor.

Have a good day and keep up with the good work!

IllIllI000 · 2022-08-31T23:47:57Z

The copying has been going on for a while and I didn't mention it previously, because I agree with you that copying is fair game, however in this case it looked like some sort of drastic penalty was being applied, and without an explanation it was confusing. Thank you for your explanation, it makes sense now, and I'll continue improving my system

IllIllI000 · 2022-08-31T23:58:42Z

@jack-the-pug Regarding your comment about [G-21], it's invalid to use _msgSender() without also implementing isTrustedForwarder(), which the contract did not do, and that's why I flagged it. I linked to the EIP, but I guess I could have been clearer about the reason in my text. The EIP says, The Recipient MUST check that it trusts the Forwarder to prevent it from extracting address data appended from an untrusted contract. Does that make this finding first of its kind rather than invalid?

jack-the-pug · 2022-09-01T00:12:57Z

Yes, [G-12] is valid. Using _msgSender() in DebtToken is not necessary as it does not work. I've updated your score from 60 to 65.

But for the sake of consistency with other access control modifiers across the codebase, I think it's okay to keep it so.

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L72-L75

    modifier onlyAdmin() {
        require(admin == _msgSender(), "HomeFi::!Admin");
        _;
    }

code423n4 added bug Something isn't working G (Gas Optimization) old-submission-method labels Aug 6, 2022

code423n4 added a commit that referenced this issue Aug 6, 2022

IllIllI issue #176

7c74afc

jack-the-pug added the valid label Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas Optimizations #176

Gas Optimizations #176

code423n4 commented Aug 6, 2022

IllIllI000 commented Aug 31, 2022 •

edited

jack-the-pug commented Aug 31, 2022

IllIllI000 commented Aug 31, 2022

IllIllI000 commented Aug 31, 2022

jack-the-pug commented Sep 1, 2022

Gas Optimizations #176

Gas Optimizations #176

Comments

code423n4 commented Aug 6, 2022

Summary

Gas Optimizations

Gas Optimizations

[G‑01] State variables can be packed into fewer storage slots

[G‑02] Using calldata instead of memory for read-only arguments in external functions saves gas

[G‑03] Avoid contract existence checks by using solidity version 0.8.10 or later

[G‑04] State variables should be cached in stack variables rather than re-reading them from storage

[G‑05] Multiple accesses of a mapping/array should use a local variable cache

[G‑06] The result of external function calls should be cached rather than re-calling the function

[G‑07] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

[G‑08] internal functions only called once can be inlined to save gas

[G‑09] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

[G‑10] <array>.length should not be looked up in every loop of a for-loop

[G‑11] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

[G‑12] Optimize names to save gas

[G‑13] Using bools for storage incurs overhead

[G‑14] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

[G‑15] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

[G‑16] Splitting require() statements that use && saves gas

[G‑17] Stack variable used as a cheaper cache for a state variable is only used once

[G‑18] require() or revert() statements that check input arguments should be at the top of the function

[G‑19] Use custom errors rather than revert()/require() strings to save gas

[G‑20] Functions guaranteed to revert when called by normal users can be marked payable

[G‑21] Don't use _msgSender() if not supporting EIP-2771

IllIllI000 commented Aug 31, 2022 • edited

jack-the-pug commented Aug 31, 2022

IllIllI000 commented Aug 31, 2022

IllIllI000 commented Aug 31, 2022

jack-the-pug commented Sep 1, 2022

[G‑02] Using `calldata` instead of `memory` for read-only arguments in `external` functions saves gas

[G‑07] `<x> += <y>` costs more gas than `<x> = <x> + <y>` for state variables

[G‑08] `internal` functions only called once can be inlined to save gas

[G‑09] Add `unchecked {}` for subtractions where the operands cannot underflow because of a previous `require()` or `if`-statement

[G‑10] `<array>.length` should not be looked up in every loop of a `for`-loop

[G‑11] `++i`/`i++` should be `unchecked{++i}`/`unchecked{i++}` when it is not possible for them to overflow, as is the case when used in `for`- and `while`-loops

[G‑13] Using `bool`s for storage incurs overhead

[G‑14] Using `> 0` costs more gas than `!= 0` when used on a `uint` in a `require()` statement

[G‑15] `++i` costs less gas than `i++`, especially when it's used in `for`-loops (`--i`/`i--` too)

[G‑16] Splitting `require()` statements that use `&&` saves gas

[G‑18] `require()` or `revert()` statements that check input arguments should be at the top of the function

[G‑19] Use custom errors rather than `revert()`/`require()` strings to save gas

[G‑20] Functions guaranteed to revert when called by normal users can be marked `payable`

[G‑21] Don't use `_msgSender()` if not supporting EIP-2771

IllIllI000 commented Aug 31, 2022 •

edited