QA Report

[code423n4]

Summary

Low Risk Issues

	Issue	Instances
1	Missing modifier specified in comment	1
2	SignatureDecoder.recoverKey and ecrecover does not check for zero address	1
3	Incomplite action type control	1

Non-critical Issues

	Issue	Instances
1	Use struct in mapping instead of casting struct to uint256	1
2	Remove tautological code	1
3	Use built-in constants instead of a number	1
4	Misleading comment	1
5	redundant cast uint256 to uint256	1
6	Missing initializer modifier on constructor	6

Total: instances over issues

---

Low Risk Issues

---

1. Missing modifier specified in comment

   The function does not check the status of the task. You can set an inactive status and unapprove a subcontractor for a task that is already inactive or even completed.

   • 2022-08-rigor/contracts/libraries/Tasks.sol:156-160

   156      * @dev modifier onlyActive
   ...
   160      function unApprove(Task storage _self) internal {

   fix:

   function unApprove(Task storage _self) internal onlyActive(_self) {

2. SignatureDecoder.recoverKey and ecrecover does not check for zero address
   The solidity ecrecover function is called directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks. A replay attack may not be possible here, but ensuring the signatures are not malleable is considered a best practice.

   • 2022-08-rigor/contracts/Disputes.sol:90-94,116

   90      address _signer = SignatureDecoder.recoverKey(
   91          keccak256(_data),
   92          _signature,
   93          0
   94      );
   ...
   116    _dispute.raisedBy = _signer;

   fix:

   if (_signer==0) revert...;

3. Incomplite action type control.

   resolveHandler calls executeTaskPay if the action type is neither ActionType.TaskAdd nor ActionType.TaskChange. But apart from ActionType.TaskPay there is a possibility that the action type can be set to ActionType.None.

   At least it is more readable. But it can be used(theoretically) to call executeTaskPay, which will release subcontractor payment.

   • 2022-08-rigor/contracts/Disputes.sol:200-223

   200  /**
   201  * @notice Given an id, attempt to execute the action to enforce the arbitration
   202
   203  * @notice logic for decoding and enforcing outcome of arbitration judgement
   204
   205  * @param _disputeID uint256 - the dispute to attempt to
   206  */
   207 function resolveHandler(uint256 _disputeID) internal {
   208     // Local instance of variable. For saving gas.
   209     Dispute storage dispute = disputes[_disputeID];
   210
   211     // If action type is add task then execute add task
   212     if (dispute.actionType == ActionType.TaskAdd) {
   213         executeTaskAdd(dispute.project, dispute.actionData);
   214     }
   215     // If action type is task change then execute task change
   216     else if (dispute.actionType == ActionType.TaskChange) {
   217         executeTaskChange(dispute.project, dispute.actionData);
   218     }
   219     // Else execute task pay
   220     else {
   221         executeTaskPay(dispute.project, dispute.actionData);
   222     }
   223 }

   fix:

   220     else if (dispute.actionType == ActionType.TaskPay) {
   221         executeTaskPay(dispute.project, dispute.actionData);
   222     } else {revert InvalidTask();}

---

Non-critical Issues

---

1. Use struct in mapping instead of casting struct to uint256

   Optional fix, looks more informative, but slightly increases deployment gas due to enum import in the contract.

   • 2022-08-rigor/contracts/libraries/Tasks.sol:16

   16       mapping(uint256 => bool) alerts; // Alerts of task //@audit bool usage //@audit use struct instead of casting to uint256
   ...
   57       _self.alerts[uint256(Lifecycle.TaskAllocated)],

   fix:

        mapping(Lifecycle => bool) alerts;

2. Remove tautological code.

   • 2022-08-rigor/contracts/Community.sol:266

   266     _community.publishNonce = ++_community.publishNonce;

   fix:

        ++_community.publishNonce;

3. Use built-in constants instead of a number

   • 2022-08-rigor/contracts/Community.sol:685-686

   685     uint256 _noOfDays = (block.timestamp -
   686         _communityProject.lastTimestamp) / 86400; // 24*60*60

   fix:

   685     uint256 _noOfDays = (block.timestamp -
   686         _communityProject.lastTimestamp) / 1 days; // 24*60*60

4. Misleading comment

   Comment probably copied from a nearby function and is misleading about the function's purpose.

   • 2022-08-rigor/contracts/Disputes.sol:226,236-240

   226     * @dev Arbitration enforcement of task change orders
           ...
   236     function executeTaskAdd(address _project, bytes memory _actionData)
   237         internal
   238     {
   239         IProject(_project).addTasks(_actionData, bytes(""));
   240     }

5. redundant cast uint256 to uint256.

   • 2022-08-rigor/contracts/Project.sol:198,200

   198     uint256 _newTotalLent = totalLent + _cost;
   ...
   200     projectCost() >= uint256(_newTotalLent),

6. Missing initializer modifier on constructor
   OpenZeppelin recommends that the initializer modifier be applied to constructors:
   The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.

   • 2022-08-rigor/contracts/Community.sol
   • 2022-08-rigor/contracts/DebtToken.sol
   • 2022-08-rigor/contracts/Disputes.sol
   • 2022-08-rigor/contracts/HomeFi.sol
   • 2022-08-rigor/contracts/HomeFiProxy.sol
   • 2022-08-rigor/contracts/ProjectFactory.sol

   fix: add constructor() initializer {}