Platform: Code4rena
Start Date: 01/08/2022
Pot Size: $50,000 USDC
Total HM: 26
Participants: 133
Period: 5 days
Judge: Jack the Pug
Total Solo HM: 6
Id: 151
League: ETH
Rank: 50/133
Findings: 2
Award: $88.10
š Selected for report: 0
š Solo Findings: 0
š Selected for report: Lambda
Also found by: 0x1f8b, 0x52, 0xA5DF, 0xNazgul, 0xNineDec, 0xSmartContract, 0xSolus, 0xf15ers, 0xkatana, 0xsolstars, 8olidity, Aymen0909, Bahurum, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Extropy, Funen, GalloDaSballo, Guardian, IllIllI, JC, Jujic, MEP, Noah3o6, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, SooYa, Soosh, Throne6g, TomJ, Tomio, TrungOre, Waze, Yiko, _Adam, __141345__, a12jmx, ajtra, ak1, arcoun, asutorufos, ayeslick, benbaessler, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, byndooa, c3phas, codexploder, cryptonue, cryptphi, defsec, delfin454000, dipp, djxploit, erictee, exd0tpy, fatherOfBlocks, gogo, hake, hansfriese, horsefacts, hyh, ignacio, indijanc, joestakey, kaden, mics, minhquanym, neumo, obront, oyc_109, p_crypt0, pfapostol, poirots, rbserver, robee, rokinot, rotcivegaf, sach1r0, saian, samruna, saneryee, scaraven, sikorico, simon135, sseefried, supernova
62.7082 USDC - $62.71
| Issue | Instances | |
|---|---|---|
| [Lā01] | _safeMint() should be used rather than _mint() wherever possible | 1 |
| [Lā02] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
| [Lā03] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 7 |
Total: 9 instances over 3 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | ecrecover() signature validity not checked | 1 |
| [Nā02] | Operation may run out of gas | 1 |
| [Nā03] | Missing initializer modifier on constructor | 4 |
| [Nā04] | The nonReentrant modifier should occur before all other modifiers | 1 |
| [Nā05] | public functions not called by the contract should be declared external instead | 1 |
| [Nā06] | constants should be defined rather than using magic numbers | 18 |
| [Nā07] | Numeric values having to do with time should use time units for readability | 1 |
| [Nā08] | Missing event and or timelock for critical parameter change | 1 |
| [Nā09] | Use a more recent version of solidity | 2 |
| [Nā10] | Inconsistent spacing in comments | 1 |
| [Nā11] | NatSpec is incomplete | 1 |
| [Nā12] | Not using the named return variables anywhere in the function is confusing | 4 |
| [Nā13] | Duplicated require()/revert() checks should be refactored to a modifier or function | 4 |
Total: 40 instances over 13 issues
_safeMint() should be used rather than _mint() wherever possible_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
There is 1 instance of this issue:
File: contracts/HomeFi.sol 292: _mint(_to, projectCount);
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: contracts/Project.sol 103: builder = _sender;
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There are 7 instances of this issue:
File: contracts/Community.sol 21 contract Community is 22 ICommunity, 23 PausableUpgradeable, 24 ReentrancyGuardUpgradeable, 25: ERC2771ContextUpgradeable
File: contracts/DebtToken.sol 11: contract DebtToken is IDebtToken, ERC20Upgradeable {
File: contracts/Disputes.sol 17 contract Disputes is 18 IDisputes, 19 ReentrancyGuardUpgradeable, 20: ERC2771ContextUpgradeable
File: contracts/HomeFiProxy.sol 14: contract HomeFiProxy is OwnableUpgradeable {
File: contracts/HomeFi.sol 27 contract HomeFi is 28 IHomeFi, 29 ReentrancyGuardUpgradeable, 30 ERC721URIStorageUpgradeable, 31: ERC2771ContextUpgradeable
File: contracts/ProjectFactory.sol 16 contract ProjectFactory is 17 IProjectFactory, 18 Initializable, 19: ERC2771ContextUpgradeable
File: contracts/Project.sol 24 contract Project is 25 IProject, 26 ReentrancyGuardUpgradeable, 27: ERC2771ContextUpgradeable
ecrecover() signature validity not checkedecrecover() returns the zero address if the signature is invalid. If the signer provided is also zero, then all incorrect signatures will be allowed
There is 1 instance of this issue:
File: /contracts/libraries/SignatureDecoder.sol 39: return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);
If the array is sufficiently large, the function call may run out of gas
There is 1 instance of this issue:
File: /contracts/Project.sol 711: _cost += tasks[_taskID].cost;
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There are 4 instances of this issue:
File: contracts/Community.sol 24: ReentrancyGuardUpgradeable,
File: contracts/Disputes.sol 19: ReentrancyGuardUpgradeable,
File: contracts/HomeFi.sol 29: ReentrancyGuardUpgradeable,
File: contracts/ProjectFactory.sol 18: Initializable,
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There is 1 instance of this issue:
File: contracts/Disputes.sol 145: ) external override onlyAdmin nonReentrant resolvable(_disputeID) {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There is 1 instance of this issue:
File: contracts/libraries/Tasks.sol 75: function initialize(Task storage _self, uint256 _cost) public {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 18 instances of this issue:
File: contracts/Community.sol /// @audit 1000 394: (_projectInstance.lenderFee() + 1000); /// @audit 86400 686: _communityProject.lastTimestamp) / 86400; // 24*60*60 /// @audit 365000 694: 365000;
File: contracts/libraries/SignatureDecoder.sol /// @audit 65 25: if (messageSignatures.length % 65 != 0) { /// @audit 27 /// @audit 28 35: if (v != 27 && v != 28) { /// @audit 0x41 75: let signaturePos := mul(0x41, pos) /// @audit 0x20 76: r := mload(add(signatures, add(signaturePos, 0x20))) /// @audit 0x40 77: s := mload(add(signatures, add(signaturePos, 0x40))) /// @audit 0x60 78: v := byte(0, mload(add(signatures, add(signaturePos, 0x60)))) /// @audit 27 82: if (v < 27) { /// @audit 27 83: v += 27;
File: contracts/libraries/Tasks.sol /// @audit 3 178: returns (bool[3] memory _alerts)
File: contracts/Project.sol /// @audit 3 369: require(tasks[_taskID].getState() == 3, "Project::!Complete"); /// @audit 50 576: uint256 _maxLoop = 50; /// @audit 3 720: returns (bool[3] memory _alerts) /// @audit 1000 /// @audit 1000 907: ((_amount / 1000) * 1000) == _amount,
There are units for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used
There is 1 instance of this issue:
File: contracts/Community.sol /// @audit 86400 686: _communityProject.lastTimestamp) / 86400; // 24*60*60
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There is 1 instance of this issue:
File: contracts/HomeFi.sol 200 function setTrustedForwarder(address _newForwarder) 201 external 202 override 203 onlyAdmin 204 noChange(trustedForwarder, _newForwarder) 205 { 206 trustedForwarder = _newForwarder; 207: }
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 2 instances of this issue:
File: contracts/Community.sol 3: pragma solidity 0.8.6;
File: contracts/Project.sol 3: pragma solidity 0.8.6;
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There is 1 instance of this issue:
File: contracts/Disputes.sol 29: uint256 public override disputeCount; //starts from 0
There is 1 instance of this issue:
File: contracts/libraries/SignatureDecoder.sol /// @audit Missing: '@return' 59 * @param signatures concatenated rsv signatures 60 */ 61 function signatureSplit(bytes memory signatures, uint256 pos) 62 internal 63 pure 64 returns ( 65 uint8 v, 66 bytes32 r, 67: bytes32 s
Consider changing the variable to be an unnamed one
There are 4 instances of this issue:
File: contracts/Community.sol /// @audit sender 899 function _msgSender() 900 internal 901 view 902 override(ContextUpgradeable, ERC2771ContextUpgradeable) 903: returns (address sender)
File: contracts/HomeFi.sol /// @audit sender 303 function _msgSender() 304 internal 305 view 306 override(ContextUpgradeable, ERC2771ContextUpgradeable) 307: returns (address sender)
File: contracts/libraries/Tasks.sol /// @audit _state 191 function getState(Task storage _self) 192 internal 193 view 194: returns (uint256 _state)
File: contracts/Project.sol /// @audit _alerts 716 function getAlerts(uint256 _taskID) 717 public 718 view 719 override 720: returns (bool[3] memory _alerts)
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 4 instances of this issue:
File: contracts/DebtToken.sol 104: revert("DebtToken::blocked");
File: contracts/Project.sol 241: require(_projectAddress == address(this), "Project::!projectAddress"); 277: require(_nonce == hashChangeNonce, "Project::!Nonce"); 511: require(_project == address(this), "Project::!projectAddress");
š Selected for report: c3phas
Also found by: 0x040, 0x1f8b, 0xA5DF, 0xNazgul, 0xSmartContract, 0xSolus, 0xc0ffEE, 0xkatana, 0xsam, 8olidity, Aymen0909, Bnke0x0, CertoraInc, Chinmay, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Extropy, Fitraldys, Funen, GalloDaSballo, Guardian, IllIllI, JC, Lambda, MEP, Metatron, MiloTruck, Noah3o6, NoamYakov, PaludoX0, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, SooYa, TomJ, Tomio, Waze, _Adam, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, ballx, benbaessler, bharg4v, bobirichman, brgltd, cryptonue, defsec, delfin454000, dharma09, djxploit, durianSausage, eierina, erictee, fatherOfBlocks, gerdusx, gogo, hake, hyh, ignacio, jag, kaden, kyteg, lucacez, mics, minhquanym, oyc_109, pfapostol, rbserver, ret2basic, robee, rokinot, sach1r0, saian, samruna, scaraven, sikorico, simon135, supernova, teddav, tofunmi, zeesaw
25.3906 USDC - $25.39
| 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 bools 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
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;
calldata instead of memory for read-only arguments in external functions saves gasWhen 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 {
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 {
File: contracts/HomeFi.sol /// @audit _hash 210 function createProject(bytes memory _hash, address _currency) 211 external 212 override 213: nonReentrant
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(),
File: contracts/ProjectFactory.sol /// @audit admin() 65: _msgSender() == IHomeFi(homeFi).admin(), /// @audit isTrustedForwarder() 104: return IHomeFi(homeFi).isTrustedForwarder(_forwarder);
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++) {
File: contracts/Disputes.sol /// @audit disputeCount on line 112 121: emit DisputeRaised(disputeCount++, _reason);
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;
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) {
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];
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();
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);
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 7 instances of this issue:
File: contracts/HomeFi.sol 289: projectCount += 1;
File: contracts/Project.sol 179: hashChangeNonce += 1; 290: hashChangeNonce += 1; 431: totalAllocated -= _withdrawDifference; 440: totalAllocated += _newCost - _taskCost; 456: totalAllocated -= _taskCost; 772: totalLent -= _amount;
internal functions only called once can be inlined to save gasNot 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)
File: contracts/HomeFiProxy.sol 197 function _replaceImplementation( 198 bytes2 _contractName, 199 address _contractAddress 200: ) internal nonZero(_contractAddress) {
File: contracts/HomeFi.sol 284 function mintNFT(address _to, string memory _tokenURI) 285 internal 286: returns (uint256)
File: contracts/Project.sol 770: function autoWithdraw(uint256 _amount) internal {
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(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;
File: contracts/Project.sol /// @audit if-condition on line 425 427: uint256 _withdrawDifference = _taskCost - _newCost;
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)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++) {
++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-loopsThe 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++) {
File: contracts/HomeFiProxy.sol 87: for (uint256 i = 0; i < _length; i++) { 136: for (uint256 i = 0; i < _length; i++) {
File: contracts/libraries/Tasks.sol 181: for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
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++) {
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 {
File: contracts/libraries/Tasks.sol /// @audit initialize() 41: library Tasks {
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.
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;
File: contracts/HomeFiProxy.sol 30: mapping(address => bool) internal contractsActive;
File: contracts/HomeFi.sol 50: bool public override addrSet;
File: contracts/Project.sol 68: bool public override contractorConfirmed; 78: bool public override contractorDelegated; 84: mapping(address => mapping(bytes32 => bool)) public override approvedHashes;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis 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");
File: contracts/Project.sol 195: require(_cost > 0, "Project::!value>0");
++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++) {
File: contracts/HomeFiProxy.sol 87: for (uint256 i = 0; i < _length; i++) { 136: for (uint256 i = 0; i < _length; i++) {
File: contracts/libraries/Tasks.sol 181: for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
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++) {
require() statements that use && saves gasSee 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: );
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: );
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;
require() or revert() statements that check input arguments should be at the top of the functionChecks 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");
revert()/require() strings to save gasCustom 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: );
File: contracts/DebtToken.sol 31 require( 32 communityContract == _msgSender(), 33 "DebtToken::!CommunityContract" 34: ); 50: require(_communityContract != address(0), "DebtToken::0 address");
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");
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");
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: );
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");
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");
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: );
payableIf 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 {
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
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) {
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)
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)
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)
_msgSender() if not supporting EIP-2771Use 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(),
#0 - 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 https://github.com/code-423n4/2022-08-rigor-findings/issues/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).
#1 - 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:
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!
#2 - 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
#3 - 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?
#4 - 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.
modifier onlyAdmin() { require(admin == _msgSender(), "HomeFi::!Admin"); _; }