Omniscia Nevermined Audit

EscrowPaymentCondition Manual Review Findings

EscrowPaymentCondition Manual Review Findings

EPC-01M: Hash Collision

Description:

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

Example:

contracts/conditions/rewards/EscrowPaymentCondition.sol
82function hashValues(
83    bytes32 _did,
84    uint256[] memory _amounts,
85    address[] memory _receivers,
86    address _lockPaymentAddress,
87    address _tokenAddress,
88    bytes32 _lockCondition,
89    bytes32 _releaseCondition
90)
91public pure
92returns (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            _releaseCondition
107        )
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

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:

contracts/conditions/rewards/EscrowPaymentCondition.sol
214function _transferAndFulfillERC20(
215    bytes32 _id,
216    address _tokenAddress,
217    address[] memory _receivers,
218    uint256[] memory _amounts
219)
220private
221returns (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.Fulfilled
240    );
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.

Prev

DIDRegistryLibrary.sol (DIL-M)

Next

LockPaymentCondition.sol (LPC-M)

On this page