Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #258

Open
code423n4 opened this issue Aug 6, 2022 · 0 comments
Open

Gas Optimizations #258

code423n4 opened this issue Aug 6, 2022 · 0 comments
Labels

Comments

@code423n4
Copy link
Contributor

Summary

Gas savings are estimated using existing tests and may differ depending on the implementation of the fix. I keep my version of the fix for each finding and can provide them if you need.


Gas Optimizations

Issue Instances Estimated gas(deployments)
1 Use Custom Errors instead of Revert/Require Strings to save Gas 76 582064
2 Increment optimization 32 5388 / 23781 / 56517
3 Using bools for storage incurs overhead 7 50164
4 Call to an external contract should be cached. 1 23972
5 The same modifier can be combined into a library 5 8674
6 Loop length can be cached to save gas 2 4113
7 The variable is cached and used only once. 2 1738
8 It is more gas efficient to compare uint variables with != 0 than it is with > 0 10 1533
9 Splitting require() statements that use && saves gas 3 -

Total: 138 instances over 9 issues


  1. Use Custom Errors instead of Revert/Require Strings to save Gas (76 instances)

    Deployment gas saved: 582064

    Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

    Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string.
    Additional gas is saved due to the lack of defining string. https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth


    31          require(
    32              communityContract == _msgSender(),
    33              "DebtToken::!CommunityContract"
    34          ); //@audit replace with revert error
    ...
    50          require(_communityContract != address(0), "DebtToken::0 address"); //@audit replace with revert error
    ...
    96          revert("DebtToken::blocked"); //@audit use custom error
    ...
    104         revert("DebtToken::blocked"); //@audit use custom error
    • 2022-08-rigor/contracts/ProjectFactory.sol:36,64-67,84
    36          require(_address != address(0), "PF::0 address"); //@audit replace with revert error
    ...
    64          require(
    65               _msgSender() == IHomeFi(homeFi).admin(),
    66               "ProjectFactory::!Owner"
    67           ); //@audit replace with revert error
    ...
    84          require(_msgSender() == homeFi, "PF::!HomeFiContract"); //@audit replace with revert error
    41          require(_address != address(0), "Proxy::0 address"); //@audit replace with revert error
    ...
    81          require(_length == _implementations.length, "Proxy::Lengths !match"); //@audit replace with revert error
    ...
    105         require(
    106               contractAddress[_contractName] == address(0),
    107               "Proxy::Name !OK"
    108         ); //@audit replace with revert error
    ...
    133         require(_length == _contractAddresses.length, "Proxy::Lengths !match"); //@audit replace with revert error
    39          require(_address != address(0), "Disputes::0 address"); //@audit replace with revert error
    ...
    46          require(homeFi.admin() == _msgSender(), "Disputes::!Admin"); //@audit replace with revert error
    ...
    52          require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project"); //@audit replace with revert error
    ...
    61          require(
    62                _disputeID < disputeCount &&
    63                   disputes[_disputeID].status == Status.Active,
    64                "Disputes::!Resolvable"
    65          ); //@audit replace with revert error
    ...
    106         require(
    107               _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
    108               "Disputes::!ActionType"
    109         ); //@audit replace with revert error
    ...
    183         require(_result, "Disputes::!Member"); //@audit replace with revert error
    73          require(admin == _msgSender(), "HomeFi::!Admin"); //@audit replace with revert error
    ...
    78          require(_address != address(0), "HomeFi::0 address"); //@audit replace with revert error
    ...
    84          require(_oldAddress != _newAddress, "HomeFi::!Change"); //@audit replace with revert error
    ...
    142         require(!addrSet, "HomeFi::Set"); //@audit replace with revert error
    ...
    191         require(lenderFee != _newLenderFee, "HomeFi::!Change"); //@audit replace with revert error
    ...
    255         require(
    256            _currency == tokenCurrency1 ||
    257                  _currency == tokenCurrency2 ||
    258                  _currency == tokenCurrency3,
    259             "HomeFi::!Currency"
    260         ); //@audit replace with revert error
    123         require(!contractorConfirmed, "Project::GC accepted"); //@audit replace with revert error
    ...
    132         require(_projectAddress == address(this), "Project::!projectAddress"); //@audit replace with revert error
    ...
    135         require(_contractor != address(0), "Project::0 address"); //@audit replace with revert error
    ...
    150         require(_msgSender() == builder, "Project::!B"); //@audit replace with revert error
    ...
    153         require(contractor != address(0), "Project::0 address"); //@audit replace with revert error
    ...
    176         require(_nonce == hashChangeNonce, "Project::!Nonce"); //@audit replace with revert error
    ...
    189         require(
    190               _sender == builder || _sender == homeFi.communityContract(),
    191               "Project::!Builder&&!Community"
    192         ); //@audit replace with revert error @audit logical improvment for separate
    ...
    195         require(_cost > 0, "Project::!value>0"); //@audit replace with revert error @audit rewrite with !=
    ...
    199         require(
    200            projectCost() >= uint256(_newTotalLent),
    201            "Project::value>required"
    202         ); //@audit replace with revert error
    ...
    238         require(_taskCount == taskCount, "Project::!taskCount"); //@audit replace with revert error
    ...
    241         require(_projectAddress == address(this), "Project::!projectAddress"); //@audit replace with revert errorwith !=
    ...
    245         require(_length == _taskCosts.length, "Project::Lengths !match"); //@audit replace with revert error
    ...
    277         require(_nonce == hashChangeNonce, "Project::!Nonce"); //@audit replace with revert error
    ...
    301         require(
    302               _msgSender() == builder || _msgSender() == contractor,
    303               "Project::!Builder||!GC"
    304         ); //@audit replace with revert error @audit logical improvment for separate
    ...
    308         require(_length == _scList.length, "Project::Lengths !match"); //@audit replace with revert error
    ...
    341         require(_projectAddress == address(this), "Project::!Project"); //@audit replace with revert error
    ...
    369         require(tasks[_taskID].getState() == 3, "Project::!Complete"); //@audit replace with revert error
    ...
    406         require(_project == address(this), "Project::!projectAddress"); //@audit replace with revert error
    ...
    511         require(_project == address(this), "Project::!projectAddress"); //@audit replace with revert error
    ...
    515         require(
    516            signer == builder || signer == contractor,
    517            "Project::!(GC||Builder)"
    518         ); //@audit replace with revert error @audit try to optimize
    ...
    521         require(
    522            signer == builder ||
    523               signer == contractor ||
    524               signer == tasks[_task].subcontractor,
    525            "Project::!(GC||Builder||SC)"
    526         ); //@audit replace with revert error //@audit try to optimize
    ...
    530         require(getAlerts(_task)[2], "Project::!SCConfirmed"); //@audit replace with revert error
    ...
    753         require(_sc != address(0), "Project::0 address"); //@audit replace with revert error
    ...
    886         require(
    887               _recoveredSignature == _address || approvedHashes[_address][_hash],
    888               "Project::invalid signature"
    889         ); //@audit replace with revert error @audit separate
    ...
    906         require(
    907               ((_amount / 1000) * 1000) == _amount,
    908               "Project::Precision>=1000"
    909         ); //@audit replace with revert error
    69          require(_address != address(0), "Community::0 address"); //@audit replace with revert error
    ...
    75          require(_msgSender() == homeFi.admin(), "Community::!admin"); //@audit replace with revert error
    ...
    81          require(
    82                projectPublished[_project] == _communityID,
    83                "Community::!published"
    84          ); //@audit replace with revert error
    ...
    90          require(
    91                _msgSender() == IProject(_project).builder(),
    92                "Community::!Builder"
    93          ); //@audit replace with revert error
    ...
    131         require(
    132               !restrictedToAdmin || _sender == homeFi.admin(),
    133               "Community::!admin"
    134         ); //@audit replace with revert error
    ...
    159         require(
    160               _communities[_communityID].owner == _msgSender(),
    161               "Community::!owner"
    162         ); //@audit replace with revert error
    ...
    191         require(
    192               !_community.isMember[_newMemberAddr],
    193               "Community::Member Exists"
    194         ); //@audit replace with revert error
    ...
    235         require(
    236               _publishNonce == _community.publishNonce,
    237               "Community::invalid publishNonce"
    238         ); //@audit replace with revert error
    ...
    241         require(homeFi.isProjectExist(_project), "Community::Project !Exists"); //@audit replace with revert error
    ...
    248         require(_community.isMember[_builder], "Community::!Member"); //@audit replace with revert error
    ...
    251         require(
    252               _projectInstance.currency() == _community.currency,
    253               "Community::!Currency"
    254         ); //@audit replace with revert error
    ...
    312         require(
    313               !_communityProject.publishFeePaid,
    314               "Community::publish fee paid"
    315         ); //@audit replace with revert error
    ...
    347         require(
    348               _communityProject.publishFeePaid,
    349               "Community::publish fee !paid"
    350         ); //@audit replace with revert error
    ...
    353         require(
    354               _lendingNeeded >= _communityProject.totalLent &&
    355                  _lendingNeeded <= IProject(_project).projectCost(),
    356               "Community::invalid lending"
    357         ); //@audit replace with revert error //@audit separate
    ...
    384         require(
    385               _sender == _communities[_communityID].owner,
    386               "Community::!owner"
    387         ); //@audit replace with revert error
    ...
    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         ); //@audit replace with revert error
    ...
    491         require(
    492               _msgSender() == _communities[_communityID].owner,
    493               "Community::!Owner"
    494         ); //@audit replace with revert error
    ...
    536         require(_builder == _projectInstance.builder(), "Community::!Builder"); //@audit replace with revert error
    ...
    539         require(
    540               _lender == _communities[_communityID].owner,
    541               "Community::!Owner"
    542         ); //@audit replace with revert error
    ...
    557         require(!restrictedToAdmin, "Community::restricted"); //@audit replace with revert error
    ...
    568         require(restrictedToAdmin, "Community::!restricted"); //@audit replace with revert error
    ...
    764         require(_repayAmount > 0, "Community::!repay"); //@audit replace with revert error
    ...
    792         require(_lentAndInterest >= _repayAmount, "Community::!Liquid"); //@audit replace with revert error
    ...
    886         require(
    887               _recoveredSignature == _address || approvedHashes[_address][_hash],
    888               "Community::invalid signature"
    889         ); //@audit replace with revert error
    • 2022-08-rigor/contracts/libraries/Tasks.sol:44,50,56-59,124
    44          require(_self.state == TaskStatus.Inactive, "Task::active"); //@audit replace with revert error
    ...
    50          require(_self.state == TaskStatus.Active, "Task::!Active"); //@audit replace with revert error
    ...
    56          require(
    57                _self.alerts[uint256(Lifecycle.TaskAllocated)],
    58                "Task::!funded"
    59          ); //@audit replace with revert error
    ...
    124         require(_self.subcontractor == _sc, "Task::!SC"); //@audit replace with revert error
  2. Increment optimization.

    1. Prefix increments are cheaper than postfix increments, especially when it's used in for-loops. Gas saved: 5388
    2. <x> = <x> + 1 even more efficient than increment. Gas saved: 23781
    3. Increment can be unchecked when used in a loop. Gas saved: 56517

    • 2022-08-rigor/contracts/HomeFi.sol:289
    289         projectCount += 1; //@audit use pc = pc + 1;
    • 2022-08-rigor/contracts/HomeFiProxy.sol:87,136
    87          for (uint256 i = 0; i < _length; i++) {//@audit increment and inicialize
    ...
    136         for (uint256 i = 0; i < _length; i++) {//@audit increment and inicialize
    179         hashChangeNonce += 1; //@audit revwrite hcn = hcn + 1;
    ...
    248         for (uint256 i = 0; i < _length; i++) {//@audit increment and inicialize
    ...
    250         _taskCount += 1; //@audit increment fix
    ...
    290         hashChangeNonce += 1; // @audit fix increment
    ...
    311         for (uint256 i = 0; i < _length; i++) {//@audit increment and inicialize
    ...
    322         for (uint256 i = 0; i < _length; i++) {//@audit increment and inicialize
    ...
    368         for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {//@audit increment
    ...
    431         totalAllocated -= _withdrawDifference; //@audit don't use short form
    ...
    440         totalAllocated += _newCost - _taskCost; //@audit don't use short form
    ...
    456         totalAllocated -= _taskCost; //@audit don't use short form
    ...
    603         for (; i < _changeOrderedTask.length; i++) {//@audit increment
    ...
    616         _costToAllocate -= _taskCost;
    ...
    625         _loopCount++; //@audit increment
    ...
    650         for (++j; j <= taskCount; j++) {//@audit increment
    ...
    663         _costToAllocate -= _taskCost; //@audit increment
    ...
    672         _loopCount++; //@audit increment
    ...
    686         lastAllocatedTask = --j; //@audit increment
    ...
    710         for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {//@audit increment
    711         _cost += tasks[_taskID].cost; //@audit increment
    ...
    772         totalLent -= _amount; //@audit increment
    140         communityCount++; //@audit increment
    ...
    266         _community.publishNonce = ++_community.publishNonce; //@audit increment
    ...
    423         .totalLent += _amountToProject;
    ...
    435         .lentAmount += _lendingAmount;
    ...
    624         for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {//@audit increment and inicialize
    ...
    798         _interest -= _repayAmount; //@audit increment
    • 2022-08-rigor/contracts/libraries/Tasks.sol:181
    181         for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i]; //@audit increment and inicialize
    • 2022-08-rigor/contracts/libraries/SignatureDecoder.sol:83
    83          v += 27;
    • 2022-08-rigor/contracts/Disputes.sol:121
    121         emit DisputeRaised(disputeCount++, _reason);
  3. Using bools for storage incurs overhead

    Deployment gas saved: 50164

    Important: this optimization affects the use of gas to call the method differently. Function call can become either cheaper or more expensive. Should be tested manually and analyzed according to which functions you consider more important.

    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 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


    • 2022-08-rigor/contracts/HomeFi.sol:50
    50    bool public override addrSet; //@audit bool usage//7363 deploy gas
    • 2022-08-rigor/contracts/HomeFiProxy.sol:30
    30      mapping(address => bool) internal contractsActive; //@audit bool usage//2995 deploy gas
    • 2022-08-rigor/contracts/Project.sol:68,78,84
    68      bool public override contractorConfirmed; //@audit bool usage//4785 deploy gas
    ...
    78      bool public override contractorDelegated; //@audit bool usage//10780 deploy gas
    ...
    84      mapping(address => mapping(bytes32 => bool)) public override approvedHashes; //@audit bool usage//3205 deploy gas
    • 2022-08-rigor/contracts/Community.sol:55,61
    55      bool public override restrictedToAdmin; //@audit bool usage //16308 deploy gas
    ...
    61      mapping(address => mapping(bytes32 => bool)) public override approvedHashes; //@audit bool usage//4728 deploy gas
  4. Call to an external contract should be cached.
    Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
    Gas saved: 23972 deploy gas


    • 2022-08-rigor/contractsCommunity.sol:392-394
    392     // Calculate lenderFee
    393     uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
    394         (_projectInstance.lenderFee() + 1000);

    fix:

    393     uint256 _lenderFee = _projectInstance.lenderFee();
    394     _lenderFee = (_lendingAmount * _lenderFee) / (_lenderFee + 1000);
  5. The same modifier can be combined into a library
    8674 gas saved.


    • 2022-08-rigor/contractsCommunity.sol:67-71
    67      modifier nonZero(address _address) {
    68          // Revert if _address zero address (0x00) (invalid)
    69          require(_address != address(0), "Community::0 address");
    70          _;
    71      }
    • 2022-08-rigor/contractsDisputes.sol:37-41
    37      modifier nonZero(address _address) {
    38          // Revert if _address zero address (0x00)
    39          require(_address != address(0), "Disputes::0 address");
    40          _;
    41      }
    • 2022-08-rigor/contractsHomeFi.sol:77-80
    77      modifier nonZero(address _address) {
    78          require(_address != address(0), "HomeFi::0 address");
    79          _;
    80      }
    • 2022-08-rigor/contractsHomeFiProxy.sol:40-43
    40      modifier nonZero(address _address) {
    41          require(_address != address(0), "Proxy::0 address");
    42          _;
    43      }
    • 2022-08-rigor/contractsProjectFactory.sol:34-38
    34      modifier nonZero(address _address) {
    35          // Ensure an address is not the zero address (0x00)
    36          require(_address != address(0), "PF::0 address");
    37          _;
    38      }
    • 2022-08-rigor/contractsDebtToken.sol:50
    50     require(_communityContract != address(0), "DebtToken::0 address");
    • 2022-08-rigor/contractsProject.sol:753
    753         require(_sc != address(0), "Project::0 address");
  6. Loop length is calculated for each iteration in loop, this can be cached to save gas

    Gas saved: 4113 deploy gas. + method call gas depending on loop length


    • 2022-08-rigor/contractsProject.sol:592,601,603
    592         taskCount - j + _changeOrderedTask.length - i
    ...
    601         if (_changeOrderedTask.length > 0) {
    ...
    603         for (; i < _changeOrderedTask.length; i++) {@audit loop length
    • 2022-08-rigor/contractsCommunity.sol:620,624
    620         _communities[_communityID].memberCount
    ...
    624         for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
  7. The variable is cached and used only once.
    Caching variables cost extra gas.
    Gas saved: 1738


    419     // Local variable for total cost allocated. For gas saving.
    420     uint256 _totalAllocated = totalAllocated;
    ...
    438     else if (totalLent - _totalAllocated >= _newCost - _taskCost) {
    • 2022-08-rigor/contractsCommunity.sol:533,536
    533     IProject _projectInstance = IProject(_project);
    ...
    536     require(_builder == _projectInstance.builder(), "Community::!Builder");
  8. It is more gas efficient to compare variables with != 0 than it is with > 0. These two comparisons are equivalent when the compared variable is an unsigned integer.

    Gas saved: 1533


    • 2022-08-rigor/contracts/HomeFi.sol:245
    245         return projectTokenId[_project] > 0; //@audit !=
    195         require(_cost > 0, "Project::!value>0");
    ...
    380         if (_leftOutTokens > 0) {//@audit replace with !=
    ...
    601         if (_changeOrderedTask.length > 0) {//@audit use !=
    ...
    691         if (_loopCount > 0) emit TaskAllocated(_tasksAllocated); //@audit !=0
    261         if (projectPublished[_project] > 0) {//@audit !=
    ...
    427         _communities[_communityID].projectDetails[_project].lentAmount > 0 //@audit !=
    ...
    764         require(_repayAmount > 0, "Community::!repay");
    ...
    840         if (_interestEarned > 0) {//@audit !=
    • 2022-08-rigor/contracts/Disputes.sol:107
    107         _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
  9. Splitting require() statements that use && saves gas
    Important: While two separate checks provide gas savings, deployment requires more gas due to code size and string usage. I can recommend to use this assertion logic optimization with if() revert Error() model


    • 2022-08-rigor/contractsCommunity.sol:353-357
    353        require(
    354            _lendingNeeded >= _communityProject.totalLent &&
    355                _lendingNeeded <= IProject(_project).projectCost(),
    356            "Community::invalid lending"
    357        );
    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        );
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Aug 6, 2022
code423n4 added a commit that referenced this issue Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants