Rigor Protocol contest - brgltd's results

[L-01] The function changeOrder doesn't follow the check-effects-interaction pattern

Impact

Wherever possible, the contract should follow the check-effects-interaction pattern.

On the changeOrder function, the state is modified tasks[_taskID].cost = _newCost after an external call is made autoWithdraw(_withdrawDifference).

File: contracts/Project.sol
435: autoWithdraw(_withdrawDifference);

464: tasks[_taskID].cost = _newCost;

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

If the external call throws, the state should be reverted. Also, since the value for tasks[_taskID].cost is already cached on the _taskCost variable, updating the state before the external call shouldn't impact the bussiness logic.

Recommended Mitigation Steps

Even if the function is already using the nonReetrant modifier, I would still recommend updating the state before making external calls.

$ git diff --no-index Project.sol.orig Project.sol
diff --git a/Project.sol.orig b/Project.sol
index 7e0ae45..2644d87 100644
--- a/Project.sol.orig
+++ b/Project.sol
@@ -411,6 +411,9 @@ contract Project is
         // Local variable indicating if subcontractor is already unapproved.
         bool _unapproved = false;

+        // Store new cost for the task
+        tasks[_taskID].cost = _newCost;
+
         // If task cost is to be changed.
         if (_newCost != _taskCost) {
             // Check new task cost precision. Revert if too precise.
@@ -460,9 +463,6 @@ contract Project is
                 }
             }

-            // Store new cost for the task
-            tasks[_taskID].cost = _newCost;
-
             emit ChangeOrderFee(_taskID, _newCost);
         }

[L-02] Lack of zero address check for critical input parameters

Critical inputs of type address should not be able to unintentionally receive the value zero.

Not checking agaist input address zero can cause misuse of tokens and force redeployments.

File: contracts/Project.sol
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);
}

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

[NC-01] Avoid typo

There's a typo on the following comment.

File: contracts/libraries/SignatureDecoder.sol
8: * @notice Decodes signatures that a encoded as bytes

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

[NC-02] Missing return for natspec

The function signatureSplit should contain a description of the returned rsv data. Consider adding the @return tag to ensure natspec best practices.

File: contracts/libraries/SignatureDecoder.sol
/**
* @dev Divides bytes signature into `uint8 v, bytes32 r, bytes32 s`.
* @dev Make sure to perform a bounds check for @param pos, to avoid out of bounds access on @param signatures

* @param pos which signature to read. A prior bounds check of this parameter should be performed, to avoid out of bounds access
* @param signatures concatenated rsv signatures
*/
function signatureSplit(bytes memory signatures, uint256 pos)
    internal
    pure
    returns (
        uint8 v,
        bytes32 r,
        bytes32 s
    )
{

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

[NC-03] Public functions not consumed by the contract should be declared external

File: contracts/Community.sol
709: function isTrustedForwarder(address _forwarder)

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

[NC-04] Use time units for calculations involving time

Instead of using the number of seconds, solidity allows the usage of time units such as days and weeks to improve redability.

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

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

[G-01] ++variable costs less gas than variable++, especially in for loops

Saves about 5 gas per loop.

File: contracts/Project.sol
248: for (uint256 i = 0; i < _length; i++) {

603: for (; i < _changeOrderedTask.length; i++) {

625: _loopCount++;

710: for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

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

[G-02] The increment in for loop post condition can be made unchecked

The solidity compiler will apply arithmetic checks for the increment step during for loops. This can be disabled since the value being incremented won't surpass the upper bound that's checked on the break condition.

Adding uncheck can save 30-40 gas per loop.

File: contracts/Project.sol
248: for (uint256 i = 0; i < _length; i++) {

603: for (; i < _changeOrderedTask.length; i++) {

710: for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {

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

[G-03] Array.length should not be computed on every interation during a loop

Instead of computing array.length for every iteration, the value for array.length should be cached before the loop to save gas.

File: contracts/Project.sol
603: for (; i < _changeOrderedTask.length; i++) {

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

[G-04] It costs more gas to initialize variables to zero than to let the default of zero be applied

File: contracts/Project.sol
248: for (uint256 i = 0; i < _length; i++) {

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

[G-05] != costs less gas than > in conditional contexts for uints

Saves about 6 gas.

File: contracts/Project.sol
195: require(_cost > 0, "Project::!value>0");

691: if (_loopCount > 0) emit TaskAllocated(_tasksAllocated);

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

[G-06] Use custom errors rather than revert()/require() to save gas

File: contracts/Project.sol
123: require(!contractorConfirmed, "Project::GC accepted");

132: require(_projectAddress == address(this), "Project::!projectAddress");

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

File: contracts/Community.sol
69: require(_address != address(0), "Community::0 address");

75: require(_msgSender() == homeFi.admin(), "Community::!admin");

81: require(

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

File: contracts/HomeFi.sol
142: require(!addrSet, "HomeFi::Set");

191: require(lenderFee != _newLenderFee, "HomeFi::!Change");

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