Both withdraw
and renounce
implement a total of 3 callbacks.
In the case of withdraw
there is a callback to the recipient and to the sender and in the case of renounce
, only a callback to the recipient.
The issue here is that the caller of the function chooses how much gas he uses for the transaction, because of this he can specify an amount of gas that would be enough to execute the entire transaction, but not enough to execute the callbacks.
Since withdraw
is called by either the owner of the NFT or an approved operator, either one can call withdraw with an amount of gas that won't be enough for the sender callback to execute.
In the case of renounce
, the sender can specify an amount of gas that won't be enough to execute the callback of the recipient.
Since the callback is wrapped in a try/catch
, the caller can send enough gas so that the callback won't have enough to execute, reverting and entering the catch block, but the whole transaction will still have enough to execute. This happens because of the 63/64 rule, when an external call is made only 63/64 gas is forwarded to it, meaning there is always some leftover gas so the rest of the transaction can execute.
In our case, only an event is emitted after the callbacks, so we only need around 1600 gas left over.
This means that sender may calculate the gas, so the callback will recieve 63 * 1600 = 100800
, which may not be enough for the complex operations inside the callback. This would result in failed callback execution, but success in _withdraw
, function, because we have a catch
block and another 1600 gas to emit the event after the callback:
As stated by the protocol, the callbacks can be complex and vital to both the sender
and the recipient
, so skipping them is not good, as logic that might be vital to them, won't execute and those 100800 units of gas may not be enough.
README statement:
NOTE
Note that this issue differs from one, which enforces function caller to send more gas, because of the event emission after the callback. The fix for that, would even make the current attack path easier to implement for malicious actor, because he could make callback function receive even less gas and still accomplish successful withdraw
transaction
Skipping important logic, breaking the atomicity of the system.
Manual Review
Not sure what is the best solution here.
The best would be to remove the callback functionality. Only events could be emitted and if partyes are interested in taking executing actions on those events, they will implement off-chain listeners with event emittion as triggeres.
https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.