LockPaymentCondition Manual Review Findings

LPC-01M: Hash Collision

Description:

The hashValues function can produce the same hash for two different sets of inputs.

Example:

Copy
83function hashValues(
84    bytes32 _did,
85    address _rewardAddress,
86    address _tokenAddress,
87    uint256[] memory _amounts,
88    address[] memory _receivers
89)
90    public
91    pure
92    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

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:

Copy
183function _transferETH(
184    address payable _rewardAddress,
185    uint256 _amount
186)
187internal
188{
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

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:

Copy
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.