Rigor Protocol contest - saneryee's results

Lines of code

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

Vulnerability details

Impact

In the main rigor flow, There is a control point Note that you cannot submit a project with no total budget. Therefore it requires at least one task with a budget > 0. in From project creation to community lending to the project.

From project creation to community lending to the project

6. Builder publishes his project to the community. It requires signing data that includes community ID, APR, publishing fee and nonce . Both builder and community owner have to sign the data. The signatures and data are used to call publishProject(bytes _data, bytes _signature) . Note that you cannot submit a project with no total budget. Therefore it requires at least one task with a budget > 0.

However, the code to implement this control points is not found in the function publishProject(bytes _data, bytes _signature) This will invalidate the control point and potentially allow projects with no total budget to be published under some conditions.

Proof of Concept

    /// @inheritdoc ICommunity
    function publishProject(bytes calldata _data, bytes calldata _signature)
        external
        virtual
        override
        whenNotPaused
    {
        // Compute hash from bytes
        bytes32 _hash = keccak256(_data);


        // Decode params from _data
        (
            uint256 _communityID,
            address _project,
            uint256 _apr,
            uint256 _publishFee,
            uint256 _publishNonce,
            bytes memory _messageHash
        ) = abi.decode(
                _data,
                (uint256, address, uint256, uint256, uint256, bytes)
            );


        // Local instance of community and community  project details. For saving gas.
        CommunityStruct storage _community = _communities[_communityID];
        ProjectDetails storage _communityProject = _community.projectDetails[
            _project
        ];


        // Revert if decoded nonce is incorrect. This indicates wrong _data.
        require(
            _publishNonce == _community.publishNonce,
            "Community::invalid publishNonce"
        );


        // Reverts if _project not originated from HomeFi
        require(homeFi.isProjectExist(_project), "Community::Project !Exists");


        // Local instance of variables. For saving gas.
        IProject _projectInstance = IProject(_project);
        address _builder = _projectInstance.builder();


        // Revert if project builder is not community member
        require(_community.isMember[_builder], "Community::!Member");


        // Revert if project currency does not match community currency
        require(
            _projectInstance.currency() == _community.currency,
            "Community::!Currency"
        );


        // check signatures
        checkSignatureValidity(_community.owner, _hash, _signature, 0); // must be community owner
        checkSignatureValidity(_builder, _hash, _signature, 1); // must be project builder


        // If already published then unpublish first
        if (projectPublished[_project] > 0) {
            _unpublishProject(_project);
        }


        // Store updated details
        _community.publishNonce = ++_community.publishNonce;
        _communityProject.apr = _apr;
        _communityProject.publishFee = _publishFee;
        projectPublished[_project] = _communityID;


        // If _publishFee is zero than mark publish fee as paid
        if (_publishFee == 0) _communityProject.publishFeePaid = true;


        emit ProjectPublished(
            _communityID,
            _project,
            _apr,
            _publishFee,
            _communityProject.publishFeePaid,
            _messageHash
        );
    }

Recommended Mitigation Steps

Add code like the following to the function publishProject(bytes _data, bytes _signature)

// Revert if no total budget.
require(_total_budget > 0, "Project::!value>0");

Summary

Low Risk Issues

Total: 6 instances over 1 issues

Low Risk Issues

1. Too many similar modifier

There are 5 same functional noZero modifiers in five contracts( HomeFiProxy.sol， HomeFi.sol, ProjectFactory.sol, DebtToken.sol, Disputes.sol, Community.sol). Then there are two places in the project.sol where the noZero address is determined, but the modifier is not used.

With so many duplicate codes spread across different codes, this brings a potential risk for the contract. For example, instance # 5 is missing a comment compared to the other 4 codes. Although this comment does not affect the contract, it poses a high-level risk if you will change the business logic in the future.

There are 6 instances of this issue:

File: contracts/HomeFi.sol #1

77: modifier nonZero(address _address) {
78:    require(_address != address(0), "HomeFi::0 address");
79:     _;
80: }

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

File: contracts/ProjectFactory.sol #2

34:    modifier nonZero(address _address) {
35:       // Ensure an address is not the zero address (0x00)
36:        require(_address != address(0), "PF::0 address");
37:       _;
38:    }

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L34-L38

File: contracts/Disputes.sol #3

37:    modifier nonZero(address _address) {
38:       // Ensure an address is not the zero address (0x00)
39:        require(_address != address(0), "Disputes::0 address");
40:       _;
41:    }

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

File: contracts/Community.sol #4

67:    modifier nonZero(address _address) {
68:       // Ensure an address is not the zero address (0x00)
69:        require(_address != address(0), "Community::0 address");
70:       _;
71:    }

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

File: contracts/HomeFiProxy.sol #5

40:    modifier nonZero(address _address) {
41:        require(_address != address(0), "Proxy::0 address");
42:       _;
43:    }

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

The noZero address is determined, but the modifier is not used.

File: contracts/Project.sol #6

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

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

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

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