QA Report

[code423n4]

QA Report

Low Risk Findings

[L-01] HomeFi.sol implements a secondary non-EIP1967 admin address

Description

The underlying proxy for HomeFi.sol stores the admin address in the EIP1967 compliant location at:

bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)

HomeFi.sol implements a secondary non-EIP1967 admin address in L52, which is set in L113 of initialize.

Lines of Code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L52

Mitigation

admin should be renamed to something else such as owner to avoid any confusion

[L-02] Proxies implement a payable fallback and receive

Description

Contracts never deal directly with eth, but implement a payable fallback and receive

Lines of Code

N/A

Mitigation

Fallback should not be payable and receive should revert.

[L-03] HomeFi.sol mints users an NFT with no function when they call createProject

Description

HomeFi.sol mints an NFT when calling createProject, but never uses any feature associated with it. It consumes a lot of gas to mint an NFT then never use it for anything. Implementing the ERC721 standard increases gas cost to deploy the contract because of the extra code required.

Lines of Code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L232

Mitigation

Don't mint NFTs when calling createProject and don't implement ERC721

[L-04] Current method for storing acceptable currency is limited

Description

Current method of storing acceptable currency can only ever support 3 currencies. In order to add more, a new implementation would have to be deployed. If currencies were stored in a mapping it would be easy and convenient to add new currencies without having to deploy a new contract.

Lines of Code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L253-L261

Mitigation

Store the approved currencies in a mapping(address => bool) to approve a currency set the bool for that currency address to true. To validate a currency simply check if the mapping returns true for its address.

Non-Critical Findings

[NC-01] Community.sol has no way to remove members

Description

There is no way to remove a member from a community after they've been added. It could help with bloat if there was a way to remove users.

Lines of Code

N/A

Mitigation

Add a function that allows the owner to remove users. Additionally add a nonce to addMember so the transaction data can't be replayed after the user is removed