Rigor Protocol contest - CertoraInc's results

QA Report

• Error messages length Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc. An even better and gas efficient approach will be to use Solidity Custom Errors instead of revert strings.

HomeFi

• Use accept admin-ship to avoid transferring the admin role to a non-existing address In the current implementation, if the replaceAdmin function is called with an invalid address, the admin-ship will be transferred to this address and it can DoS the admin role. It will be better to make the new admin call a function before actually transferring the admin-ship, in order to validate that the new admin's address is valid and reachable.
• Missing a check that _forwarder != address(0) in the constructor
• Missing underscore in the beginning of the mintNFT internal function

Project

• Missing underscore in the beginning of the checkSignature, autoWithdraw internal functions
• Typo in line 214 (then instead of them) - // Allocate funds to tasks and mark then as allocated
• Magic numbers in lines 281, 369, 423 (use the enum values instead)

Gas Optimizations

HomeFi

• Prefix incrementation in the mintNFT function (change projectCount += 1 to ++projectCount)
• Use the calldata modifier on struct and array arguments instead of memory in the createProject and mintNFT functions
• Use return value of mintNFT instead of accessing the projectCount storage variable 3 times code before:

  function createProject(bytes memory _hash, address _currency)
      external
      override
      nonReentrant
  {
      // Revert if currency not supported by HomeFi
      validCurrency(_currency);
  
      address _sender = _msgSender();
  
      // Create a new project Clone and mint a new NFT for it
      address _project = projectFactoryInstance.createProject(
          _currency,
          _sender
      );
      mintNFT(_sender, string(_hash));
  
      // Update project related mappings
      projects[projectCount] = _project;
      projectTokenId[_project] = projectCount;
  
      emit ProjectAdded(projectCount, _project, _sender, _currency, _hash);
  }

  code after:

  function createProject(bytes memory _hash, address _currency)
      external
      override
      nonReentrant
  {
      // Revert if currency not supported by HomeFi
      validCurrency(_currency);
  
      address _sender = _msgSender();
  
      // Create a new project Clone and mint a new NFT for it
      address _project = projectFactoryInstance.createProject(
          _currency,
          _sender
      );
      uint tokenId = mintNFT(_sender, string(_hash));
  
      // Update project related mappings
      projects[tokenId] = _project;
      projectTokenId[_project] = tokenId;
  
      emit ProjectAdded(tokenId, _project, _sender, _currency, _hash);
  }

ProjectFactory

• Cache homeFi in the createProject function instead of accessing the same storage slot twice code before:

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

  code after:

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

Project

• Use _homeFiAddress in the initialize instead of accessing the storage twice to read homeFi (which contains the same value) code before:

  function initialize(
      address _currency,
      address _sender,
      address _homeFiAddress
  ) external override initializer {
      // Initialize variables
      homeFi = IHomeFi(_homeFiAddress);
      disputes = homeFi.disputesContract();
      lenderFee = homeFi.lenderFee();
      builder = _sender;
      currency = IDebtToken(_currency);
  }

  code after:

  function initialize(
      address _currency,
      address _sender,
      address _homeFiAddress
  ) external override initializer {
      // Initialize variables
      homeFi = IHomeFi(_homeFiAddress);
      disputes = IHomeFi(_homeFiAddress).disputesContract();
      lenderFee = IHomeFi(_homeFiAddress).lenderFee();
      builder = _sender;
      currency = IDebtToken(_currency);
  }

  another way is to change the _homeFiAddress argument to IHomeFi type

• Cache contractor in the checkSignature function code before:

  function checkSignature(bytes calldata _data, bytes calldata _signature)
      internal
  {
      // Calculate hash from bytes
      bytes32 _hash = keccak256(_data);
  
      // When there is no contractor
      if (contractor == address(0)) {
          // Check for builder's signature
          checkSignatureValidity(builder, _hash, _signature, 0);
      }
      // When there is a contractor
      else {
          // When builder has delegated his rights to contractor
          if (contractorDelegated) {
              //  Check contractor's signature
              checkSignatureValidity(contractor, _hash, _signature, 0);
          }
          // When builder has not delegated rights to contractor
          else {
              // Check for both B and GC signatures
              checkSignatureValidity(builder, _hash, _signature, 0);
              checkSignatureValidity(contractor, _hash, _signature, 1);
          }
      }
  }

  code after:

  function checkSignature(bytes calldata _data, bytes calldata _signature)
      internal
  {
      // Calculate hash from bytes
      bytes32 _hash = keccak256(_data);
  
      address _contractor = contractor;
  
      // When there is no contractor
      if (_contractor == address(0)) {
          // Check for builder's signature
          checkSignatureValidity(builder, _hash, _signature, 0);
      }
      // When there is a contractor
      else {
          // When builder has delegated his rights to contractor
          if (contractorDelegated) {
              //  Check contractor's signature
              checkSignatureValidity(_contractor, _hash, _signature, 0);
          }
          // When builder has not delegated rights to contractor
          else {
              // Check for both B and GC signatures
              checkSignatureValidity(builder, _hash, _signature, 0);
              checkSignatureValidity(_contractor, _hash, _signature, 1);
          }
      }
  }

• Cache hashChangeNonce in updateProjectHash code before:

  // Revert if decoded nonce is incorrect. This indicates wrong _data.
  require(_nonce == hashChangeNonce, "Project::!Nonce");
  
  // Increment to ensure a set of data and signature cannot be re-used.
  hashChangeNonce += 1;

  code after:

  // Revert if decoded nonce is incorrect. This indicates wrong _data.
  // Increment to ensure a set of data and signature cannot be re-used.
  require(_nonce == hashChangeNonce++, "Project::!Nonce");

• Cache the condition in the lendToProject function instead of calculating it twice

  address _sender = _msgSender();
  bool senderIsBuilder == _sender == builder;
  
  // Revert if sender is not builder or Community Contract (lender)
  require(
      senderIsBuilder || _sender == homeFi.communityContract(),
      "Project::!Builder&&!Community"
  );
  
  // Revert if try to lend 0
  require(_cost > 0, "Project::!value>0");
  
  // Revert if try to lend more than project cost
  uint256 _newTotalLent = totalLent + _cost;
  require(
      projectCost() >= uint256(_newTotalLent),
      "Project::value>required"
  );
  
  if (senderIsBuilder) {
      // Transfer assets from builder to this contract
      currency.safeTransferFrom(_sender, address(this), _cost);
  }
  
  // ...
  

• Optimizations to the allocateFunds function:

  1. cache totalLent and taskCount
  2. cache _changeOrderedTask[i]
  3. cache tasks[j]
  4. save the array's length instead of accessing it in every iteration (_changeOrderedTask.length)
  5. prefix incrementation of the loop indices The code will look like this after the changes:

  function allocateFunds() public override {
      // Max amount out times this loop will run
      // This is to ensure the transaction do not run out of gas (max gas limit)
      uint256 _maxLoop = 50;
  
      uint _taskCount = taskCount;
      uint _totalLent = totalLent;
  
      // Difference of totalLent and totalAllocated is what can be used to allocate new tasks
      uint256 _costToAllocate = _totalLent - totalAllocated;
  
      // Bool if max loop limit is exceeded
      bool _exceedLimit;
  
      // Local instance of lastAllocatedChangeOrderTask. To save gas.
      uint256 i = lastAllocatedChangeOrderTask;
  
      // Local instance of lastAllocatedTask. To save gas.
      uint256 j = lastAllocatedTask;
  
      uint length = _changeOrderedTask.length;
  
      // Initialize empty array in which allocated tasks will be added.
      uint256[] memory _tasksAllocated = new uint256[](
          _taskCount - j + length - i
      );
  
      // Number of times a loop has run.
      uint256 _loopCount;
  
      Task storage _task;
  
      /// CHANGE ORDERED TASK FUNDING ///
  
      // Any tasks added to _changeOrderedTask will be allocated first
      if (length > 0) {
          // Loop from lastAllocatedChangeOrderTask to _changeOrderedTask length (until _maxLoop)
          for (; i < length; ++i) {
              _changeOrderedTask_i = _changeOrderedTask[i];
              _task = tasks[_changeOrderedTask_i];
  
              // Local instance of task cost. To save gas.
              uint256 _taskCost = _task.cost;
  
              // If _maxLoop limit is reached then stop looping
              if (_loopCount >= _maxLoop) {
                  _exceedLimit = true;
                  break;
              }
  
              // If there is enough funds to allocate this task
              if (_costToAllocate >= _taskCost) {
                  // Reduce task cost from _costToAllocate
                  _costToAllocate -= _taskCost;
  
                  // Mark the task as allocated
                  _task.fundTask();
  
                  // Add task to _tasksAllocated array
                  _tasksAllocated[_loopCount] = _changeOrderedTask_i;
  
                  // Increment loop counter
                  ++_loopCount;
              }
              // If there are not enough funds to allocate this task then stop looping
              else {
                  break;
              }
          }
  
          // If all the change ordered tasks are allocated, then delete
          // the changeOrderedTask array and reset lastAllocatedChangeOrderTask.
          if (i == length) {
              lastAllocatedChangeOrderTask = 0;
              delete _changeOrderedTask;
          }
          // Else store the last allocated change order task index.
          else {
              lastAllocatedChangeOrderTask = i;
          }
      }
  
      /// TASK FUNDING ///
  
      // If lastAllocatedTask is lesser than taskCount, that means there are un-allocated tasks
      if (j < _taskCount) {
          // Loop from lastAllocatedTask + 1 to taskCount (until _maxLoop)
          for (++j; j <= _taskCount; j++) {
  
              _task = tasks[j];
  
              // Local instance of task cost. To save gas.
              uint256 _taskCost = _task.cost;
  
              // If _maxLoop limit is reached then stop looping
              if (_loopCount >= _maxLoop) {
                  _exceedLimit = true;
                  break;
              }
  
              // If there is enough funds to allocate this task
              if (_costToAllocate >= _taskCost) {
                  // Reduce task cost from _costToAllocate
                  _costToAllocate -= _taskCost;
  
                  // Mark the task as allocated
                  _task.fundTask();
  
                  // Add task to _tasksAllocated array
                  _tasksAllocated[_loopCount] = j;
  
                  // Increment loop counter
                  _loopCount++;
              }
              // If there are not enough funds to allocate this task then stop looping
              else {
                  break;
              }
          }
  
          // If all pending tasks are allocated store lastAllocatedTask equal to taskCount
          if (j > _taskCount) {
              lastAllocatedTask = _taskCount;
          }
          // If not all tasks are allocated store updated lastAllocatedTask
          else {
              lastAllocatedTask = --j;
          }
      }
  
      // If any tasks is allocated, then emit event
      if (_loopCount > 0) emit TaskAllocated(_tasksAllocated);
  
      // If allocation was incomplete, then emit event
      if (_exceedLimit) emit IncompleteAllocation();
  
      // Update totalAllocated with all allocations
      totalAllocated = _totalLent - _costToAllocate;
  }

• Emit the event in the calling function instead of sending it to _inviteSC, that will save the gas spent on the check of the sent boolean.

  The function will look like this now:

  function _inviteSC(
      uint256 _taskID,
      address _sc
  ) internal {
      // Revert if sc to invite is address 0
      require(_sc != address(0), "Project::0 address");
  
      // Internal call to tasks invite contractor
      tasks[_taskID].inviteSubcontractor(_sc);
  }

  The call from inviteSC will look like this:

  function inviteSC(uint256[] calldata _taskList, address[] calldata _scList)
      external
      override
  {
      // Revert if sender is neither builder nor contractor.
      require(
          _msgSender() == builder || _msgSender() == contractor,
          "Project::!Builder||!GC"
      );
  
      // Revert if taskList array length not equal to scList array length.
      uint256 _length = _taskList.length;
      require(_length == _scList.length, "Project::Lengths !match");
  
      // Invite subcontractor for each task.
      for (uint256 i = 0; i < _length; i++) {
          _inviteSC(_taskList[i], _scList[i]);
      }
  
      emit MultipleSCInvited(_taskList, _scList);
  }

  And the call from changeOrder will look like this:

  // If new subcontractor is not zero address.
  if (_newSC != address(0)) {
      // Invite the new subcontractor for the task.
      _inviteSC(_taskID, _newSC);
      emit SingleSCInvited(_taskID, _newSC);
  }

Community

• Prefix incrementation on line 140
• Cache communityCount

  function createCommunity(bytes calldata _hash, address _currency)
      external
      override
      whenNotPaused
  {
      // Local variable for sender. For gas saving.
      address _sender = _msgSender();
  
      // Revert if community creation is paused or sender is not HomeFi admin
      require(
          !restrictedToAdmin || _sender == homeFi.admin(),
          "Community::!admin"
      );
  
      // Revert if currency is not supported by HomeFi
      homeFi.validCurrency(_currency);
  
      uint _communityCount = communityCount + 1;
  
      // Increment community counter
      communityCount = _communityCount;
  
      // Store community details
      CommunityStruct storage _community = _communities[_communityCount];
      _community.owner = _sender;
      _community.currency = IDebtToken(_currency);
      _community.memberCount = 1;
      _community.members[0] = _sender;
      _community.isMember[_sender] = true;
  
      emit CommunityAdded(_communityCount, _sender, _currency, _hash);
  }

• Redundant assignment on line 266
• Cache _projectInstance.lenderFee() in lines 393-394 instead of making 2 external calls with the same result
• Cache _communities[_communityID].projectDetails[_project] in lines 402-407

Tasks

• Use a uint8 field member instead of uint => bool mapping (use the first 3 bits of a uint instead of creating a whole mapping)

// Task metadata
struct Task {
    // Metadata //
    uint256 cost; // Cost of task
    address subcontractor; // Subcontractor of task
    // Lifecycle //
    TaskStatus state; // Status of task
    uint8 alerts; // Alerts of task
}

function getAlert(Task storage task, TaskStatus state) returns (bool) {
    return task.alerts & (1 << uint8(state)) != 0;
}

function setAlert(Task storage task, TaskStatus state, bool value) {
    if (value) {
        task.alerts = task.alerts | (1 << uint8(state));
    } else {
        task.alerts = task.alerts & ~(1 << uint8(state));
    }
}