Sablier

Sablier
DeFiFoundry
53,440 USDC
View results
Submission Details
Severity: low
Invalid

`SablierV2Lockup.sol` - Callbacks can consume 63/64 of the gas to DoS and increase gas fees for renounce and for withdraw.

Summary

The protocol implements callbacks to either the recipient or the sender of a stream in _cancel , withdraw, renounce.

You'll notice that in the case of withdraw and renounce, the callbacks aren't technically the last piece of code that executes, both the functions have the updateMetadata modifier attached to them.

modifier updateMetadata(uint256 streamId) {
_;
emit MetadataUpdate({ _tokenId: streamId });
}

After the function is finished, the modifier will emit the event MetadataUpdate.

Vulnerability Details

Knowing this, the callbacks can be weaponized and consume all the gas that is sent to them, in Solidity when an external call is made, 63/64 of the tx gas is sent to the callee, meaning that the callbacks can consume all that gas and after they are done, there won't be enough gas to emit the event.

  • In the case of renounce, the recipient of the stream can maliciously increase the gas cost of the transaction, forcing the sender to pay more to execute renounce and in some edge cases, the tx might be DoS'ed, if not enough gas is called.

  • In the case of withdraw the sender of the stream, can inflate the gas cost of each withdraw call, forcing the recipient or an approved operator to pay more gas for each withdraw call. Again in some edge cases, there might not be enough gas to actually emit the event in the end, DoSing the function.

  • Considering withdraw in almost all cases will be called multiple times, the gas that the recipient or approved operator have to pay will be heavily inflated and in some rare cases, if the stream is holding a small amount of funds, it might not even be worth it to call withdraw to retrieve the funds.

Another way this can happen is if both callbacks consume quite a lot of gas, without being malicious there might not be enough gas to emit the event and finish the transaction.

Note that this issue is very similar to issue 3.2 reported by Cantina, which was the last audit the protocol had before this and the issue was considered a Medium.

That issue would only slightly increase the gas cost and DoS cancel once, but in the case of withdraw it can technically be DoS'ed multiple times and in the long term will cost much more gas for the recipient, because of the way streams work, because of this, we think this is a Medium severity issue as well.

Also note that isn't a gas bomb attack, as the gas that the callback eats up, doesn't have to be much and is chosen by the caller of withdraw, it just has to consume all of it in order to force the emit event to revert.

Impact

  • Possibility of DoS
    -Overpaying gas

Tools Used

Manual Review

Recommendations

We recommend the same fix, as 3.2:

Change updateMetadata so that it emits the event before the execution of the underlying function, not after it. This way the callbacks will always be the last piece of code executed, thus they can't have the same effect.

Also as stated in the Cantina report, events are read off-chain after the transaction, so it doesn't matter where/when the event is emitted in the code.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Withdraw DoS enabled by external call and `updateMetadata` modifier

charlescheerful Auditor
about 1 year ago
bladesec Auditor
about 1 year ago
alymurtazamemon Judge
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
vesla0x1 Auditor
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Withdraw DoS enabled by external call and `updateMetadata` modifier

Support

FAQs

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