Rigor Protocol contest - benbaessler's results

Community lending and instant payments for new home construction.

General Information

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

Rigor Protocol

Findings Distribution

Researcher Performance

Rank: 57/133

Findings: 2

Award: $66.08

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Rigor Protocol

Findings Information

🌟 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

Awards

40.692 USDC - $40.69

Labels

bug

QA (Quality Assurance)

valid

External Links

Link to GitHub issue

[L-01] Use a more recent version of Solidity

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

[L-02] Function does not follow Natspec description

There is 1 instance of this issue:

HomeFiProxy.sol#L52

[L-03] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

There are 4 instances of this issue:

[L-04] Fee in `replaceLenderFee` should be upper-bounded

HomeFi.sol#L185-L197

[L-05] Missing zero-address check in setter function

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly. Consider adding the nonZero modifier.

There is 1 instance of this issue:

HomeFi.sol#L200-L207

[N-01] No use of two-phase ownership transfers

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

There is 1 instance of this issue:

HomeFi.sol#L157-L168

[N-02] The `nonReentrant` modifier should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers

There is 1 instance of this issue:

Disputes.sol#L145

Findings Information

🌟 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

Awards

25.3906 USDC - $25.39

Labels

bug

G (Gas Optimization)

valid

External Links

Link to GitHub issue

[G-01] Usage of `uints`/`ints` smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed.

There are 2 instances of this issue:

DebtToken.sol#L16

Disputes.sol#L100

[G-02] Use custom error rather than `revert()`/`require()` strings to save deployment gas

Custom errors are available from solidity version 0.8.4. The instances below match or exceed that version.

There are 63 instances of this issue:

ProjectFactory.sol#L36

Disputes.sol#L64

25 instances here

20 instances here

[G-03] Duplicated `require()` / `revert()` checks should be refactored to a modifier or function

Saves deployment costs.

There is 1 instance of this issue:

DebtToken.sol#L96

DebtToken.sol#L104

[G-04] Using `calldata` instead of `memory` for read-only arguments in `external` function saves gas

There are 4 instances of this issue:

DebtToken.sol#L45-L46

HomeFi.sol#L210

Community.sol#L489

[G-05] State variables only set in the constructor should be declared `immutable`

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 17 instances of this issue:

ProjectFactory.sol#L28

Community.sol#L33-L35

Community.sol#L48

[G-06] 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, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

There are 6 instances of this issue:

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

Saves 6 gas per loop.

There are 9 instances of this issue:

[G-08] Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

There are 9 instances of this issue:

[G-09] Functions guaranteed to revert when called 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.

There are 18 instances of this issue:

[G-10] Not using the named return variables when a function returns, wastes deployment gas

There is 1 instance of this issue:

HomeFi.sol#L307

[G-11] 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

There are 5 instances of this issue:

Disputes.sol#L61-L65

Disputes.sol#L106-L109

Project.sol#L195

Community.sol#L353-L357

[G-12] `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 2 instances of this issue:

HomeFi.sol#L284-L297

Disputes.sol#L207-L223

[G-13] `public` functions to `external`

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

There are 2 instances of this issue:

HomeFi.sol#L265

Project.sol#L726

[G-14] `x = x + y` is cheaper than `x += y`

There are 4 instances of this issue:

[G-15] Using bools for storage incurs overhead

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 4 instances of this issue:

[G-16] Using `> 0` costs more gas than `!= 0` when used in a `require()` statement

This change saves 6 gas per instance.

There are 2 instances of this issue:

Disputes.sol#L107

Community.sol#L764

[G-17] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

There is 1 instance of this issue:

Project.sol#L60

[G-18] `<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:

Project.sol#L603

Rigor Protocol contest - benbaessler's results

Community lending and instant payments for new home construction.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-28] QA Report - $40.69

Findings Information

Awards

Labels

External Links

[L-01] Use a more recent version of Solidity

[L-02] Function does not follow Natspec description

[L-03] Add a timelock to critical functions

[L-04] Fee in replaceLenderFee should be upper-bounded

[L-05] Missing zero-address check in setter function

[N-01] No use of two-phase ownership transfers

[N-02] The nonReentrant modifier should occur before all other modifiers

[G-11] Gas Report - $25.39

Findings Information

Awards

Labels

External Links

[G-01] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

[G-02] Use custom error rather than revert()/require() strings to save deployment gas

[G-03] Duplicated require() / revert() checks should be refactored to a modifier or function

[G-04] Using calldata instead of memory for read-only arguments in external function saves gas

[G-05] State variables only set in the constructor should be declared immutable

[G-06] No need to explicitly initialize variables with default values

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

[G-08] Increments can be unchecked

[G-09] Functions guaranteed to revert when called normal users can be marked payable

[G-10] Not using the named return variables when a function returns, wastes deployment gas

[G-11] Splitting require() statements that use && saves gas

[G-12] internal functions only called once can be inlined to save gas

[G-13] public functions to external

[G-14] x = x + y is cheaper than x += y

[G-15] Using bools for storage incurs overhead

[G-16] Using > 0 costs more gas than != 0 when used in a require() statement

[G-17] Using private rather than public for constants, saves gas

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

[L-04] Fee in `replaceLenderFee` should be upper-bounded

[N-02] The `nonReentrant` modifier should occur before all other modifiers

[G-01] Usage of `uints`/`ints` smaller than 32 bytes (256 bits) incurs overhead

[G-02] Use custom error rather than `revert()`/`require()` strings to save deployment gas

[G-03] Duplicated `require()` / `revert()` checks should be refactored to a modifier or function

[G-04] Using `calldata` instead of `memory` for read-only arguments in `external` function saves gas

[G-05] State variables only set in the constructor should be declared `immutable`

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

[G-09] Functions guaranteed to revert when called normal users can be marked `payable`

[G-11] Splitting `require()` statements that use `&&` saves gas

[G-12] `internal` functions only called once can be inlined to save gas

[G-13] `public` functions to `external`

[G-14] `x = x + y` is cheaper than `x += y`

[G-16] Using `> 0` costs more gas than `!= 0` when used in a `require()` statement

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