Rigor Protocol contest - Guardian's results

1. In the contest docs, it is specifically mentioned that: “Note that you cannot submit a project with no total budget. Therefore it requires at least one task with a budget > 0.”

However, it is indeed possible to submit a project with a total budget of 0. Observe the following passing test cases:

projectTests.ts

it('is able to add task with 0 budget', async () => {
    const hashArray = ['0x'];
    const costArray = [0]; // 0 total cost
    taskList.push(0);
    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);
  });

communityTests.ts (edit createTasks in projectHelpers.ts so costArray is [0,0,0])

it('should publish project with 0 budget', async () => {
      apr = 10;
      const communityId = 1;

      expect(await project.projectCost()).to.eq(0); //indeed 0 budget

      const data = {
        types: ['uint256', 'address', 'uint256', 'uint256', 'uint256', 'bytes'],
        values: [
          communityId,
          projectAddress,
          apr,
          publishFeeAmount,
          (await communityContract.communities(communityId)).publishNonce,
          sampleHash,
        ],
      };

      const [encodedData, signature] = await multisig(data, [
        community1Owner,
        signers[0],
      ]);

      const tx = await communityContract.publishProject(encodedData, signature);

      await expect(tx)
        .to.emit(communityContract, 'ProjectPublished')
        .withArgs(
          communityId,
          projectAddress,
          apr,

          publishFeeAmount,
          publishFeeAmount.gt(0) ? false : true,
          sampleHash,
        );

      const [
        projectApr,
        projectLendingNeeded,
        projectTotalLent,
        publishFee,
        publishFeePaid,
      ] = await communityContract.projectDetails(communityId, projectAddress);

      expect(projectApr).to.be.equal(apr);
      expect(projectLendingNeeded).to.be.equal(0);
      expect(projectTotalLent).to.be.equal(0);
      expect(publishFee).to.be.equal(publishFeeAmount);
      expect(publishFeePaid).to.be.equal(false);

      const { publishNonce } = await communityContract.communities(communityId);
      expect(publishNonce).to.equal(1);
    });

2. It is possible for a builder to maliciously siphon funds for a task using changeOrder to lower the cost of a task after it has been allocated without any sort of consent from the community (only contractors).

Perhaps include the relevant community/community owner as a signer for such a change.

3. Community.sol::849, The comment indicates that wrapped tokens are burned when they are in fact minted.

4. Myriad internal/private functions lack a prepended _:

Community.sol

• checkSignatureValidity
• claimInterest

Disputes.sol

• resolveHandler
• executeTaskAdd
• executeTaskChange
• executeTaskPay

HomeFi.sol

• mintNFT

Project.sol

• autoWithdraw
• checkSignature
• checkSignatureTask
• checkSignatureValidity
• checkPrecision

4. HomeFi.sol::200, there is no corresponding event emitted when updating the trustedForwarder in the setTrustedForwarder function.

5. Tasks.sol::146,158, These comments do not match what the corresponding functions do. These comments are copies of line 136 “Task the task being set as funded” on function fundTask.

1. HomeFi.sol never utilizes the onlyInitializing modifier from the initializable.sol openzeppelin contract. Therefore the initializer modifier is gas inefficient, as it unnecessarily performs logic for _initializing.

Potentially inherit from another more gas-efficient initializable implementation or implement a simpler initializable functionality that avoids such gas inefficiencies.

2. Variables should not be initialized to defaults (uint256 default is 0):

  contracts/Community.sol::624 => for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
  contracts/HomeFiProxy.sol::87 => for (uint256 i = 0; i < _length; i++) {
  contracts/HomeFiProxy.sol::136 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::248 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::311 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::322 => for (uint256 i = 0; i < _length; i++) {
  contracts/libraries/Tasks.sol::181 => for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

3. Length of array should be computed outside of for-loop:

  contracts/Community.sol::618 => // Initiate empty equal equal to member count length
  contracts/HomeFiProxy.sol::78 => uint256 _length = allContractNames.length;
  contracts/HomeFiProxy.sol::80 => // Revert if _implementations length is wrong. Indicating wrong set of _implementations.
  contracts/HomeFiProxy.sol::81 => require(_length == _implementations.length, "Proxy::Lengths !match");
  contracts/HomeFiProxy.sol::87 => for (uint256 i = 0; i < _length; i++) {
  contracts/HomeFiProxy.sol::130 => uint256 _length = _contractNames.length;
  contracts/HomeFiProxy.sol::132 => // Revert if _contractNames and _contractAddresses length mismatch
  contracts/HomeFiProxy.sol::133 => require(_length == _contractAddresses.length, "Proxy::Lengths !match");
  contracts/HomeFiProxy.sol::136 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::243 => // Revert if IPFS hash array length is not equal to task cost array length.
  contracts/Project.sol::244 => uint256 _length = _hash.length;
  contracts/Project.sol::245 => require(_length == _taskCosts.length, "Project::Lengths !match");
  contracts/Project.sol::248 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::306 => // Revert if taskList array length not equal to scList array length.
  contracts/Project.sol::307 => uint256 _length = _taskList.length;
  contracts/Project.sol::308 => require(_length == _scList.length, "Project::Lengths !match");
  contracts/Project.sol::311 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::321 => uint256 _length = _taskList.length;
  contracts/Project.sol::322 => for (uint256 i = 0; i < _length; i++) {
  contracts/Project.sol::367 => uint256 _length = taskCount;
  contracts/Project.sol::368 => for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
  contracts/Project.sol::592 => taskCount - j + _changeOrderedTask.length - i
  contracts/Project.sol::601 => if (_changeOrderedTask.length > 0) {
  contracts/Project.sol::602 => // Loop from lastAllocatedChangeOrderTask to _changeOrderedTask length (until _maxLoop)
  contracts/Project.sol::603 => for (; i < _changeOrderedTask.length; i++) {
  contracts/Project.sol::635 => if (i == _changeOrderedTask.length) {
  contracts/Project.sol::707 => uint256 _length = taskCount;
  contracts/Project.sol::710 => for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
  contracts/libraries/SignatureDecoder.sol::25 => if (messageSignatures.length % 65 != 0) {
  contracts/libraries/Tasks.sol::180 => uint256 _length = _alerts.length;
  contracts/libraries/Tasks.sol::181 => for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

4. != is more efficient than > 0 for uint comparisons:

  contracts/Community.sol::261 => if (projectPublished[_project] > 0) {
  contracts/Community.sol::425 => // First claim interest if principal lent > 0
  contracts/Community.sol::427 => _communities[_communityID].projectDetails[_project].lentAmount > 0
  contracts/Community.sol::764 => require(_repayAmount > 0, "Community::!repay");
  contracts/Community.sol::840 => if (_interestEarned > 0) {
  contracts/Disputes.sol::107 => _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
  contracts/HomeFi.sol::245 => return projectTokenId[_project] > 0;
  contracts/Project.sol::195 => require(_cost > 0, "Project::!value>0");
  contracts/Project.sol::380 => if (_leftOutTokens > 0) {
  contracts/Project.sol::601 => if (_changeOrderedTask.length > 0) {
  contracts/Project.sol::691 => if (_loopCount > 0) emit TaskAllocated(_tasksAllocated);

5. Switching from division/multiplication to right-shift/left-shift can save gas:

  contracts/Community.sol::686 => _communityProject.lastTimestamp) / 86400; // 24*60*60