QA Report

[code423n4]

• Low
  • 1. Outdated and mixed pragmas
  • 2. Upgradable contracts without GAP can lead a upgrade disaster
  • 3. Lack of ACK during owner change
  • 4. lack of checks
  • 5. Wrong comment
  • 6. Duplicate nft hash
  • 7. isPublishedToCommunity doesn't check that exists
  • 8. Denial of service by gas exhaustion
• Non-Critical
  • 9. Avoid without storage keyword in modifiers
  • 10. Unify code style
  • 11. initialize Front running
  • 12. Improve usability reducing approves
  • 13. Lack of event index
  • 14. Avoid magic numbers
  • 15. Ensure formula

Low

1. Outdated and mixed pragmas

The pragma version used are:

pragma solidity 0.8.6;
pragma solidity ^0.8.9;

Note that mixing pragma are not recommended.

The minimum required version must be 0.8.15; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

Apart from these, there are several minor bug fixes and improvements.

2. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• DebtToken.sol#L11
• ProjectFactory.sol#L19
• HomeFiProxy.sol#L14
• Disputes.sol#L19-L20
• HomeFi.sol#L27
• Project.sol#L26-L27
• Community.sol#L23-L25

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• HomeFiProxy.sol#L150
• HomeFi.sol#L165

4. lack of checks

Check that the _data is 64 bytes length, _data is used for hashing and signature, and if it's longer than 64 bytes, the real data will stay in the fist 64 bytes, but the dummy data will create the hash, so it allows to replay the same payload without the same data.

In short, if data is concatenated at the end of data it will generate a different hash for the same payload.

• Project.inviteContractor
• Project.setComplete
• Project.changeOrder
• Project.raiseDispute

We have the same issue but with a dynamic variable, so maybe it's easier to encode the payload instead of trust in _data:

• Project.updateProjectHash
• Project.addTasks
• Project.updateTaskHash
• Project.setComplete
• Project.addMember
• Project.publishProject
• Community.escrow

lenderFee must be <= 10:

• HomeFi.sol#L115
• HomeFi.sol#L194

The following lines requires a valid address as an argument, in some of them, once the value is set, it cannot be changed again, so it is mandatory to check if these values are as expected.

Affected source code for address(0):

• Project.sol#L95-L96

5. Wrong comment

In HomeFi.sol#L115 we can read the comment:

• // the percentage must be multiplied with 10.

But the true is that the fee factor is 1000, like we can see in Community.sol#L393.

        // Calculate lenderFee
        uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
            (_projectInstance.lenderFee() + 1000);

6. Duplicate nft hash

When calling createProject an argument is passed that is used as the NFT's tokenUri, this is not checked for its existence and allows duplications between different NFTs.

Affected source code:

• HomeFi.sol#L225

7. isPublishedToCommunity doesn't check that exists

If isPublishedToCommunity is called with a _communityID = 0 and a _project that does not exist, this modifier will pass, and should not

Affected source code:

• Community.sol#L82

8. Denial of service by gas exhaustion

If a large number of members have been entered, the members method will be denied.

Affected source code:

• Community.sol#L625

---

Non-Critical

9. Avoid without storage keyword in modifiers

It is not a good practice for a modifier to make modifications, in this case it does not, but receives a variable as storage without being necessary, it is recommended to make the following change

-   modifier onlyInactive(Task storage _self) {
-       require(_self.state == TaskStatus.Inactive, "Task::active");
+   modifier onlyInactive(TaskStatus _state) {
+       require(_state == TaskStatus.Inactive, "Task::active");
        _;
    }

    /// @dev only allow active tasks. Task is inactive if SC is confirmed.
-   modifier onlyActive(Task storage _self) {
-       require(_self.state == TaskStatus.Active, "Task::!Active");
+   modifier onlyActive(TaskStatus _state) {
+       require(_state == TaskStatus.Active, "Task::!Active");
        _;
    }

Affected source code:

• Tasks.sol#L43-L61

10. Unify code style

In other methods such as replaceTreasury or replaceAdmin to verify that a change is not made, the noChange modifier is used, however in the replaceLenderFee method of HomeFi.sol#L191 is not the case and it is done inline.

    function replaceLenderFee(uint256 _newLenderFee)
        external
        override
+       noChange(lenderFee, _newLenderFee)
        onlyAdmin
    {
-       // Revert if no change in lender fee
-       require(lenderFee != _newLenderFee, "HomeFi::!Change");
- 
        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

11. initialize Front running

To initialize contract parameters, most contracts employ an initialize pattern (rather than a constructor). They are vulnerable 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 attack unless they are enforced to be atomic with contact deployment via deployment script or factory contracts.

Affected source code:

• Community.sol#L102
• DebtToken.sol#L43
• Disputes.sol#L74
• HomeFi.sol#L92
• Project.sol#L94
• ProjectFactory.sol#L45

12. Improve usability reducing approves

Each transferFrom that is made requires the user to approve homeFi.treasury() and another to the _project in order to pay correctly. It is more usable for the user if the approve has him towards the community, and the community already transfers the funds at will.

-       // Transfer _lenderFee to HomeFi treasury from lender account
-       _currency.safeTransferFrom(_msgSender(), homeFi.treasury(), _lenderFee);
+       _currency.safeTransferFrom(_msgSender(), address(this), _lenderFee);
+       _currency.safeTransfer(homeFi.treasury(), _lenderFee);
+       _currency.safeTransfer(_project, _amountToProject);

-       // Transfer _amountToProject to _project from lender account
-       _currency.safeTransferFrom(_msgSender(), _project, _amountToProject);

Affected source code:

• Community.sol#L442-L446

13. Lack of event index

Community use this event so it's under scope

It would be convenient to add an index per account to the ApproveHash event.

event ApproveHash(bytes32 _hash, address indexed _signer);

Affected source code:

• ICommunity.sol#L111

14. Avoid magic numbers

It is not good practice to use magic numbers when solidity literals can be used.

        uint256 _noOfDays = (block.timestamp -
+           _communityProject.lastTimestamp) / 1 days;
-           _communityProject.lastTimestamp) / 86400; // 24*60*60

Affected source code:

• Community.sol#L686

15. Ensure formula

Although solidity does it well, it is more readable to use '('

        /// Interest formula = (principal * APR * days) / (365 * 1000)
        // prettier-ignore
        uint256 _unclaimedInterest = 
-               _lentAmount *
+               (_lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
+               _noOfDays) /
-               _noOfDays /
                365000;

Affected source code:

• Community.sol#L688-L694