Rigor Protocol contest - hake's results

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Admin transfer should be a two-step process

If ownership is accidentally transferred to an inactive account all functionalities depending on it will be lost. 1 instance of this issue has been found:

[L-01] HomeFi.sol#L157-L168

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

        emit AdminReplaced(_newAdmin);
    }

[L-02] Strict equality might lead to unexpected revert

Strict equallity might lead to eventual revert and loss of gas for users. Consider accepting a range above required and reimburse the difference. 1 instance of this issue has been found:

[L-02] Project.sol#L199-L202

        require(
            projectCost() >= uint256(_newTotalLent),
            "Project::value>required"
        );

[L-03] Fee-on-transfer token not supported

Transfer amount is currently based on input rather than difference in balance before and after the transfer. If protocol ever wishes to accept/support fee-on-transfer tokens it should follow the latter. 8 instances of this issue have been found:

[L-03] Community.sol#L474-L475

        _communities[_communityID].currency.safeTransferFrom(

[L-03b] Community.sol#L446-L447

        _currency.safeTransferFrom(_msgSender(), _project, _amountToProject);

[L-03c] Community.sol#L443-L444

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

[L-03d] Community.sol#L321-L322

        _community.currency.safeTransferFrom(

[L-03e] Project.sol#L775-L776

        currency.safeTransfer(builder, _amount);

[L-03f] Project.sol#L381-L382

            _token.safeTransfer(builder, _leftOutTokens);

[L-03g] Project.sol#L353-L354

        currency.safeTransfer(

[L-03h] Project.sol#L206-L207

            currency.safeTransferFrom(_sender, address(this), _cost);

Non-critical Findings

[N-01] The use of magic numbers is not recommended

Consider setting constant numbers as a constant variable for better readability and clarity. 4 instances of this issue have been found:

[N-01] Community.sol#L690-L694

        uint256 _unclaimedInterest = 
                _lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
                _noOfDays /
                365000;

[N-01b] Community.sol#L686-L687

            _communityProject.lastTimestamp) / 86400; // 24*60*60

[N-01c] Community.sol#L394-L395

            (_projectInstance.lenderFee() + 1000);

[N-01d] Project.sol#L906-L909

        require(
            ((_amount / 1000) * 1000) == _amount,
            "Project::Precision>=1000"
        );

[N-02] Critical functions should return a value

Providing a return value improves code clarity and readability. 2 instances of this issue have been found:

[N-02] Project.sol#L219-L264

 function addTasks(bytes calldata _data, bytes calldata _signature)
        external
        override
    {
        // If the sender is disputes contract, then do not check for signatures.
        if (_msgSender() != disputes) {
            // Check for required signatures
            checkSignature(_data, _signature);
        }

        // Decode params from _data
        (
            bytes[] memory _hash,
            uint256[] memory _taskCosts,
            uint256 _taskCount,
            address _projectAddress
        ) = abi.decode(_data, (bytes[], uint256[], uint256, address));

        // Revert if decoded taskCount is incorrect. This indicates wrong data.
        require(_taskCount == taskCount, "Project::!taskCount");

        // Revert if decoded project address does not match this contract. Indicating incorrect _data.
        require(_projectAddress == address(this), "Project::!projectAddress");

        // Revert if IPFS hash array length is not equal to task cost array length.
        uint256 _length = _hash.length;
        require(_length == _taskCosts.length, "Project::Lengths !match");

        // Loop over all the new tasks.
        for (uint256 i = 0; i < _length; i++) {
            // Increment local task counter.
            _taskCount += 1;

            // Check task cost precision. Revert if too precise.
            checkPrecision(_taskCosts[i]);

            // Initialize task.
            tasks[_taskCount].initialize(_taskCosts[i]);
        }

        // Update task counter equal to local task counter.
        taskCount = _taskCount;

        emit TasksAdded(_taskCosts, _hash);
    }

[N-02b] HomeFi.sol#L210-L232

    function createProject(bytes memory _hash, address _currency)
        external
        override
        nonReentrant
    {
        // Revert if currency not supported by HomeFi
        validCurrency(_currency);

        address _sender = _msgSender();

        // Create a new project Clone and mint a new NFT for it
        address _project = projectFactoryInstance.createProject(
            _currency,
            _sender
        );
        mintNFT(_sender, string(_hash));

        // Update project related mappings
        projects[projectCount] = _project;
        projectTokenId[_project] = projectCount;

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

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operatio 1 instance of this issue has been found:

[G-01] HomeFi.sol#L64-L66

    mapping(address => uint256) public override projectTokenId;
    /// @inheritdoc IHomeFi
    mapping(address => address) public override wrappedToken;

[G-02] require()/revert() checks used multiples times should be turned into a function or modifier

Doing so increases code readability decreases number of instructions for the compiler. 4 instances of this issue have been found:

[G-02] Project.sol#L153-L154

        require(contractor != address(0), "Project::0 address");

[G-02b] Project.sol#L135-L136

        require(_contractor != address(0), "Project::0 address");

[G-02c] DebtToken.sol#L96-L97

        revert("DebtToken::blocked");

[G-02d] DebtToken.sol#L104-L105

        revert("DebtToken::blocked");

[G-03] bool is gas inefficient when used in storage

Instead use uint256 values to represent true/false instead. Reference 3 instances of this issue have been found:

[G-03] Project.sol#L78-L79

    bool public override contractorDelegated;

[G-03b] Project.sol#L68-L69

    bool public override contractorConfirmed;

[G-03c] HomeFi.sol#L50-L51

    bool public override addrSet;

[G-04] Redundant require check

Modifier noChange can be used instead. 1 instance of this issue has been found:

[G-04] HomeFi.sol#L191-L192

        require(lenderFee != _newLenderFee, "HomeFi::!Change");

[G-05] Implementing return is redundant if function already has a named returns method implemented

Redundant return methods increase gas on deployment and execution. 2 instances of this issue have been found:

[G-05] HomeFi.sol#L318-L321

    returns (bytes calldata)
    {
        // We want to use the _msgData() implementation of ERC2771ContextUpgradeable
        return super._msgData();

[G-05b] HomeFi.sol#L307-L310

        returns (address sender)
    {
        // We want to use the _msgSender() implementation of ERC2771ContextUpgradeable
        return super._msgSender();

[G-06] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

2 instances of this issue have been found:

[G-06] Community.sol#L624-L625

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

[G-06b] Project.sol#L603-L604

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

[G-07] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

6 instances of this issue have been found:

[G-07] Community.sol#L624-L625

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

[G-07b] Project.sol#L650-L651

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

[G-07c] Project.sol#L322-L323

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

[G-07d] Project.sol#L311-L312

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

[G-07e] Project.sol#L248-L249

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

[G-07f] Project.sol#L603-L604

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

[G-08] Using prefix(++i) instead of postfix (i++) saves gas

It saves 6 gas per iteration. 7 instances of this issue have been found:

[G-08] Community.sol#L624-L625

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

[G-08b] Project.sol#L650-L651

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

[G-08c] Project.sol#L322-L323

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

[G-08d] Project.sol#L311-L312

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

[G-08e] Project.sol#L248-L249

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

[G-08f] Project.sol#L625-L626

                    _loopCount++;

[G-08g] Project.sol#L603-L604

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

[G-09] Setting variable to default value is redundant

Setting variable to default value is redundant. 4 instances of this issue have been found:

[G-09] Community.sol#L624-L625

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

[G-09b] Project.sol#L322-L323

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

[G-09c] Project.sol#L311-L312

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

[G-09d] Project.sol#L248-L249

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

[G-10] Use custom errors rather than revert()/require() strings to save gas

Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. 20 instances of this issue have been found:

[G-10] Project.sol#L341-L342

        require(_projectAddress == address(this), "Project::!Project");

[G-10b] Project.sol#L199-L202

        require(
            projectCost() >= uint256(_newTotalLent),
            "Project::value>required"
        );

[G-10c] Project.sol#L195-L196

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

[G-10d] Project.sol#L189-L192

        require(
            _sender == builder || _sender == homeFi.communityContract(),
            "Project::!Builder&&!Community"
        );

[G-10e] Project.sol#L176-L177

        require(_nonce == hashChangeNonce, "Project::!Nonce");

[G-10f] Project.sol#L153-L154

        require(contractor != address(0), "Project::0 address");

[G-10g] Project.sol#L150-L151

        require(_msgSender() == builder, "Project::!B");

[G-10h] Project.sol#L135-L136

        require(_contractor != address(0), "Project::0 address");

[G-10i] Project.sol#L132-L133

        require(_projectAddress == address(this), "Project::!projectAddress");

[G-10j] Project.sol#L123-L124

        require(!contractorConfirmed, "Project::GC accepted");

[G-10k] HomeFi.sol#L255-L260

        require(
            _currency == tokenCurrency1 ||
                _currency == tokenCurrency2 ||
                _currency == tokenCurrency3,
            "HomeFi::!Currency"
        );

[G-10l] HomeFi.sol#L191-L192

        require(lenderFee != _newLenderFee, "HomeFi::!Change");

[G-10m] HomeFi.sol#L142-L143

        require(!addrSet, "HomeFi::Set");

[G-10n] HomeFi.sol#L84-L85

        require(_oldAddress != _newAddress, "HomeFi::!Change");

[G-10o] HomeFi.sol#L78-L79

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

[G-10p] HomeFi.sol#L73-L74

        require(admin == _msgSender(), "HomeFi::!Admin");

[G-10q] DebtToken.sol#L31-L35

        require(
            communityContract == _msgSender(),
            "DebtToken::!CommunityContract"
        );
        _;

[G-10r] DebtToken.sol#L50-L51

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

[G-10s] DebtToken.sol#L96-L97

        revert("DebtToken::blocked");

[G-10t] DebtToken.sol#L104-L105

        revert("DebtToken::blocked");