QA Report

[code423n4]

Executive Summary

Codebase looks mature and well thought-out.
Documentation is complete and testing coverage is high.

A few technicalities related to handling of signatures as well as values due to integer division should be addressed before release.

I would also recommend considering pragmatic changes to offer gas savings via hardcoding of unchangeable variables, with the goal of saving tens of thousands of gas.

Legend:

• U -> Impact is unclear and potentially higher than Low Severity - NONE
• L -> Low Severity -> Could cause issues however impact is limited - NONE
• R -> Refactoring -> Suggested Code Change to improve readability and maintainability or to offer better User Experience
• NC -> Non-Critical / Informational -> No risk of loss, pertains to events or has no impact

U - Lack of Validation for APR - Means APR can be set to any value

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L267-L268

        _communityProject.apr = _apr;

The apr value for a project, cannot be changed, is not sanitized and is used to calculate interest.
It may be best to provide logical boundaries to the value (below usury rate)

Suggested Remediation

Add rational (and potentially legal) constraints to apr

U - Lack of validation for lenderFee means lender can take more funds than expected

lenderFee is expected to be always <= 1000, however no validation is provided meaning the value could be set to anything, potentially receiving 100% of the lent amounts

Suggested Remediation

Add validation to replaceLenderFee and HomeFi.initialize

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

    /// @inheritdoc IHomeFi
    function replaceLenderFee(uint256 _newLenderFee)
        external
        override
        onlyAdmin
    {
        // Revert if no change in lender fee
        require(lenderFee != _newLenderFee, "HomeFi::!Change");

        /// @dev add a check for rational max (e.g. 20%)
        require(lenderFee < MAX_FEE, "HomeFi::!Change");

        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

L - Initialize ReentrancyGuardUpgradeable

The initializer for Community is missing the initialization for ReentrancyGuard

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L102-L110

    function initialize(address _homeFi)
        external
        override
        initializer
        nonZero(_homeFi)
    {
        // Initialize pausable. Set pause to false;
        __Pausable_init();

No funds are at risk and the functionality will be fine, however the first caller will pay additional gas to set the slot from 0 to 1

Also found:

• Disputes
• HomeFi

L - Up to 1 day - 1 second in loss of interest due to rounding decision

The math for returnToLender calculates _noOfDays with a division, and then uses that value to calculate _unclaimedInterest
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L684-L694

        // Calculate number of days difference current and last timestamp
        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) / 86400; // 24*60*60

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

Because you're dividing milliseconds by 86400 any time below that value will be rounded down to zero.

As such up to 86399 seconds may be rounded down when doing the math because of this early division

Suggested Remediation

Inline the whole operation and perform the division at the end

L - Multichain Replay Warning

From the Readme it is clear that the project will deploy on one chain.

However, it's worth stating that re-deploying the contract on any additional chain, will make both version subjects to replay attacks.

That's because, in lack of using EIP-721, using chainId as part of the signature verification, any signature will be recognized as valid on all subsequent depoyments

Suggested Remediation

Document the dangers of a multi-chain deployment and ensure code is only used in it's canonical chain

R - Unconventional apr base divisor

All financial math is expressed in basis points (BPS), denominated by 1 / 10_000

However apr is expressed as 1 / 1_000, this will cause loss of precision and may also cause honest mistakes due to most people being used to the more conventional BPS denomination.

Suggested Remediation

Refactor to express apr in BPS, or document the decision extensively and explicitly to avoid gotchas

R - Better Signing UX via EIP-712

The SignatureDecoder is expected to receive a EthProvider.Sign signature to verify it.

This is completely fine, however the UX for end users is they will be setting up their data, and then they'll be prompted by Metamask to sign a hash of unintelligible content.

A better UX is provided by using EIP-721, which allows all the fields to be previewed on Metamask (or Hardware Wallet), giving a lot more confidence to end-users as to what they are signing

More info about: EIP-712
https://medium.com/metamask/eip712-is-coming-what-to-expect-and-how-to-use-it-bb92fd1a7a26

EIP-712 Spec
https://eips.ethereum.org/EIPS/eip-712

R - Inconsistent usage of noChange

Some functions use noChange for an early revert if the function called would result in a idempotent result. However some setters do not follow this convention.

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

    modifier noChange(address _oldAddress, address _newAddress) {

Personally I'd recommend removing the modifier as there is no risk, and the check ends up costing 100+ gas.

However, for the sake of consistency, you should consider either:

• Removing noChange from all function (scrap it)
• Use noChange for all setters (Consistency)

R - Minor oversight - Internal Function is named as if it was public

claimInterest is internal but is not prefixed with a _

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L824-L825

    function claimInterest(

Suggested Remediation

Rename to _claimInterest

Additional Instances

• executeTaskAdd
• executeTaskChange
• executeTaskPay

NC - Incorrect Comments

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L513-L531

Comments refer to sender but anyone can call the function.

Suggestion:
Rephrase Revet if sender is not builder, contractor or task's subcontractor
To Revert if >Signer< is not builder, contractor or task's subcontractor

NC - Typos

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L520-L521

Revet -> Revert