Rigor Protocol contest - dipp's results

1. Check if receiver can receive ERC721

Line References

HomeFi.sol#L292

Impact

In HomeFi.sol, when creating a project the mintNFT function is called which uses the ERC721Upgradeable _mint function. This function does not check if the recipient is an EOA or a contract that is able to receive ERC721 tokens.

Consider using the _safeMint function instead.

2. Able to call changeOrder for a completed task

Line References

Project.sol#L386-L490

Impact

The changeOrder function in Project.sol is used to change a task's subcontractor or cost. The function does not check if the task being changed is completed or not. If the function is called for a completed task then the totalAllocated state variable may be affected which could lead some functions, such as allocatedFunds, to behave unexpectedly.

If the new cost is less than the old cost, the difference is withdraw and sent to the builder of the project. If the builder was malicious then this issue could be used to rug the project.

Since the changeOrder function requires valid signatures, it is unlikely that it would be called for a completed task.

Proof of Concept

The code below shows how changeOrder changes the totalAllocated state variable if the new cost != the old cost.

        // If new task cost is less than old task cost.
        if (_newCost < _taskCost) {
            // Find the difference between old - new.
            uint256 _withdrawDifference = _taskCost - _newCost;

            // Reduce this difference from total cost allocated.
            // As the same task is now allocated with lesser cost.
            totalAllocated -= _withdrawDifference;

            // Withdraw the difference back to builder's account.
            // As this additional amount may not be required by the project.
            autoWithdraw(_withdrawDifference);
        }
        // If new cost is more than task cost but total lent is enough to cover for it.
        else if (totalLent - _totalAllocated >= _newCost - _taskCost) {
            // Increase the difference of new cost and old cost to total allocated.
            totalAllocated += _newCost - _taskCost;
        }
        // If new cost is more than task cost and totalLent is not enough.
        else {
            // Un-confirm SC, mark task as inactive, mark allocated as false, mark lifecycle as None

            // Mark task as inactive by unapproving subcontractor.
            // As subcontractor can only be approved if task is allocated
            _unapproved = true;
            tasks[_taskID].unApprove();

            // Mark task as not allocated.
            tasks[_taskID].unAllocateFunds();

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

            // Add this task to _changeOrderedTask array. These tasks will be allocated first.
            _changeOrderedTask.push(_taskID);
        }

Recommended Mitigation Steps

Consider checking if a task is complete in the changeOrder function and reverting