QA Report

[code423n4]

[L-01] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Community.sol::440 => .lastTimestamp = block.timestamp;
Community.sol::685 => uint256 _noOfDays = (block.timestamp -
Community.sol::847 => _communityProject.lastTimestamp = block.timestamp;

[L-02] Missing Zero address checks

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters.
Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

HomeFi.sol::119 => trustedForwarder = _forwarder;
HomeFi.sol::206 => trustedForwarder = _newForwarder;
Project.sol::100 => homeFi = IHomeFi(_homeFiAddress);
Project.sol::103 => builder = _sender;
Project.sol::104 => currency = IDebtToken(_currency);

[L-03] Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

HomeFi.sol::200 => function setTrustedForwarder(address _newForwarder)

[L-04] ecrecover() not checked for signer address of zero

The solidity function ecrecover is used, however the error result of 0 is not checked for.
See documentation:
https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions
"recover the address associated with the public key from elliptic curve signature or return zero on error. "

Recommended Mitigation Steps:

Verify that the result from ecrecover isn't 0

SignatureDecoder.sol::39 => return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);

[L-05] _safeMint() should be used rather than _mint() wherever possible

If the _to account is a contract that cannot handle ERC721 tokens, using _mint could result in the NFT being stuck. Recommend using _safeMint() instead.

DebtToken.sol::66 => _mint(_to, _total);
HomeFi.sol::292 => _mint(_to, projectCount);

[L-06] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

__gap is empty reserved space in storage that is recommended to be put in place in upgradeable contracts. It allows new state variables to be added in the future without compromising the storage compatibility with existing deployments

Community.sol::8 => import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
DebtToken.sol::6 => import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
Disputes.sol::8 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
HomeFiProxy.sol::5 => import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
HomeFi.sol::7 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
ProjectFactory.sol::8 => import {ClonesUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/ClonesUpgradeable.sol";
Project.sol::9 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

[L-07] Implementation contract may not be initialized

Implementation contract does not have a constructor with the initializer modifier therefore may be uninitialized. Implementation contracts should be initialized to avoid potential griefs or exploits.

Community.sol::8 => import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
DebtToken.sol::6 => import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
Disputes.sol::8 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
HomeFiProxy.sol::5 => import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
HomeFi.sol::7 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
ProjectFactory.sol::8 => import {ClonesUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/ClonesUpgradeable.sol";

[L-08] Upgrade Open Zeppelin contract dependency

An outdated OZ version is used (which has known vulnerabilities, see https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

The solution uses:

    "@openzeppelin/contracts": "^4.4.2",
    "@openzeppelin/contracts-upgradeable": "^4.4.2"

[L-09] No Transfer Ownership Pattern

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

HomeFi.sol

    function replaceAdmin(address _newAdmin)
        external
        override
        onlyAdmin
        nonZero(_newAdmin)
        noChange(admin, _newAdmin)
    {
        // Replace admin
        admin = _newAdmin;

        emit AdminReplaced(_newAdmin);
    }

HomeFiProxy.sol

    function changeProxyAdminOwner(address _newAdmin)
        external
        onlyOwner
        nonZero(_newAdmin)
    {
        // Transfer ownership to new admin.
        proxyAdmin.transferOwnership(_newAdmin);
    }

[L-10] Fees can be modified without timelock and upperbound check

The lender fee in HomeFi.sol can be modified by the admin by calling replaceLenderFee(), this function has no upper bound check nor timelock.

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L185-L197

This could lead to front-running either on purpose or accidentally to cause a higher fee to be paid to the homeFi.treasury(), and directly reduces the amount going to project

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L397
https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L443

Recommendation: add upper bound check and timelock to critical setters

[L-11] Same function defined in two locations

Both Project.sol and Community.sol have the function checkSignatureValidity() which repeats the same code only with a different revert string

It is not recommended to repeat code in two separate locations as this could lead to errors.

Recommend refactoring this function into a library contract

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L875-L892
https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L871-L893

[L-12] Checks should be added for signature malleability

The function recoverKey() returns the value from ecrecover() without using a nonce, this could lead to signature replay attacks

SignatureDecoder.sol::39 => return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);

Recommended Mitigation Steps

Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(,)
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Community.sol::3 => pragma solidity 0.8.6;
DebtToken.sol::3 => pragma solidity 0.8.6;
Disputes.sol::3 => pragma solidity 0.8.6;
HomeFiProxy.sol::3 => pragma solidity 0.8.6;
HomeFi.sol::3 => pragma solidity 0.8.6;
ProjectFactory.sol::3 => pragma solidity 0.8.6;
Project.sol::3 => pragma solidity 0.8.6;
SignatureDecoder.sol::3 => pragma solidity 0.8.6;
Tasks.sol::3 => pragma solidity 0.8.6;

[N-02] Constants should be defined rather than using magic numbers

It is bad practice to use numbers directly in code without explanation

Community.sol::394 => (_projectInstance.lenderFee() + 1000);
Community.sol::694 => 365000;
Project.sol::907 => ((_amount / 1000) * 1000) == _amount,
SignatureDecoder.sol::25 => if (messageSignatures.length % 65 != 0) {
SignatureDecoder.sol::35 => if (v != 27 && v != 28) {
SignatureDecoder.sol::82 => if (v < 27) {
SignatureDecoder.sol::83 => v += 27;

[N-03] Adding a return statement when the function defines a named return variable, is redundant

It is not necessary to have both a named return and a return statement.

Community.sol::903 => returns (address sender)
Community.sol::914 => returns (bytes calldata)
HomeFi.sol::307 => returns (address sender)
HomeFi.sol::318 => returns (bytes calldata)
Project.sol::720 => returns (bool[3] memory _alerts)
Tasks.sol::194 => returns (uint256 _state)

[N-04] Missing event for critical parameter change

Emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following changes to the contract.

HomeFi.sol::200 => function setTrustedForwarder(address _newForwarder)

[N-05] Use native time units such as seconds, minutes, hours, days, weeks and years, rather than numbers of days

Community.sol::694 => 365000;