QA Report

[code423n4]

Summary

Overall, the protocol was very well-developed with good readability. I very much liked how well the project was commented and how much work was put into helping wardens understand the protocol (particularly the flow descriptions and diagrams). As this project is quite special in that it has a defined use case outside the blockchain, it loses a lot of the security which comes from the blockchain, mainly trustlessness. There is a very heavy human element required for the protocol to work correctly, for example deciding disputes, which if compromised can cause massive damages and reputational loss.

Issue 1 Builder is not forced to pay back a loan

I have talked to the sponsors about this and agreed that this issue is mostly invalid because it is very likely that the builder must sign a legal contract. Additionally, they said that they plan to use KYC checks, so legal consequences can be enforced if a builder refuses to pay back a loan.

An idea I think which might be interesting is to use a smart contract which acts as an escrow contract. Instead of sending funds to the builder or subcontractor e.g. in autoWithdraw(), it may be sensible to send all funds to the escrow contract and only when the community owner approves can the funds be sent to a builder/subcontractor. This would prevent the builder running off with the loan because they would never own any of the assets at anytime. Additionally, this would prevent a builder from illegitimately sending funds to themselves indirectly (by completing their own tasks as a subcontractor) because all funding will have to be approved by the community owner. Of course, if the community owner does not want this responsibility, they can delegate all of this to the builder and so nothing changes.

I simply included this issue because I thought it might be an interesting idea which the sponsor may want to think about/build upon for a potential fix for this problem

Issue 2 HomeFi admin can easily rug all assets

One problem is that the HomeFi admin is able to control numerous operations in the protocol such as upgrading contracts, deciding disputes and changing protocol fees. If the admin is malicious, then all assets can be stolen by upgrading to a malicious contract which sends all funds to the admin. Therefore, I think it is very important that the admin is either a multi-sig wallet (which still has its problems) or even better a DAO. It may also be wise to separate admins for deciding disputes and all other operations, as deciding disputes requires faster responses than upgrading contracts.

Issue 3 Precision in checkPrecision() is independent of token decimals

The code for checkPrecision() is:

    function checkPrecision(uint256 _amount) internal pure {
        // Divide and multiply amount with 1000 should be equal to amount.
        // This ensures the amount is not too precise.
        require(
            ((_amount / 1000) * 1000) == _amount,
            "Project::Precision>=1000"
        );
    }

This, however, does not account for the token decimals being used e.g. USDC which is one of the currencies being used has 6 decimals while DAI has 18 decimals. Therefore 1000 is much more significant when using USDC then when using DAI

Consider changing the function to something like:

    function checkPrecision(uint256 _amount, uint256 decimals) internal pure {
        // Divide and multiply amount with 1000 should be equal to amount.
        // This ensures the amount is not too precise.
        uint256 precision = 10**(decimals-3);
        require(
            ((_amount / precision) * precision) == _amount,
            "Project::Precision Too Low"
        );
    }

Issue 4 __ReentrancyGuard_init() is not used

Several contracts including Community.sol, Disputes.sol, HomeFi.sol and Project.sol use ReentrancyGuardUpgradeable but do not initialize it. Consider, initializing it.

Issue 5 projectCost() checks can be easily bypassed

lendToProject() In Project.sol and toggleLendingNeeded() in Community.sol use projectCost() to make sure that the lending given is less than the total sum of costs of tasks required in the project. This can be easily bypassed by the builder by simply initializing new tasks with arbitrary costs and then simply setting the cost to zero later after a loan is given out. This makes the check completely defunct so it may be worthwhile to simply remove the use of this entire function