Omniscia Nevermined Audit
EscrowPaymentCondition Manual Review Findings
EscrowPaymentCondition Manual Review Findings
EPC-01M: Hash Collision
Type | Severity | Location |
---|---|---|
Logical Fault | Major | EscrowPaymentCondition.sol:L82-L109, L147 |
Description:
The hashValues
function and fulfill
statement can produce the same hash for two different sets of inputs.
Example:
82function hashValues(83 bytes32 _did,84 uint256[] memory _amounts,85 address[] memory _receivers,86 address _lockPaymentAddress,87 address _tokenAddress,88 bytes32 _lockCondition,89 bytes32 _releaseCondition90)91public pure92returns (bytes32)93{94 require(95 _amounts.length == _receivers.length,96 'Amounts and Receivers arguments have wrong length'97 );98 return keccak256(99 abi.encodePacked(100 _did,101 _amounts,102 _receivers,103 _lockPaymentAddress, 104 _tokenAddress,105 _lockCondition,106 _releaseCondition107 )108 );109}
Recommendation:
We advise the abi.encodePacked
invocations to be replaced by abi.encode
invocations 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
.
EPC-02M: Potential Token Incompatibility
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | EscrowPaymentCondition.sol:L231-L234 |
Description:
The _transferAndFulfillERC20
function requires that the transfer
function yields true
which may not be the case if a non-compliant ERC20 token is utilized, such as Tether (USDT).
Example:
214function _transferAndFulfillERC20(215 bytes32 _id,216 address _tokenAddress,217 address[] memory _receivers,218 uint256[] memory _amounts219)220private221returns (ConditionStoreLibrary.ConditionState)222{223 224 IERC20Upgradeable token = ERC20Upgradeable(_tokenAddress);225 226 for(uint i = 0; i < _receivers.length; i++) {227 require(228 _receivers[i] != address(this),229 'Escrow contract can not be a receiver'230 );231 require(232 token.transfer(_receivers[i], _amounts[i]),233 'Could not transfer token'234 );235 }236
237 return super.fulfill(238 _id,239 ConditionStoreLibrary.ConditionState.Fulfilled240 );241}
Recommendation:
We advise the SafeERC20
library of OpenZeppelin to be utilized instead that opportunistically evaluates the return value if it exists.
Alleviation:
The _transferAndFulfillERC20
function was upgraded to no longer yield a bool
and instead internally utilize safeTransfer
properly accounting for all types of ERC20 tokens.