Sablier

Sablier
DeFiFoundry
53,440 USDC
View results
Submission Details
Severity: medium
Valid

`SablierV2Lockup.sol` - The caller of withdraw and renounce can skip callbacks, by sending less gas

Summary

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.

Vulnerability Details

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:

if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}

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:

In SablierV2Lockup, when onLockupStreamCanceled(), onLockupStreamWithdrawn() and onLockupStreamRenounced() hooks are called, there could be gas bomb attacks because we do not limit the gas transferred to the external contract. This is intended to support complex transactions implemented in those hooks.

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

Impact

Skipping important logic, breaking the atomicity of the system.

Tools Used

Manual Review

Recommendations

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.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Someone could skip callback by sending just the right amount of gas

0xnevi Judge
about 1 year ago
nmirchev8 Submitter
about 1 year ago
drynooo Auditor
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
nmirchev8 Submitter
about 1 year ago
ge6a Judge
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Someone could skip callback by sending just the right amount of gas

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.