QA Report

[code423n4]

1. 1-step ownership change (non-critical)

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

HomeFi sets a new admin immediately:

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

HomeFi's admin:

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

        emit AdminReplaced(_newAdmin);
    }

HomeFiProxy calls for the proxyAdmin ownership transfer immediately:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L150-L157

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

Recommended Mitigation Steps

Consider utilizing two-step ownership transferring process (proposition and acceptance in separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system

2. Lender fee is uncapped

Consider checking for the validity of the new fee (1-1000):

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

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

        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

3. Comment is incomplete

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

        // Emit event if _hash. This way this hash needs not be stored in memory.
        emit UpdateCommunityHash(_communityID, _hash);

Recommended Mitigation Steps

Update the comment, for example: // Emit event broadcasting new _hash. This way this hash needs not be stored in memory

4. Comment claims burn instead of mint

claimInterest() mints new _wrappedToken, not burns it:

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

            // Burn _interestEarned amount wrapped token to lender
            _wrappedToken.mint(_lender, _interestEarned);

Recommended Mitigation Steps

Should be mint as the debt record is increased

5. Typos in comments

To be Revertx2:

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

            // Revet if sender is not builder or contractor
            require(
                signer == builder || signer == contractor,
                "Project::!(GC||Builder)"
            );
        } else {
            // Revet if sender is not builder, contractor or task's subcontractor

To be // As it needs to go though:

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

                    // Reduce total allocation by old task cost.
                    // As as needs to go though funding process again.
                    totalAllocated -= _taskCost;

6. Incorrect function description

unApprove() description doesn't match the logic:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L153-L164

    /**
     * @dev Set a task as un accepted/approved for SC

     * @dev modifier onlyActive

     * @param _self Task the task being set as funded
     */
    function unApprove(Task storage _self) internal {
        // State/ lifecycle //
        _self.alerts[uint256(Lifecycle.SCConfirmed)] = false;
        _self.state = TaskStatus.Inactive;
    }

7. raiseDispute doesn't check for signatory validity

Whenever _signature isn't valid there is no revert, but zero _signer will be put to disputes structure:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L84-L116

    function raiseDispute(bytes calldata _data, bytes calldata _signature)
        external
        override
        onlyProject
    {
        // Recover signer from signature
        address _signer = SignatureDecoder.recoverKey(
            keccak256(_data),
            _signature,
            0
        );

        // Decode params from _data
        (
            address _project,
            uint256 _taskID,
            uint8 _actionType,
            bytes memory _actionData,
            bytes memory _reason
        ) = abi.decode(_data, (address, uint256, uint8, bytes, bytes));

        // Revert if _actionType is invalid
        require(
            _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
            "Disputes::!ActionType"
        );

        // Store dispute details
        Dispute storage _dispute = disputes[disputeCount];
        _dispute.status = Status.Active;
        _dispute.project = _project;
        _dispute.taskID = _taskID;
        _dispute.raisedBy = _signer;

8. Project's changeOrder uses hard coded TaskAllocated constant

changeOrder() hard codes 1 as TaskAllocated enumeration item:

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

            // If tasks are already allocated with old cost.
            if (tasks[_taskID].alerts[1]) {

Recommended Mitigation Steps

Consider using the constant to improve readability and reduce operation error surface:

            // If tasks are already allocated with old cost.
-           if (tasks[_taskID].alerts[1]) {
+           if (tasks[_taskID].alerts[Lifecycle.TaskAllocated]) {

[parv3213]

1. 1-step ownership change (non-critical). Duplicate: Single-step process for critical ownership transfer/renounce is risky #368. @code423n4.
2. Duplicate: Missing upper limit definition in replaceLenderFee() of HomeFi.sol #400.
   3-6: Good documentation fixes.
3. Invalid as raiseDispute has onlyProject modifier. And signatures are checked inside Project's raiseDispute.
4. Good suggestion for better code quality.

[dmitriia]

Yes, Disputes' raiseDispute() one is actually Gas, as the same signature is checked twice, listed it there. I.e. the mitigation here is just to remove the second signature verification, calling from Project with already verified signer as an argument instead