Rigor Protocol contest - robee's results

Remove parameter names from the following function

Community.sol.reduceDebt that inherent ICommunity.sol.reduceDebt : https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/interfaces/ICommunity.sol#L259

Treasury may be address(0)

Make sure the treasury is not address(0).

Code instances:

    HomeFi.sol.initialize _treasury
    HomeFiMock.sol.replaceTreasury _newTreasury
    HomeFiMock.sol.initialize _treasury
    HomeFi.sol.replaceTreasury _newTreasury

Init frontrun

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

Code instances:

    HomeFi.sol, initialize, 92
    Project.sol, constructor, 88
    Community.sol, initialize, 102
    DebtToken.sol, initialize, 43
    ProjectFactory.sol, initialize, 45
    HomeFiProxy.sol, initiateHomeFi, 58
    HomeFiMock.sol, initialize, 51
    Project.sol, initialize, 94
    Disputes.sol, initialize, 74

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    HomeFiMock.sol, _msgSender
    Community.sol, _msgSender
    HomeFi.sol, _msgSender
    Tasks.sol, getState
    Project.sol, getAlerts

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instances:

    Project.sol, initialize is missing a reentrancy modifier
    Project.sol, raiseDispute is missing a reentrancy modifier
    Community.sol, escrow is missing a reentrancy modifier
    Community.sol, publishProject is missing a reentrancy modifier
    Project.sol, recoverTokens is missing a reentrancy modifier
    Community.sol, initialize is missing a reentrancy modifier
    Project.sol, setComplete is missing a reentrancy modifier
    Community.sol, toggleLendingNeeded is missing a reentrancy modifier

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

Code instances:

    Project.sol, updateTaskHash
    Community.sol, updateCommunityHash
    Project.sol, updateProjectHash

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L352
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L381
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L473
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/mock/USDC.sol#L25
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L774
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L320
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L442
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L205

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L200
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/mock/HomeFiMock.sol#L183
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/mock/HomeFiV2Mock.sol#L30
    https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/mock/DebtTokenV2Mock.sol#L13

Mult instead div in compares

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

Code instance:

    Project.sol, 906, require( ((_amount / 1000) * 1000) == _amount, "Project::Precision>=1000" );

Missing fee parameter validation

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    HomeFiMock.initialize (_lenderFee)
    HomeFi.replaceLenderFee (_newLenderFee)
    HomeFiMock.replaceLenderFee (_newLenderFee)
    HomeFi.initialize (_lenderFee)

Require with not comprehensive message

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

Code instance:

    Solidity file: Tasks.sol, In line 124 with Require message: Task::!SC

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    HomeFi.sol.replaceAdmin _newAdmin
    DebtToken.sol.burn _to
    Community.sol.initialize _homeFi
    HomeFi.sol.setAddr _hTokenCurrency2

Unnecessary equals boolean

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

Code instance:

    Community.sol, 272: if (_publishFee == 0) _communityProject.publishFeePaid = true;

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instances:

    builder in Project.sol
    proxyAdmin in HomeFiProxy.sol
    disputes in Project.sol
    lenderFee in Project.sol
    homeFi in Project.sol
    currency in Project.sol
    _decimals in DebtToken.sol

Unused state variables

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

Code instances:

    HomeFiV3Mock.sol, newVariable
    Project.sol, VERSION

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: HomeFiProxy.sol, i, 136
    change to prefix increment and unchecked: Community.sol, i, 624
    change to prefix increment and unchecked: Project.sol, _taskID, 710
    change to prefix increment and unchecked: HomeFiProxy.sol, i, 87
    change to prefix increment and unchecked: Project.sol, i, 322
    change to prefix increment and unchecked: Tasks.sol, i, 181
    change to prefix increment and unchecked: Project.sol, i, 248
    change to prefix increment and unchecked: Project.sol, i, 311

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    Community.sol, 624
    Project.sol, 311
    Tasks.sol, 181
    Project.sol, 322
    HomeFiProxy.sol, 136
    Project.sol, 248
    HomeFiProxy.sol, 87

Storage double reading. Could save SLOAD

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

Code instances:

    Project.sol: builder is read twice in raiseDispute
    Project.sol: taskCount is read twice in allocateFunds
    Project.sol: totalLent is read twice in allocateFunds

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instances:

In Community.sol,rearranging the storage fields can optimize to: 5 slots from: 6 slots. The new order of types (you choose the actual variables): 1. IHomeFi 2. uint256 3. address 4. bool 5. address 6. address

In Project.sol,rearranging the storage fields can optimize to: 13 slots from: 14 slots. The new order of types (you choose the actual variables): 1. IHomeFi 2. IDebtToken 3. uint256 4. uint256 5. uint256 6. uint256 7. uint256 8. uint256 9. uint256 10. uint256 11. address 12. bool 13. bool 14. address 15. address

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    Project.sol, 195: change '_cost > 0' to '_cost != 0'
    Community.sol, 764: change '_repayAmount > 0' to '_repayAmount != 0'

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    Tasks.sol, getState, { return uint256(_self.state); }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    Disputes.sol, executeTaskChange
    Disputes.sol, executeTaskPay
    Disputes.sol, resolveHandler
    Project.sol, autoWithdraw
    SignatureDecoder.sol, toEthSignedMessageHash