Human Protocol

CertiK21 Oct 2023Files named in the report

Audit

CertiK reviewed HUMAN Protocol on 21 Oct 2023: 20 findings, 0 critical; 16 marked fixed, 3 acknowledged, 1 open.

No open critical findings

Open the report Open HUMAN Protocol’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings20

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	3	1
Medium	0	0	1
Low	1	0	14

HighPotential Damage Due to Unprotected InitializerFixed

The logic of `EscrowFactory` does not protect its initializer. An attacker can call the initializer and assume ownership of the logic contract, whereby she can perform privileged operations that either indirectly break the proxy by destroying the logic contract or trick unsuspecting users into believing that she is the owner of the upgradeable contract.

MediumCheck Effect Interaction Pattern ViolatedFixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects.

Low`bulkPayOut()` Can Still Be Called When Status IsFixed

Technically it is still possible to call `bulkPayOut()` even if the current status is `Complete` or `Cancelled` because the function does not prevent the execution with these statuses. \ This could lead to potential errors from a user.

LowFunction 'transferFrom' of Contract HMToken DoesFixed

It is expected that calls of the form `transferFrom(from, dest, amount)` fail if the address value provided in the `from` in-parameter is the zero address.

LowImproper Usage of `public` and `external` TypeFixed

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

LowInconsistency Between Comment and CodeFixed

In the contract `Escrow.sol` the function `bulkPayOut()` perform a check on two input parameters as follow: ```solidity=159 bool writeOnchain = bytes(_hash).length != 0 || bytes(_url).length != 0; if (writeOnchain) { // Be sure they are both zero if one of them is finalResultsUrl = _url; finalResultsHash = _hash; } ``` However, the comment seems inconsistent with this check, indeed the conditional statement will not execute the block of code only if at most one of the two parameters is equal to zero. Then, if only one parameter is equal to zero, nothing in the logic of the function ensures that: ```solidity // Be sure they are both zero if one of them is ```

LowLogic Issue in Function `increaseApproval()`Fixed

In the function `increaseApproval()`, the following conditionnal statement: ```solidity=81 if (_oldValue.add(_delta) < _oldValue || _oldValue.add(_delta) >= MAX_UINT256) { // Truncate upon overflow. allowed[msg.sender][_spender] = MAX_UINT256.sub(1); ``` is equivalent to: ```solidity if (_oldValue.add(_delta) = MAX_UINT256) { allowed[msg.sender][_spender] = MAX_UINT256.sub(1); ``` because nothing is greater than `MAX_UINT256` and `add()` from `SafeMath` will revert if `_oldValue + _delta < _oldValue`.

LowMisleading Function NameFixed

The function `approveBulk()` does not allow a user to approve certain addresses to transfer certain values, contrary to what the name suggests, but it allows the user to increase the values already approved for this address. In particular, a user could believe he/she can reduce the allowances of a list of addresses, however, it is only possible to increase the allowances.

LowMissing Emit EventsFixed

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

LowMissing Zero Address ValidationFixed

Addresses should be checked before assignment or external calls to make sure they are not zero addresses.

Show 6 more

LowPotential Logical Issue in `bulkPayOut()`Fixed

In the function `bulkPayOut()`, in the following piece of code: ```solidity=157 for (uint256 i; i < _amounts.length; i++) { aggregatedBulkAmount += _amounts[i]; } require(aggregatedBulkAmount < BULK_MAX_VALUE, "Bulk value too high"); ``` it is possible to bypass the check if the sum of the `_amounts[i]` is greater than `max(uint256)`.

LowRedundant CheckFixed

In the function `setup()`, in the following piece of code: ```solidity=85 uint256 totalStake = _reputationOracleStake.add(_recordingOracleStake); require( totalStake >= 0 && totalStake <= 100, "Stake out of bounds" ); ``` `totalStake` is an `uint256`, hence it cannot be less than zero. ----- In the function `complete()`, in the following check: ```solidity=123 require( msg.sender == reputationOracle || areTrustedHandlers[msg.sender], "Address calling is not trusted" ); ``` the `reputationOracle` is always a trusted handler, hence requiring `areTrustedHandlers[msg.sender]` is enough.

LowToo Many DigitsFixed

Literals with many digits are difficult to read and review.

LowUnclear Logic of `bulkPayOut()`Fixed

In the `Escrow.sol` contract, the `bulkPayOut()` function from the new commit: [afedc405f537b2449bb9ec18b9815664431f2bcc](https://github.com/humanprotocol/human-protocol/tree/afedc405f537b2449bb9ec18b9815664431f2bcc/packages/core/contracts) behaves differently than before. Previously the function was not expected to fail if one of the transfers did not work, this was made using the `transferQuiet()` function, now all transfers are made using `_safeTransfer()` meaning all transfers much succeed. \ This change seems to have also interfered with the `bulkPaid` logic since now: ```solidity=257 bulkPaid = true; balance = getBalance(); if (bulkPaid) { ``` the *if* block will always be executed. Also, a new state variable has been introduced, `remainingSolutions`, which is impacting the potential state transition occurring at the end of `bulkPayOut()`.

LowUnnecessary Use of `SafeMath` FunctionsFixed

In the linked code, arithmetical operations are performed after a conditional statement has occurred preventing any risk of integer underflow/overflow.

LowUnoptimized CodeFixed

In the function `transferQuiet()`, the following block: ```solidity=150 if (_to == address(0)) return false; // Preclude burning tokens to uninitialized address. if (_to == address(this)) return false; // Preclude sending tokens to the contract. if (balances[msg.sender] < _value) return false; ``` is equivalent to : ```solidity if ((_to == address(0)) || (_to == address(this)) || (balances[msg.sender] < _value)) return false; ```