Omniscia Nevermined Audit
LockPaymentCondition Manual Review Findings
LockPaymentCondition Manual Review Findings
LPC-01M: Hash Collision
Type | Severity | Location |
---|---|---|
Logical Fault | Major | LockPaymentCondition.sol:L83-L95 |
Description:
The hashValues
function can produce the same hash for two different sets of inputs.
Example:
83function hashValues(84 bytes32 _did,85 address _rewardAddress,86 address _tokenAddress,87 uint256[] memory _amounts,88 address[] memory _receivers89)90 public91 pure92 returns (bytes32)93{94 return keccak256(abi.encodePacked(_did, _rewardAddress, _tokenAddress, _amounts, _receivers));95}
Recommendation:
We advise the abi.encodePacked
invocation to be replaced by an abi.encode
invocation as the current usage can cause a hash colission for multiple different inputs if _amounts
and _receivers
are sufficiently manipulated.
Alleviation:
The abi.encodePacked
invocation was properly replaced by abi.encode
in the linked segment as well as in other out-of-scope places of the codebase, indicating that the whole project was upgraded to utilize abi.encode
over abi.encodePacked
.
LPC-02M: Deprecated Ether Transfer
Type | Severity | Location |
---|---|---|
Language Specific | Medium | LockPaymentCondition.sol:L189 |
Description:
The transfer
method of the payable
address has been deprecated due to its hard-coded constant gas which can cause Ether transfers to fail either due to EIPs that increase gas costs, which occured with the recent Berlin fork, or by the recipients conducting some extra logic.
Example:
183function _transferETH(184 address payable _rewardAddress,185 uint256 _amount186)187internal188{189 _rewardAddress.transfer(_amount);190}
Recommendation:
We advise all or a portion of the available gas to be forwarded along with the transfer to ensure that it will be successful regardless of the evolution of Ethereum.
Alleviation:
The high-level transfer
of Ether was replaced by a low-level call{value}
invocation future-proofing the Ether transfer mechanism.
LPC-03M: Potential Token Incompatibility
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | LockPaymentCondition.sol:L131-L134, L175 |
Description:
The fulfill
function requires that the _transferERC20
function yields true
which may not be the case of its transferFrom
invocation if a non-compliant ERC20 token is utilized, such as Tether (USDT).
Example:
131require(132 _transferERC20(_rewardAddress, _tokenAddress, calculateTotalAmount(_amounts)),133 'Could not transfer token'134);
Recommendation:
We advise the SafeERC20
library of OpenZeppelin to be utilized instead that opportunistically evaluates the return value if it exists.
Alleviation:
The _transferERC20
function was upgraded to no longer yield a bool
and instead internally utilize safeTransferFrom
properly accounting for all types of ERC20 tokens.