Rigor Protocol contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L498-L502 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/SignatureDecoder.sol#L25

Vulnerability details

Impact

Disputes enable an actor to arbitrate & potentially enforce requested state changes. However, the current implementation does not properly implement authorization, thus anyone is able to create disputes and spam the system with invalid disputes.

Proof of Concept

Calling the Project.raiseDispute function with an invalid _signature, for instance providing a _signature with a length of 66 will return address(0) as the recovered signer address.

Project.raiseDispute

function raiseDispute(bytes calldata _data, bytes calldata _signature)
    external
    override
{
    // Recover the signer from the signature
    address signer = SignatureDecoder.recoverKey(
        keccak256(_data),
        _signature,
        0
    );

    ...
  }

SignatureDecoder.sol#L25

function recoverKey(
  bytes32 messageHash,
  bytes memory messageSignatures,
  uint256 pos
) internal pure returns (address) {
  if (messageSignatures.length % 65 != 0) {
      return (address(0));
  }

  ...
}

If _task is set to 0 and the project does not have a contractor, the require checks will pass and IDisputes(disputes).raiseDispute(_data, _signature); is called. The same applies if a specific _task is given and if the task has a subcontractor. Then the check will also pass.

Project.raiseDispute

function raiseDispute(bytes calldata _data, bytes calldata _signature)
    external
    override
{
    // Recover the signer from the signature
    address signer = SignatureDecoder.recoverKey(
        keccak256(_data),
        _signature,
        0
    );

    // Decode params from _data
    (address _project, uint256 _task, , , ) = abi.decode(
        _data,
        (address, uint256, uint8, bytes, bytes)
    );

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

    if (_task == 0) {
        // Revet if sender is not builder or contractor
        require(
            signer == builder || signer == contractor, // @audit-info if `contractor = address(0)` and the recovered signer is also the zero-address, this check will pass
            "Project::!(GC||Builder)"
        );
    } else {
        // Revet if sender is not builder, contractor or task's subcontractor
        require(
            signer == builder ||
                signer == contractor || // @audit-info if `contractor = address(0)` and the recovered signer is also the zero-address, this check will pass
                signer == tasks[_task].subcontractor,
            "Project::!(GC||Builder||SC)"
        );

        if (signer == tasks[_task].subcontractor) {
            // If sender is task's subcontractor, revert if invitation is not accepted.
            require(getAlerts(_task)[2], "Project::!SCConfirmed");
        }
    }

    // Make a call to Disputes contract raiseDisputes.
    IDisputes(disputes).raiseDispute(_data, _signature); // @audit-info Dispute will be created. Anyone can spam the system with fake disputes
}

Tools Used

Manual review

Recommended mitigation steps

Consider checking the recovered signer address in Project.raiseDispute to not equal the zero-address:

function raiseDispute(bytes calldata _data, bytes calldata _signature)
    external
    override
{
    // Recover the signer from the signature
    address signer = SignatureDecoder.recoverKey(
        keccak256(_data),
        _signature,
        0
    );

    require(signer != address(0), "Zero-address"); // @audit-info Revert if signer is zero-address

    ...
  }

Lines of code

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

Vulnerability details

Impact

The function Project.projectCost calculates the project costs by calculating the sum of all project task costs. However, due to the unbound for loop, iterating over a potentially large amount of project tasks, this function can potentially DoS due to reaching the block gas limit.

If Project.projectCost reverts due to reaching the block gas limit, other functions which call this function will revert too. The following functions are affected:

• Community.toggleLendingNeeded
• Project.lendToProject

Proof of Concept

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

function projectCost() public view override returns (uint256 _cost) {
    // Local instance of taskCount. To save gas.
    uint256 _length = taskCount;

    // Iterate over all tasks to sum their cost
    for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {tracking task costs in a storage variable instead
        _cost += tasks[_taskID].cost;
    }
}

Tools Used

Manual review

Recommended mitigation steps

Consider tracking the project costs in a storage variable instead of calculating the costs dynamically by iterating over all tasks.

Lines of code

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

Vulnerability details

Owner of project NFT has no purpose

Impact

Creating a new project mints a NFT to the _sender (builder). The builder of a project has special permissions and is required to perform various tasks.

However, if the minted NFT is transferred to a different address, the builder of a project stays the same and the new owner of the transferred NFT has no purpose and no permissions to access authorized functions in Rigor.

If real-world use-cases require a change of the builder address, there is currently no way to do so. Funds could be locked in the project contract if the current builder address is unable to perform any more actions.

Proof of Concept

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

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

Tools Used

Manual review

Recommended mitigation steps

Consider preventing transferring the project NFT to a different address for now as long as there is no use-case for the NFT owner/holder or use the actual NFT owner as the builder of a project.

Lines of code

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

Vulnerability details

Impact

The function Project.inviteContractor adds a contractor to the project. Once a contractor is set and contractorConfirmed is set to true, the contractor address can not be changed anymore.

If there are any real-world disputes between the project builder and the contractor, there is no way to part ways and define a new contractor. The project could then be stalled due to the contractor not signing multisig messages and griefing the system. Funds in the project contract could be locked as recovering the currency tokens does not work due to tasks being unfinished.

Proof of Concept

Project.sol#L123

function inviteContractor(bytes calldata _data, bytes calldata _signature)
    external
    override
{
    // Revert if contractor has already confirmed his invitation
    require(!contractorConfirmed, "Project::GC accepted"); // @audit-info If a contractor is confirmed, the contractor can not be changed anymore

    // Decode params from _data
    (address _contractor, address _projectAddress) = abi.decode(
        _data,
        (address, address)
    );

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

    // Revert if contractor address is invalid.
    require(_contractor != address(0), "Project::0 address");

    // Store new contractor
    contractor = _contractor;
    contractorConfirmed = true;

    // Check signature for builder and contractor
    checkSignature(_data, _signature);

    emit ContractorInvited(contractor);
}

Project.recoverTokens

function recoverTokens(address _tokenAddress) external override {
    /* If the token address is same as currency of this project,
        then first check if all tasks are complete */
    if (_tokenAddress == address(currency)) {
        // Iterate for each task and check if it is complete.
        uint256 _length = taskCount;
        for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
            require(tasks[_taskID].getState() == 3, "Project::!Complete");
        }
    }

    // Create token instance.
    IDebtToken _token = IDebtToken(_tokenAddress);

    // Check the balance of _token in this contract.
    uint256 _leftOutTokens = _token.balanceOf(address(this));

    // If balance is present then it to the builder.
    if (_leftOutTokens > 0) {
        _token.safeTransfer(builder, _leftOutTokens);
    }
}

Project.recoverTokens reverts if currency tokens are to be rescued as long as the project has unfinished tasks. As a task can only be set as completed by calling Project.setComplete, which in turn needs the message to be signed by the contractor, the contractor is able to grief the project.

Tools Used

Manual review

Recommended mitigation steps

Consider implementing a function that allows resetting the current contractor and then allows inviting a new contractor.