New subcontractor can be set for a SCConfirmed task without current subcontractor consent

[code423n4]

Lines of code

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

Vulnerability details

Malicious builder/contractor can change the subcontractor for any task even if all the terms was agreed upon and work was started/finished, but the task wasn't set to completed yet, i.e. it's SCConfirmed, getAlerts(_taskID)[2] == true. This condition is not checked by inviteSC().

For example, a contractor can create a subcontractor of her own and front run valid setComplete() call with a sequence of inviteSC(task, own_subcontractor) -> setComplete() with a signatory from the own_subcontractor, stealing the task budget from the subcontractor who did the job. Contractor will not breach any duties with the community as the task will be done, while raiseDispute() will not work for a real subcontractor as the task record will be already changed.

Setting the severity to be high as this creates an attack vector to fully steal task budget from the subcontractor as at the moment of any valid setComplete() call the task budget belongs to subcontractor as the job completion is already verified by all the parties.

Proof of Concept

inviteSC() requires either builder or contractor to call for the change and verify nothing else:

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

    /// @inheritdoc IProject
    function inviteSC(uint256[] calldata _taskList, address[] calldata _scList)
        external
        override
    {
        // Revert if sender is neither builder nor contractor.
        require(
            _msgSender() == builder || _msgSender() == contractor,
            "Project::!Builder||!GC"
        );

        // Revert if taskList array length not equal to scList array length.
        uint256 _length = _taskList.length;
        require(_length == _scList.length, "Project::Lengths !match");

        // Invite subcontractor for each task.
        for (uint256 i = 0; i < _length; i++) {
            _inviteSC(_taskList[i], _scList[i], false);
        }

        emit MultipleSCInvited(_taskList, _scList);
    }

_inviteSC() only checks non-zero address and calls inviteSubcontractor():

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

    function _inviteSC(
        uint256 _taskID,
        address _sc,
        bool _emitEvent
    ) internal {
        // Revert if sc to invite is address 0
        require(_sc != address(0), "Project::0 address");

        // Internal call to tasks invite contractor
        tasks[_taskID].inviteSubcontractor(_sc);

        // If `_emitEvent` is true (called via changeOrder) then emit event
        if (_emitEvent) {
            emit SingleSCInvited(_taskID, _sc);
        }
    }

inviteSubcontractor() just sets the new value:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L106-L111

    function inviteSubcontractor(Task storage _self, address _sc)
        internal
        onlyInactive(_self)
    {
        _self.subcontractor = _sc;
    }

Task is paid only on completion by setComplete():

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

        // Mark task as complete. Only works when task is active.
        tasks[_taskID].setComplete();

        // Transfer funds to subcontractor.
        currency.safeTransfer(
            tasks[_taskID].subcontractor,
            tasks[_taskID].cost
        );

This way the absence of getAlerts(_taskID)[2] check and checkSignatureTask() call in inviteSC() provides a way for builder or contractor to steal task budget from a subcontractor.

Recommended Mitigation Steps

Consider calling checkSignatureTask() when getAlerts(_taskID)[2] is true, schematically:

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

        // Invite subcontractor for each task.
        for (uint256 i = 0; i < _length; i++) {
+           if (getAlerts(_taskList[i])[2])
+               checkSignatureTask(_data_with_scList[i], _signature, _taskList[i]);        
            _inviteSC(_taskList[i], _scList[i], false);
        }

This approach is already implemented in changeOrder() where _newSC is a part of hash that has to be signed by all the parties:

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

    function changeOrder(bytes calldata _data, bytes calldata _signature)
        external
        override
        nonReentrant
    {
        // Decode params from _data
        (
            uint256 _taskID,
            address _newSC,
            uint256 _newCost,
            address _project
        ) = abi.decode(_data, (uint256, address, uint256, address));

        // If the sender is disputes contract, then do not check for signatures.
        if (_msgSender() != disputes) {
            // Check for required signatures.
            checkSignatureTask(_data, _signature, _taskID);
        }

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

            // If new subcontractor is not zero address.
            if (_newSC != address(0)) {
                // Invite the new subcontractor for the task.
                _inviteSC(_taskID, _newSC, true);
            }

checkSignatureTask() checks all the signatures:

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

            // When builder has not delegated rights to contractor
            else {
                // Check for B, SC and GC signatures
                checkSignatureValidity(builder, _hash, _signature, 0);
                checkSignatureValidity(contractor, _hash, _signature, 1);
                checkSignatureValidity(_sc, _hash, _signature, 2);
            }

[zgorizzo69]

When a SC accepts an invitation the task is marked as active https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L128
So as you noted here above the inviteSubcontractor for the same task will fail because of the modifier.

    function inviteSubcontractor(Task storage _self, address _sc)
        internal
        onlyInactive(_self)
    {

[dmitriia]

Yes, you are right, in general case onlyInactive modifier guards the reset.
The issue appears to be more specific, in the case when task budget is increased, while there is no budget to cover it, i.e. totalLent - _totalAllocated < _newCost - _taskCost, the subcontractor signs only the budget increase itself, while subcontractor ends up being unassigned from it fully:

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

            // If tasks are already allocated with old cost.
            if (tasks[_taskID].alerts[1]) {
                // If new task cost is less than old task cost.
                if (_newCost < _taskCost) {
                    // Find the difference between old - new.
                    uint256 _withdrawDifference = _taskCost - _newCost;

                    // Reduce this difference from total cost allocated.
                    // As the same task is now allocated with lesser cost.
                    totalAllocated -= _withdrawDifference;

                    // Withdraw the difference back to builder's account.
                    // As this additional amount may not be required by the project.
                    autoWithdraw(_withdrawDifference);
                }
                // If new cost is more than task cost but total lent is enough to cover for it.
                else if (totalLent - _totalAllocated >= _newCost - _taskCost) {
                    // Increase the difference of new cost and old cost to total allocated.
                    totalAllocated += _newCost - _taskCost;
                }
                // If new cost is more than task cost and totalLent is not enough.
                else {
                    // Un-confirm SC, mark task as inactive, mark allocated as false, mark lifecycle as None

                    // Mark task as inactive by unapproving subcontractor.
                    // As subcontractor can only be approved if task is allocated
                    _unapproved = true;
                    tasks[_taskID].unApprove();

                    // Mark task as not allocated.
                    tasks[_taskID].unAllocateFunds();

                    // Reduce total allocation by old task cost.
                    // As as needs to go though funding process again.
                    totalAllocated -= _taskCost;

                    // Add this task to _changeOrderedTask array. These tasks will be allocated first.
                    _changeOrderedTask.push(_taskID);
                }
            }

Suppose task is 95% complete, its budget is fully spent, so changeOrder() is called per mutual agreement to add extra 0.05 * old_cost / 0.95 funds, which aren't lent yet. Dishonest contractor can call inviteSC with own subcontractor, who will receive full old_cost / 0.95 on completion.

I.e. fully removing subcontractor from already funded and started task provides a more specific similar attack surface.

By definition unApprove deals with the case of new task that needs to be reviewed:

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

    /**
     * @dev Set a task as un accepted/approved for SC

     * @dev modifier onlyActive

     * @param _self Task the task being set as funded
     */
    function unApprove(Task storage _self) internal {
        // State/ lifecycle //
        _self.alerts[uint256(Lifecycle.SCConfirmed)] = false;
        _self.state = TaskStatus.Inactive;
    }

But in changeOrder() all the parties already reviewed and accepted the terms:

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

        // Decode params from _data
        (
            uint256 _taskID,
            address _newSC,
            uint256 _newCost,
            address _project
        ) = abi.decode(_data, (uint256, address, uint256, address));

        // If the sender is disputes contract, then do not check for signatures.
        if (_msgSender() != disputes) {
            // Check for required signatures.
            checkSignatureTask(_data, _signature, _taskID);
        }

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

            // When builder has not delegated rights to contractor
            else {
                // Check for B, SC and GC signatures
                checkSignatureValidity(builder, _hash, _signature, 0);
                checkSignatureValidity(contractor, _hash, _signature, 1);
                checkSignatureValidity(_sc, _hash, _signature, 2);
            }

So, marking the task as not active and not SCConfirmed doesn't look correct in this case.

Straightforward mitigation here is to keep it active, i.e. do partial flag removal, say do unConfirm instead of unApprove:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L160-L164

    function unConfirm(Task storage _self) internal {
        // State/ lifecycle //
        _self.alerts[uint256(Lifecycle.SCConfirmed)] = false;
    }

[dmitriia]

A little bit more complex, but more correct (project logic aligned) mitigation is:

1. Introduce deActivate instead of unConfirm:

    function deActivate(Task storage _self) internal {
        // State/ lifecycle //
        _self.state = TaskStatus.Inactive;
    }

2. Introduce onlyUnconfirmed modifier and set it to the inviteSubcontractor() and acceptInvitation():

    /// @dev only allow unconfirmed tasks.
    modifier onlyUnconfirmed(Task storage _self) {
        require(
            !_self.alerts[uint256(Lifecycle.SCConfirmed)],
            "Task::SCConfirmed"
        );
        _;
    }

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L106-L111

    function inviteSubcontractor(Task storage _self, address _sc)
        internal
-       onlyInactive(_self)
+       onlyUnconfirmed(_self)
    {
        _self.subcontractor = _sc;
    }

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L119-L129

    function acceptInvitation(Task storage _self, address _sc)
        internal
-       onlyInactive(_self)
+       onlyUnconfirmed(_self)
    {
        // Prerequisites //
        require(_self.subcontractor == _sc, "Task::!SC");

        // State/ lifecycle //
        _self.alerts[uint256(Lifecycle.SCConfirmed)] = true;
        _self.state = TaskStatus.Active;
    }

onlyInactive can then be removed:

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L42-L46

    /// @dev only allow inactive tasks. Task is inactive if SC is unconfirmed.
    modifier onlyInactive(Task storage _self) {
        require(_self.state == TaskStatus.Inactive, "Task::active");
        _;
    }

3. Deactivate task only instead of fully resetting it in changeOrder():

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

                else {
-                  // Un-confirm SC, mark task as inactive, mark allocated as false, mark lifecycle as None

-                  // Mark task as inactive by unapproving subcontractor.
-                  // As subcontractor can only be approved if task is allocated
-                  _unapproved = true;
-                  tasks[_taskID].unApprove();

+                  // Mark task as inactive, mark allocated as false, add to the allocation queue

+                  // Mark task as inactive
+                  tasks[_taskID].deActivate();

                    // Mark task as not allocated.
                    tasks[_taskID].unAllocateFunds();

                    // Reduce total allocation by old task cost.
                    // As as needs to go though funding process again.
                    totalAllocated -= _taskCost;

                    // Add this task to _changeOrderedTask array. These tasks will be allocated first.
                    _changeOrderedTask.push(_taskID);
                }

Notice that the mitigation here is to make Active and SCConfirmed states independent (as a general note, it doesn't make much sense to have some fully coinciding states). Active flags whether task is in progress right now, while SCConfirmed flags whether it ever was started, being either Active (work is being done right now) or Inactive (work had started, something was done, now it's paused).

The issue basically means that this states are different and moving a task to another SC while it's SCConfirmed should be prohibited as some work was done and some payment to current SC is due

[parv3213]

Agree to the risk, but the severity should be 2.

[jack-the-pug]

This is a good one with a very detailed explanation, but I'm afraid it fits a Medium severity better, as funds are not directly at risk but rather a malfunctioning feature that can indirectly cause damage to certain roles.