Rigor Protocol contest - Aymen0909's results

UNUSED STATE VARIABLE SHOULD BE REMOVED

Impact - NON CRITICAL

File: contracts/Project.sol

line 60 : uint256 public constant override VERSION = 25000;

CONSTANTS SHOULD BE DEFINED RATHER THAN USING MAGIC NUMBERS

Impact - NON CRITICAL

File: contracts/libraries/SignatureDecoder.sol

line 25 : if (messageSignatures.length % 65 != 0)
line 35 : if (v != 27 && v != 28)
line 82 : if (v < 27)

PUBLIC FUNCTIONS NOT CALLED BY THE CONTRACT SHOULD BE DECLARED EXTERNAL INSTEAD

Impact - NON CRITICAL

File: contracts/Community.sol

line 700 : function isTrustedForwarder(address _forwarder) public view override(ERC2771ContextUpgradeable, ICommunity) returns (bool)

File: contracts/DebtToken.sol

line 82 : function decimals() public view virtual override returns (uint8)

File: contracts/Disputes.sol

line 187 : function isTrustedForwarder(address _forwarder) public view override(ERC2771ContextUpgradeable, ICommunity) returns (bool)

File: contracts/HomeFi.sol

line 264 : function isTrustedForwarder(address _forwarder) public view override(ERC2771ContextUpgradeable, ICommunity) returns (bool)

File: contracts/Project.sol

line 727 : function isTrustedForwarder(address _forwarder) public view override(ERC2771ContextUpgradeable, ICommunity) returns (bool)

File: contracts/ProjectFactory.sol

line 98 : function isTrustedForwarder(address _forwarder) public view override(ERC2771ContextUpgradeable, ICommunity) returns (bool)

1- FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED payable :

If a function modifier such as onlyAdmin 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 the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2).

Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

File: contracts/Community.sol

line 555 : function restrictToAdmin() external override onlyHomeFiAdmin
line 566 : function unrestrictToAdmin() external override onlyHomeFiAdmin
line 577 : function pause() external override onlyHomeFiAdmin 
line 582 : function unpause() external override onlyHomeFiAdmin

File: contracts/Disputes.sol

line 141 : function resolveDispute(uint256 _disputeID, bytes calldata _judgement, bool _ratify) external override onlyAdmin

File: contracts/HomeFi.sol

line 123 : function setAddr(address _projectFactory, address _communityContract, address _disputesContract, address _hTokenCurrency1, address _hTokenCurrency2, address _hTokenCurrency3) external override onlyAdmin
line 157 : function replaceAdmin(address _newAdmin) external override onlyAdmin
line 171 : function replaceTreasury(address _newTreasury) external override onlyAdmin
line 185 : function replaceLenderFee(uint256 _newLenderFee) external override onlyAdmin
line 203 : function setTrustedForwarder(address _newForwarder) external override onlyAdmin

2- USE CUSTOM ERRORS RATHER THAN REQUIRE()/REVERT() STRINGS TO SAVE DEPLOYMENT GAS :

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

There are many lines with this issue throughout the contracts :

File: contracts/Community.sol => 24 instances File: contracts/DebtToken.sol => 4 instances File: contracts/Disputes.sol => 6 instances File: contracts/HomeFi.sol => 6 instances File: contracts/HomeFiProxy.sol => 4 instances File: contracts/Project.sol => 25 instances File: contracts/ProjectFactory.sol => 3 instances File: contracts/libraries/Tasks.sol => 4 instances

3- SPLITTING REQUIRE() STATEMENTS THAT USES && SAVES GAS

File: contracts/Community.sol

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

File: contracts/Disputes.sol

line 61 : require(_disputeID < disputeCount && disputes[_disputeID].status == Status.Active, "Disputes::!Resolvable");
line 106 : require(_actionType > 0 && _actionType <= uint8(ActionType.TaskPay),"Disputes::!ActionType");

4- NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

File: contracts/Community.sol

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

File: contracts/HomeFiProxy.sol

line 87, 136 : for (uint256 i = 0; i < _length; i++)

File: contracts/Project.sol

line 248, 311, 322 : for (uint256 i = 0; i < _length; i++)

5- USE OF ++i COST LESS GAS THAN i++ IN FOR LOOPS :

File: contracts/Community.sol

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

File: contracts/HomeFiProxy.sol

line 87, 136 : for (uint256 i = 0; i < _length; i++)

File: contracts/Project.sol

line 248, 311, 322 : for (uint256 i = 0; i < _length; i++)
line 368, 710 : for (uint256 _taskID = 1; _taskID <= _length; _taskID++)
line 603 : for (; i < _changeOrderedTask.length; i++)

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

File: contracts/Community.sol

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

File: contracts/HomeFiProxy.sol

line 87, 136 : for (uint256 i = 0; i < _length; i++)

File: contracts/Project.sol

line 248, 311, 322 : for (uint256 i = 0; i < _length; i++)
line 368, 710 : for (uint256 _taskID = 1; _taskID <= _length; _taskID++)

7- <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 are 2 instance of this issue:

File: contracts/Community.sol

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

File: contracts/Project.sol

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

8- USING > 0 COSTS MORE GAS THAN != 0 WHEN USED ON A UINT IN A REQUIRE() STATEMENT :

UINT are always positive (greater or equal than 0), so use != 0 instead of > 0 this change saves 6 gas per instance

File: contracts/Community.sol

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

File: contracts/Disputes.sol

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

File: contracts/Project.sol

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

9- <X> += <Y> COSTS MORE GAS THAN <X> = <X> + <Y> FOR STATE VARIABLES

There are 7 instances of this issue:

File: contracts/HomeFi.sol

line 289 : projectCount += 1;

File: contracts/Project.sol

line 179, 290 : hashChangeNonce += 1;
line 431 : totalAllocated -= _withdrawDifference;
line 440 : totalAllocated += _newCost - _taskCost;
line 456 : totalAllocated -= _taskCost;
line 772 : totalLent -= _amount;