Rigor Protocol contest - MEP's results

Lines of code

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

Vulnerability details

In Project.sol, function updateProjectHash L162, _data (which is signed by builder and/or contractor) does not contain a reference to the project address. In all other external functions of Project.sol, _data contains the address of the project, used in this check: require(_projectAddress == address(this), "Project::!projectAddress");. The lack of this verification makes it possible to reuse the same _data, and the same _signature on another project, in the case the latter has the same builder and/or contractor, and the same _nonce. In pratice, if the same group of people starts a new project, when _nonce reaches the correct value, anyone can change the hash of a task (if we suppose that that updateTaskHash() was used in the previous project).

• In porjectFactory.sol L78 function createProject is under the EXTERNAL VIEWS commentary while not beeing a view function.
• HomeFi.sol function isProjectExist naming is not correct english. doesProjectExist is better
• The function getTask in the contract Project does not take the same argument name as indicated in the interface IProject, it is id instead of _taskId.
• In Community.sol L686, consider using 1 days instead of 86400.
• In Tasks.sol L16 the mapping alerts should be of type mapping(Lifecycle => bool). Some other parts of the code would need to be modified (remove conversions to uint256).
• HomeFi.sol, function createProject L210: everyone is free to create a project with any given hash, even the hash of an already existing project. This does not cause direct issue inside the protocol, but could be the source of some troubles if other protocols base their trust on the hash of the project. Possible fix: create a mapping (hash => bool) to avoid collisons.
• In the contract Project, the function projectCost can run out of gas if there are too many tasks. It would block the functions lendToProject and toggleLendingNeeded, that are important functions. Suggested fix: update the project cost in a storage variable each time a task is added of updated.
• For the builder to repay totally a loan, he has to give exactly the amount owed, if he gives too much, the function _reduceDebt will revert. But, because the lender is gaining interests at each block, it will be very difficult for the builder to give exacly the amount owed, so the debt can be endless. Seggested fix: accept giving more value that owed, and let the function cap the amout by itslef.

• In Community.sol L393-394, save _projectInstance.lenderFee() in a variable instead of calling it two times.
• In the contract Community in the function lendToProject, defining two storage variables as CommunityStruct storage community = _communities[_communityID] and ProjectDetails storage project = community.projectDetails[_project] would save gas (limit the scope of some variables to avoid stack too deep). Ex:

function lendToProject(
    uint256 _communityID,
    address _project,
    uint256 _lendingAmount,
    bytes calldata _hash
)
    external
    virtual
    override
    nonReentrant
    whenNotPaused
    isPublishedToCommunity(_communityID, _project)
{
    // Local instance of variable. For saving gas.
    address _sender = _msgSender();

    CommunityStruct storage community = _communities[_communityID];

    // Revert if sender is not community owner.
    // Only community owner can lend.
    require(
        _sender == community.owner,
        "Community::!owner"
    );

    // Local instance of variable. For saving gas.
    IProject _projectInstance = IProject(_project);

    // Calculate lenderFee
    uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
        (_projectInstance.lenderFee() + 1000);

    // Calculate amount going to project. Lending amount - lending fee.
    uint256 _amountToProject = _lendingAmount - _lenderFee;

    ProjectDetails storage project = community.projectDetails[_project];

    // Revert if _amountToProject is not within further investment needed.
    require(
        _amountToProject <= project.lendingNeeded - project.totalLent,
        "Community::lending>needed"
    );

    {
        // Local instance of variable. For saving gas.
        IDebtToken _currency = community.currency;
        IDebtToken _wrappedToken = IDebtToken(
            homeFi.wrappedToken(address(_currency))
        );

        // Update investment in Project
        _projectInstance.lendToProject(_amountToProject);

        // Update total lent by lender
        project.totalLent += _amountToProject;

        // First claim interest if principal lent > 0
        if (
            project.lentAmount > 0
        ) {
            claimInterest(_communityID, _project, _wrappedToken);
        }

        // Increment lent principal
        project.lentAmount += _lendingAmount;

        // Update lastTimestamp
        project.lastTimestamp = block.timestamp;

        // Transfer _lenderFee to HomeFi treasury from lender account
        _currency.safeTransferFrom(_msgSender(), homeFi.treasury(), _lenderFee);

        // Transfer _amountToProject to _project from lender account
        _currency.safeTransferFrom(_msgSender(), _project, _amountToProject);

        // Mint new _lendingAmount amount wrapped token to lender
        _wrappedToken.mint(_sender, _lendingAmount);
    }

    emit LenderLent(_communityID, _project, _sender, _lendingAmount, _hash);
}

• for loops can be optimized, the most optimized loop is:

for (uint256 i; i < length;) {
    // content
    unchecked {
        ++i;
    }
}

and if the iteration is over an array list, store its length in a variable before the loop instead of computing it at each iteration (same for all other storage variables). Unoptimized loops appear in Community.sol L624, in HomeFiProxy.sol L87, L136, in Project.sol L248, L311, L322, L368, L603, L650, L710, in Tasks.sol L181.