Rigor Protocol contest - NoamYakov's results

[1] Unnecessary SLOADs in for-each loops

There are for loops that follows this for-each pattern:

for (uint256 i = 0; i < array.length; i++) {
        // do something with `array[i]`
}

In such for loops, the array.length is read on every iteration, instead of caching it once in a local variable and read it from there. Storage reads are much more expensive than reading local variables.

Recommendation

Read these values from storage once, cache them in local variables and then read them again from the local variables. For example:

uint256 length = array.length;
for (uint256 i = 0; i < length; i++) {
        // do something with `array[i]`
}

Code References

• Project.sol: line 603

[2] Default value initialization

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc. depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example:

uint256 x = 0;

can be optimized to:

uint256 x;

Code References

• Community.sol: line 624
• HomeFiProxy.sol: lines 87, 136
• Project.sol: lines 248, 311, 322, 412
• Tasks.sol: line 181

[3] ++ is cheaper than += 1

Use prefix increments (++x) instead of increments by 1 (x += 1).

Recommendation

Change all increments by 1 to prefix increments.

Code References

• HomeFi.sol: line 289
• Project.sol: lines 179, 250, 290

[4] Unnecessary array boundaries check when loading an array element twice

Some functions loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using that local variable, skipping the redundant second array boundaries check and saving gas.

Recommendation

Load the array elements once, cache them in local variables and then read them again using the local variables. For example:

uint256 item = array[i];
// do something with `item`
// do some other thing with `item`

Code References

• Project.sol: lines 253+256 (_taskCosts[i])

[5] public functions not called by the contract should be declared external instead

external functions are cheaper than public function.

Recommendation

Re-declare public functions as external functions when they aren't called by the contract.

Code References

• Community.sol: line 710 (isTrustedForwarder)
• DebtToken.sol: lines 82 (decimals), 95 (transferFrom), 103 (transfer)
• Disputes.sol: line 188 (isTrustedForwarder)
• HomeFi.sol: line 265 (isTrustedForwarder)
• Project.sol: line 727 (isTrustedForwarder)
• ProjectFactory.sol: line 99 (isTrustedForwarder)
• Tasks.sol: line 75 (initialize)

[6] Prefix increments are cheaper than postfix increments

Use prefix increments (++x) instead of postfix increments (x++).

Recommendation

Change all postfix increments to prefix increments.

Code References

• Community.sol: lines 140, 624
• Disputes.sol: line 121
• HomeFiProxy.sol: lines 87, 136
• Project.sol: lines 248, 311, 322, 368, 603, 625, 650, 672, 710
• Tasks.sol: line 181

[7] Unnecessary checked arithmetic in for loops

There is no risk of overflow caused by increments to the iteration index in for loops (the i++ in for (uint256 i = 0; i < numIterations; i++)). Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the loop

for (uint256 i = 0; i < numIterations; i++) {
      // ...
}

to

for (uint256 i = 0; i < numIterations;) {
      // ...
      unchecked { i++; }
}

It is a little less readable but it saves a significant amount of gas.

Code References

• Community.sol: line 624
• HomeFiProxy.sol: lines 87, 136
• Project.sol: lines 248, 311, 322, 368, 603, 650, 710
• Tasks.sol: line 181

[8] Unnecessary SLOADs

Reading variables that use storage as their memory location is an expensive process. Much more expensive than reading local variables.

Recommendation

When such a variable is read more than once, cache it in a local variable to avoid more SLOADs.

Code References

• ProjectFactory.sol: lines 84+90 (homeFi)
• HomeFi.sol: lines 228+229+231 (projectCount), 292+293+295+296 (projectCount)
• Project.sol: lines 524+528 (tasks[_task].subcontractor), 605+619+622 (_changeOrderedTask[i]), 101+102 (homeFi), 190+204 (builder), 516+522 (builder), 800+812 (builder), 844+858 (builder), 144 (contractor), 516+523 (contractor), 798+807+813 (contractor), 842+852+859 (contractor), 579+697 (totalLent), 592+648+650+681+682 (taskCount)
• Community.sol: lines 620+624 (_communities[_communityID].memberCount), 113+114+115 (homeFi), 132+137 (homeFi), 414+443 (homeFi), 143+150 (communityCount)

[9] Unnecessary external call

External calls are expensive and should not take place more than once if they don't have any side-effects (i.e., if they are calls to view functions).

Recommendation

execute the external call once, cache the return value in a local variable and then use that local variable instead of executing the external call again. For example:

uint256 result = someContract.someViewFuction();
// do something with `result`
// do some other thing with `result`

Code References

• Community.sol: lines 393+394 (_projectInstance.lenderFee())

[10] Inlining internal functions

internal functions only called once can be inlined to save gas.

Recommendation

Inline internal function only called once.

Code References

• Disputes.sol: lines 149 (resolveHandler()), 213 (executeTaskAdd()), 217 (executeTaskChange()), 221 (executeTaskPay())
• HomeFi.sol: line 225 (mintNFT())
• HomeFiProxy.sol: line 137 (_replaceImplementation())
• Project.sol: line 435 (autoWithdraw())

[11] Addition to and subtraction from state variables

x += y and x -= y cost more gas than x = x + y and x = x - y for state variables.

Recommendation

Use x = x + y and x = x - y instead of x += y and x -= y for state variables.

Code References

• Community.sol: lines 423, 435
• HomeFi.sol: line 289
• Project.sol: lines 179, 290, 431, 440, 456, 772

[12] Non-zero tests for unsigned integers

Testing != 0 is cheaper than testing > 0 for unsigned integers.

Recommendation

Use != 0 instead of > 0 when testing unsigned integers.

Code References

• Community.sol: lines 261, 427, 764, 840
• Disputes.sol: line 107
• HomeFi.sol: line 245
• Project.sol: lines 195, 380, 601, 691

[13] Inefficient storage layout

State variables smaller than 32-bytes can share a storage slot if they both fit in a 32-byte slot.

Recommendation

Whenever possible, declare these state variable in a way they are packed together. For example: the layout

uint8 x;
uint256 y;
uint8 z;

can be optimized to

uint256 y;
uint8 x;
uint8 z;

where x and z are sharing a single storage slot.

Code References

• Project.sol: builder and contractorDelegated can be packed together.