Omniscia Nevermined Audit

LockPaymentCondition Static Analysis Findings

LockPaymentCondition Static Analysis Findings

LPC-01S: Arbitrary Re-Entrancy

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 _receivers
115)
116external
117payable
118returns (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 else
136 _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.Fulfilled
145 );
146
147 emit Fulfilled(
148 _agreementId,
149 _did,
150 _id,
151 _rewardAddress,
152 _tokenAddress,
153 _receivers,
154 _amounts
155 );
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.