Omniscia Nevermined Audit
LockPaymentCondition Static Analysis Findings
LockPaymentCondition Static Analysis Findings
LPC-01S: Arbitrary Re-Entrancy
Type | Severity | Location |
---|---|---|
Language Specific | Minor | LockPaymentCondition.sol:L108-L157 |
Description:
The fulfill
function performs no sanitization of the input _tokenAddress
which allows a user to re-enter the contract via a maliciously crafted transferFrom
function.
Example:
contracts/conditions/LockPaymentCondition.sol
108function fulfill(109 bytes32 _agreementId,110 bytes32 _did,111 address payable _rewardAddress,112 address _tokenAddress,113 uint256[] memory _amounts,114 address[] memory _receivers115)116external117payable118returns (ConditionStoreLibrary.ConditionState)119{120 require(121 _amounts.length == _receivers.length,122 'Amounts and Receivers arguments have wrong length'123 );124
125 require(126 didRegistry.areRoyaltiesValid(_did, _amounts, _receivers),127 'Royalties are not satisfied'128 );129
130 if (_tokenAddress != address(0))131 require(132 _transferERC20(_rewardAddress, _tokenAddress, calculateTotalAmount(_amounts)),133 'Could not transfer token'134 );135 else136 _transferETH(_rewardAddress, calculateTotalAmount(_amounts));137
138 bytes32 _id = generateId(139 _agreementId,140 hashValues(_did, _rewardAddress, _tokenAddress, _amounts, _receivers)141 );142 ConditionStoreLibrary.ConditionState state = super.fulfill(143 _id,144 ConditionStoreLibrary.ConditionState.Fulfilled145 );146
147 emit Fulfilled(148 _agreementId, 149 _did,150 _id,151 _rewardAddress,152 _tokenAddress,153 _receivers, 154 _amounts155 );156 return state;157}
Recommendation:
We advise some sanitization to be applied to the input _tokenAddress
or, if undesirable, to ensure the call is performed after all state changes thereby ensuring proper execution of state transitions.
Alleviation:
The nonReentrant
modifier was introduced from the OpenZeppelin
library thus preventing a re-entrancy from occuring at the function level. We should note that a re-entrancy would still be possible in other parts of the system that do not boast this modifier, however, we assume that this has been considered by the Nevermined team and as such consider this exhibit dealt with.