Rigor Protocol contest - saian's results

Low severity findings

Precision loss in interest calculation

In function returnToLender, if the difference between lasttimestamp and current timestamp is not a multiple of 86400 the value get rounded down, so even if 1.9 days has passed the value get rounded to 1 and the communityProject.lastTimestamp updated to the current value. So the interest is calculated on 1 days while 1.9 days is updated

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L685

        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) / 86400;

Recommended mitigation steps

Multiply and divide by a value to avoid precision loss

        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) * VAL / 86400; // 24*60*60   

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

Non-critical findings

Lender fee can be set to a high value

The function replaceLenderFee does not contain input validation, so the fee can be set to a very high value, projects deployed using the value will have a high lending fee

Proof of concept

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

    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);
    }

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L393

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

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

Recommended Mitigation steps

Add input validation to validate the lender fee

LenderFee is not updated in projects

Lenderfee updated by admin in HomeFi.sol is not reflected in project already initialized, So lenderfee queried in Community.sol will be the value initialized in the project

Proof of concept

    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);
    }

    function initialize(
        address _currency,
        address _sender,
        address _homeFiAddress
    ) external override initializer {
        ...
        lenderFee = homeFi.lenderFee(); 

Recommended Mitigation steps

Replace _projectInstance.lenderFee() with homeFi.lenderFee()

Replace _mint with _safeMint

Function _mint can be replaced with _safeMint which checks if the receiver can handle ERC721 tokens and prevent it from being stuck in the contracts

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/a42bf0a79260458b5bb2ea3dcf55947e98383606/contracts/token/ERC721/ERC721Upgradeable.sol#L275

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L292

        _mint(_to, projectCount);

Contract name and implementation mismatch

Owner has to make sure to send the contract address in the right order of contract names. Accidentally sending input in the wrong order will result in mismatch of name and proxy address

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol#L58

        allContractNames.push("HF"); // HomeFi
        allContractNames.push("CN"); // Community
        allContractNames.push("DP"); // Disputes
        allContractNames.push("PF"); // Project Factory
        allContractNames.push("DA"); // rDAI
        allContractNames.push("US"); // rUSDC
        allContractNames.push("NT"); // native token rETH - rXDAI

        // Local instance of variable. For saving gas.
        uint256 _length = allContractNames.length;

        // Revert if _implementations length is wrong. Indicating wrong set of _implementations.
        require(_length == _implementations.length, "Proxy::Lengths !match");

        // Mark this contract as active
        contractsActive[address(this)] = true;

        // Generate proxy for all implementation
        for (uint256 i = 0; i < _length; i++) {
            _generateProxy(allContractNames[i], _implementations[i]);
        }

Add unchecked to save gas

In Solidity 0.8.4 and above there is a default underflow/overflow checks on unsigned integers. it is possible to avoid this check by using unchecked. If the expression does not cause overflow/underflow, it can be unchecked to save some gas.

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L794

        require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
        _interest = 0;
        _lentAmount = _lentAndInterest - _repayAmount;


L785    if (_repayAmount > _interest) {
            ...
        } else {
            _interest -= _repayAmount;
        }

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L425

        if (_newCost < _taskCost) {
            // Find the difference between old - new.
            uint256 _withdrawDifference = _taskCost - _newCost;

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L614

        if (_costToAllocate >= _taskCost) {
            // Reduce task cost from _costToAllocate
            _costToAllocate -= _taskCost;

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L661

        if (_costToAllocate >= _taskCost) {
            // Reduce task cost from _costToAllocate
            _costToAllocate -= _taskCost;

> 0 is less efficient than != 0 for unsigned integers

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L195

    require(_cost > 0, "Project::!value>0"); 

Pre-increment(++i) costs less than post-increment(i++)

Pre-increment saves a small amount of gas than postfix increment because it doesnt have to store the previous value. This can be more significant in loops where this operation is done multiple times.

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol#L87

    for (uint256 i = 0; i < _length; i++)

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol#L136

    for (uint256 i = 0; i < _length; i++) 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L248

        for (uint256 i = 0; i < _length; i++)

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L311

        for (uint256 i = 0; i < _length; i++)

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L322

        for (uint256 i = 0; i < _length; i++)

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L368

        for (uint256 _taskID = 1; _taskID <= _length; _taskID++) 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L650

        for (++j; j <= taskCount; j++) {

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L710

        for (uint256 _taskID = 1; _taskID <= _length; _taskID++) { 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/libraries/Tasks.sol#L181

        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L624

        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++)

external call value can be cached and reused

The return value of external functions that are called multiple times can be cached and reused to save gas

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L323

        tasks[_taskList[i]].acceptInvitation(_msgSender());

Function calls can be replaced with cached value in _sender

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L446

L380    address _sender = _msgSender();

        _currency.safeTransferFrom(_msgSender(), homeFi.treasury(), _lenderFee);

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

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L505

        emit ApproveHash(_hash, _msgSender());

Split require statement that use && and save gas

require statements that use && operator to check multiple condition can be split into multiple statements of one condition to save gas

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Disputes.sol#L61

       require(
            _disputeID < disputeCount &&
                disputes[_disputeID].status == Status.Active,
            "Disputes::!Resolvable"
        ); 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Disputes.sol#L106

        require(
            _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
            "Disputes::!ActionType"
        ); 

Cache storage variable and save gas

Storage variables that are read more than once can be cached in a temporary variable to save a SLOAD(100 gas) operation and save gas

Proof of concept

projectCount in

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L231

        projects[projectCount] = _project;
        projectTokenId[_project] = projectCount;

        emit ProjectAdded(projectCount, _project, _sender, _currency, _hash); 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L295

        projectCount += 1;

        // Mints NFT and set token URI
        _mint(_to, projectCount);   
        _setTokenURI(projectCount, _tokenURI);

        emit NftCreated(projectCount, _to); 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/ProjectFactory.sol#L90

        Project(_clone).initialize(_currency, _sender, homeFi);

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L144

        emit ContractorInvited(contractor);

Array length can be cached in a variable and reused

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L603

L603        for (; i < _changeOrderedTask.length; i++)

L635        if (i == _changeOrderedTask.length) {

taskCount in

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L650

L650    for (++j; j <= taskCount; j++) {    

L681    if (j > taskCount) {
                lastAllocatedTask = taskCount;
            }

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L150

        emit CommunityAdded(communityCount, _sender, _currency, _hash);

_communities[_communityID].memberCount in

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L624

        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) 

Use custom errors than require statements

As the code is already using a compiler version >= 0.8.4, there is a more convenient and gas-efficient way to handle revert strings: custom errors. Custom errors decrease both deploy and runtime gas costs. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol

    require(_address != address(0), "Proxy::0 address");

    require(_length == _implementations.length, "Proxy::Lengths !match");

    require(
        contractAddress[_contractName] == address(0),
        "Proxy::Name !OK"
    );
    require(_length == _contractAddresses.length, "Proxy::Lengths !match"); 

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol

    require(!contractorConfirmed, "Project::GC accepted");
    require(_projectAddress == address(this), "Project::!projectAddress");
    require(_contractor != address(0), "Project::0 address");
    require(_msgSender() == builder, "Project::!B");
    require(contractor != address(0), "Project::0 address");
    require(_nonce == hashChangeNonce, "Project::!Nonce");
    require(
        _sender == builder || _sender == homeFi.communityContract(),
        "Project::!Builder&&!Community"
    );
    require(_cost > 0, "Project::!value>0"); 
    require(
        projectCost() >= uint256(_newTotalLent),
        "Project::value>required"
    );
    require(_taskCount == taskCount, "Project::!taskCount");
    require(_projectAddress == address(this), "Project::!projectAddress");
    require(_length == _taskCosts.length, "Project::Lengths !match");
    require(_nonce == hashChangeNonce, "Project::!Nonce");
    require(
        _msgSender() == builder || _msgSender() == contractor,
        "Project::!Builder||!GC"
    );
    require(_length == _scList.length, "Project::Lengths !match");
    require(_projectAddress == address(this), "Project::!Project");
            require(tasks[_taskID].getState() == 3, "Project::!Complete");
    require(_project == address(this), "Project::!projectAddress");
    require(_project == address(this), "Project::!projectAddress");
    require(
        signer == builder || signer == contractor,
        "Project::!(GC||Builder)"
    );
                require(
        signer == builder ||
            signer == contractor ||
            signer == tasks[_task].subcontractor,
        "Project::!(GC||Builder||SC)"
    );
    require(getAlerts(_task)[2], "Project::!SCConfirmed");
    require(_sc != address(0), "Project::0 address");
    require(
        _recoveredSignature == _address || approvedHashes[_address][_hash],
        "Project::invalid signature"
    );
            require(
        ((_amount / 1000) * 1000) == _amount,
        "Project::Precision>=1000"
    );

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol

    require(admin == _msgSender(), "HomeFi::!Admin");
    require(_address != address(0), "HomeFi::0 address");
    require(_oldAddress != _newAddress, "HomeFi::!Change");
    require(!addrSet, "HomeFi::Set");
    require(lenderFee != _newLenderFee, "HomeFi::!Change");
    require(
        _currency == tokenCurrency1 ||
            _currency == tokenCurrency2 ||
            _currency == tokenCurrency3,
        "HomeFi::!Currency"
    );

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Disputes.sol

        require(_address != address(0), "Disputes::0 address");
        require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
        require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
        require(
            _disputeID < disputeCount &&
                disputes[_disputeID].status == Status.Active,
            "Disputes::!Resolvable"
        ); 
                require(
            _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
            "Disputes::!ActionType"
        );
        require(_result, "Disputes::!Member");

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/ProjectFactory.sol

    require(_address != address(0), "PF::0 address");
    require(
        _msgSender() == IHomeFi(homeFi).admin(),
        "ProjectFactory::!Owner"
    );
    require(_msgSender() == homeFi, "PF::!HomeFiContract");

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/DebtToken.sol

    require(
        communityContract == _msgSender(),
        "DebtToken::!CommunityContract"
    );
    require(_communityContract != address(0), "DebtToken::0 address");

    revert("DebtToken::blocked");

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/libraries/Tasks.sol

        require(_self.subcontractor == _sc, "Task::!SC");

Using bool incurs storage overhead

Use uint256(0) & uint256(1) for false and true instead of the boolean variable

Proof of concept

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFiProxy.sol#L30

    mapping(address => bool) internal contractsActive;

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.