QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	8
Non-Critical Risk	4

Table of Contents

• 1. Low Risk Issues
  • 1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 1.2. Add constructor initializers
  • 1.3. Uninitialized Upgradeable contract
  • 1.4. Fees should be upper-bounded
  • 1.5. _safeMint() should be used rather than _mint() wherever possible
  • 1.6. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
  • 1.7. Multiple initialize() functions are front-runnable in the solution
  • 1.8. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. The nonReentrant modifier should occur before all other modifiers
  • 2.3. Use bytes.concat()
  • 2.4. Adding a return statement when the function defines a named return variable, is redundant

1. Low Risk Issues

1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/[email protected] here:

• https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/package.json#L68-L69 :

File: package.json
68:     "@openzeppelin/contracts": "^4.4.2",
69:     "@openzeppelin/contracts-upgradeable": "^4.4.2"

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

1.2. Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

Community.sol:105:        initializer
DebtToken.sol:48:    ) external override initializer {
Disputes.sol:77:        initializer
HomeFi.sol:102:        initializer
HomeFiProxy.sol:60:        initializer
ProjectFactory.sol:48:        initializer

Note that this is already mitigated here:

Project.sol:88:    constructor() initializer {}
Project.sol:98:    ) external override initializer {

1.3. Uninitialized Upgradeable contract

Similar issue in the past: here

Upgradeable dependencies should be initialized.
While not causing any harm at the moment, suppose OZ someday decide to upgrade this contract and the sponsor uses the new version: it is possible that the contract will not work anymore.

Consider calling the missing __ReentrancyGuard_init() here (ERC2771ContextUpgradeable doesn't have an init function):

File: Community.sol
21: contract Community is
22:     ICommunity,
23:     PausableUpgradeable,
24:     ReentrancyGuardUpgradeable,
25:     ERC2771ContextUpgradeable
...
102:     function initialize(address _homeFi)
...
108:         // Initialize pausable. Set pause to false;
109:         __Pausable_init();
+ 110:         __ReentrancyGuard_init(); //@audit ERC2771ContextUpgradeable doesn't have an init function

1.4. Fees should be upper-bounded

There should be an upper limit to reasonable fees

File: HomeFi.sol
185:     function replaceLenderFee(uint256 _newLenderFee)
186:         external
187:         override
188:         onlyAdmin
189:     {
190:         // Revert if no change in lender fee
191:         require(lenderFee != _newLenderFee, "HomeFi::!Change");
192: 
193:         // Reset variables
194:         lenderFee = _newLenderFee;
195: 
196:         emit LenderFeeReplaced(_newLenderFee);
197:     }

1.5. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver, see the WARNING L278:

File: ERC721Upgradeable.sol
275:     /**
276:      * @dev Mints `tokenId` and transfers it to `to`.
277:      *
278:      * WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
...
286:      */
287:     function _mint(address to, uint256 tokenId) internal virtual {

Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

Consider replacing with _safeMint() here:

File: HomeFi.sol
284:     function mintNFT(address _to, string memory _tokenURI)
...
291:         // Mints NFT and set token URI
292:         _mint(_to, projectCount);

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.6. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

• contracts/Community.sol:

   21  contract Community is
   22      ICommunity,
   23:     PausableUpgradeable,
   24:     ReentrancyGuardUpgradeable,
   25:     ERC2771ContextUpgradeable

• contracts/DebtToken.sol:

   11: contract DebtToken is IDebtToken, ERC20Upgradeable {

• contracts/Disputes.sol:

   17  contract Disputes is
   18      IDisputes,
   19:     ReentrancyGuardUpgradeable,
   20:     ERC2771ContextUpgradeable

• contracts/HomeFi.sol:

   27  contract HomeFi is
   28      IHomeFi,
   29:     ReentrancyGuardUpgradeable,
   30:     ERC721URIStorageUpgradeable,
   31:     ERC2771ContextUpgradeable

• contracts/HomeFiProxy.sol:

   14: contract HomeFiProxy is OwnableUpgradeable {

• contracts/Project.sol:

   24  contract Project is
   25      IProject,
   26:     ReentrancyGuardUpgradeable,
   27:     ERC2771ContextUpgradeable

• contracts/ProjectFactory.sol:

   16  contract ProjectFactory is
   17      IProjectFactory,
   18      Initializable,
   19:     ERC2771ContextUpgradeable
   20  {

1.7. Multiple initialize() functions are front-runnable in the solution

Consider adding some access control to them or deploying atomically or using constructor initializer:

contracts/Community.sol:
  102:     function initialize(address _homeFi)

contracts/DebtToken.sol:
  43:     function initialize(

contracts/Disputes.sol:
  74:     function initialize(address _homeFi)

contracts/HomeFi.sol:
  92:     function initialize(

contracts/ProjectFactory.sol:
  45:     function initialize(address _underlying, address _homeFi)

contracts/libraries/Tasks.sol:
  75:     function initialize(Task storage _self, uint256 _cost) public {

Only Project.sol is protected.

1.8. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role.
Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

HomeFiProxy.sol:7:import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
HomeFiProxy.sol:14:contract HomeFiProxy is OwnableUpgradeable {
HomeFiProxy.sol:156:        proxyAdmin.transferOwnership(_newAdmin);

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

contracts/Project.sol:
  212:         emit LendToProject(_cost);
  213  
  214          // Allocate funds to tasks and mark then as allocated
  215          allocateFunds();
  216      }

2.2. The nonReentrant modifier should occur before all other modifiers

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

Disputes.sol:145:    ) external override onlyAdmin nonReentrant resolvable(_disputeID) {

2.3. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

libraries/SignatureDecoder.sol:3:pragma solidity 0.8.6;
libraries/SignatureDecoder.sol:50:                abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)

2.4. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

File: Tasks.sol
191:     function getState(Task storage _self)
192:         internal
193:         view
194:         returns (uint256 _state)
195:     {
196:         return uint256(_self.state);
197:     }

File: Community.sol
899:     function _msgSender()
900:         internal
901:         view
902:         override(ContextUpgradeable, ERC2771ContextUpgradeable)
903:         returns (address sender)
904:     {
905:         // We want to use the _msgSender() implementation of ERC2771ContextUpgradeable
906:         return super._msgSender();
907:     }

File: HomeFi.sol
303:     function _msgSender()
304:         internal
305:         view
306:         override(ContextUpgradeable, ERC2771ContextUpgradeable)
307:         returns (address sender)
308:     {
309:         // We want to use the _msgSender() implementation of ERC2771ContextUpgradeable
310:         return super._msgSender();
311:     }

File: Project.sol
716:     function getAlerts(uint256 _taskID)
717:         public
718:         view
719:         override
720:         returns (bool[3] memory _alerts)
721:     {
722:         return tasks[_taskID].getAlerts();
723:     }