Resolve Certik v3 Reports (#437)

* fix unnecessary math utils, and add comments to bulkPayout function * update doc and add missing require for amount in bulkpayout * use modifier * fix test
humanprotocol · May 5, 2023 · 3cf4ee8 · 3cf4ee8
1 parent 8fffddf
commit 3cf4ee8
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 21 deletions.
diff --git a/packages/core/contracts/Escrow.sol b/packages/core/contracts/Escrow.sol
@@ -82,11 +82,9 @@ contract Escrow is IEscrow, ReentrancyGuard {
         return 0;
     }
 
-    function addTrustedHandlers(address[] memory _handlers) public override {
-        require(
-            areTrustedHandlers[msg.sender],
-            'Address calling cannot add trusted handlers'
-        );
+    function addTrustedHandlers(
+        address[] memory _handlers
+    ) public override trusted {
         for (uint256 i = 0; i < _handlers.length; i++) {
             require(_handlers[i] != address(0), ERROR_ZERO_ADDRESS);
             areTrustedHandlers[_handlers[i]] = true;
@@ -182,8 +180,21 @@ contract Escrow is IEscrow, ReentrancyGuard {
     }
 
     /**
-     * @dev Bulk payout workers
-     * Should fail if any of the transaction is failing.
+     * @dev Performs bulk payout to multiple workers
+     * Escrow needs to be complted / cancelled, so that it can be paid out.
+     * Every recipient is paid with the amount after reputation and recording oracle fees taken out.
+     * If the amount is less than the fee, the recipient is not paid.
+     * If the fee is zero, reputation, and recording oracle are not paid.
+     * Payout will fail if any of the transaction fails.
+     * If the escrow is fully paid out, meaning that the balance of the escrow is 0, it'll set as Paid.
+     * If the escrow is partially paid out, meaning that the escrow still has remaining balance, it'll set as Partial.
+     * This contract is only callable if the contract is not broke, not launched, not paid, not expired, by trusted parties.
+     *
+     * @param _recipients Array of recipients
+     * @param _amounts Array of amounts to be paid to each recipient.
+     * @param _url URL storing results as transaction details
+     * @param _hash Hash of the results
+     * @param _txId Transaction ID
      */
     function bulkPayOut(
         address[] memory _recipients,
@@ -215,6 +226,7 @@ contract Escrow is IEscrow, ReentrancyGuard {
         uint256 balance = getBalance();
         uint256 aggregatedBulkAmount = 0;
         for (uint256 i; i < _amounts.length; i++) {
+            require(_amounts[i] > 0, 'Amount should be greater than zero');
             aggregatedBulkAmount = aggregatedBulkAmount.add(_amounts[i]);
         }
         require(aggregatedBulkAmount < BULK_MAX_VALUE, 'Bulk value too high');
@@ -229,11 +241,17 @@ contract Escrow is IEscrow, ReentrancyGuard {
         ) = finalizePayouts(_amounts);
 
         for (uint256 i = 0; i < _recipients.length; ++i) {
-            _safeTransfer(_recipients[i], finalAmounts[i]);
+            if (finalAmounts[i] > 0) {
+                _safeTransfer(_recipients[i], finalAmounts[i]);
+            }
         }
 
-        _safeTransfer(reputationOracle, reputationOracleFee);
-        _safeTransfer(recordingOracle, recordingOracleFee);
+        if (reputationOracleFee > 0) {
+            _safeTransfer(reputationOracle, reputationOracleFee);
+        }
+        if (recordingOracleFee > 0) {
+            _safeTransfer(recordingOracle, recordingOracleFee);
+        }
 
         balance = getBalance();
 

diff --git a/packages/core/contracts/HMToken.sol b/packages/core/contracts/HMToken.sol
@@ -82,9 +82,9 @@ contract HMToken is HMTokenInterface, Ownable {
 
         if (_allowance != MAX_UINT256) {
             // Special case to approve unlimited transfers
-            allowed[_spender][msg.sender] = allowed[_spender][msg.sender].sub(
-                _value
-            );
+            allowed[_spender][msg.sender] =
+                allowed[_spender][msg.sender] -
+                _value;
         }
 
         emit Transfer(_spender, _to, _value);
@@ -125,11 +125,11 @@ contract HMToken is HMTokenInterface, Ownable {
             _oldValue + _delta < _oldValue || _oldValue + _delta == MAX_UINT256
         ) {
             // Truncate upon overflow.
-            allowed[msg.sender][_spender] = MAX_UINT256.sub(1);
+            allowed[msg.sender][_spender] = MAX_UINT256 - 1;
         } else {
-            allowed[msg.sender][_spender] = allowed[msg.sender][_spender].add(
-                _delta
-            );
+            allowed[msg.sender][_spender] =
+                allowed[msg.sender][_spender] +
+                _delta;
         }
         emit Approval(msg.sender, _spender, allowed[msg.sender][_spender]);
         return true;
@@ -149,9 +149,9 @@ contract HMToken is HMTokenInterface, Ownable {
             // Truncate upon overflow.
             allowed[msg.sender][_spender] = 0;
         } else {
-            allowed[msg.sender][_spender] = allowed[msg.sender][_spender].sub(
-                _delta
-            );
+            allowed[msg.sender][_spender] =
+                allowed[msg.sender][_spender] -
+                _delta;
         }
         emit Approval(msg.sender, _spender, allowed[msg.sender][_spender]);
         return true;

diff --git a/packages/core/test/Escrow.ts b/packages/core/test/Escrow.ts
@@ -161,7 +161,7 @@ describe('Escrow', function () {
           escrow
             .connect(externalAddress)
             .addTrustedHandlers([await reputationOracle.getAddress()])
-        ).to.be.revertedWith('Address calling cannot add trusted handlers');
+        ).to.be.revertedWith('Address calling not trusted');
       });
     });