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

Gas Bombing Callback Functions on Withdrawal Prevents Emission of `MetadataUpdate` Event

amaron

Summary

The SablierV2Lockup::updateMetadata modifier emits an event at the end of the function:

/// @dev Emits an ERC-4906 event to trigger an update of the NFT metadata.
modifier updateMetadata(uint256 streamId) {
_;
emit MetadataUpdate({ _tokenId: streamId });
}

The SablierV2Lockup::withdraw function uses the updateMetadata modifier. However, there are two external calls at the end of the withdraw function that a malicious user can exploit to gas bomb the function, preventing the MetadataUpdate event from being emitted due to insufficient gas.

Vulnerability Details

The MetadataUpdate event is crucial for NFT marketplaces to update metadata. A malicious user can exploit this by consuming 63/64 of the remaining gas in the callback functions, effectively preventing the MetadataUpdate event from being emitted. This can lead to buyers purchasing streams with outdated and misleading information.

let's assume that there is a stream with status Settled (Settled status - All assets have been streamed; recipient is due to withdraw them.)
The SablierV2NFTDescriptor expresses the status of the stream through SablierV2NFTDescriptor::vars.status. When an attacker withdraws all funds from their stream using the SablierV2Lockup::withdrawMax function, the stream status changes to Depleted status (Depleted stream; all assets have been withdrawn and/or refunded.)

/// @dev See the documentation for the user-facing functions that call this internal function.
function _withdraw(uint256 streamId, address to, uint128 amount) internal {
// Effect: update the withdrawn amount.
_streams[streamId].amounts.withdrawn = _streams[streamId].amounts.withdrawn + amount;
​
// Retrieve the amounts from storage.
Lockup.Amounts memory amounts = _streams[streamId].amounts;
​
// Using ">=" instead of "==" for additional safety reasons. In the event of an unforeseen increase in the
// withdrawn amount, the stream will still be marked as depleted.
if (amounts.withdrawn >= amounts.deposited - amounts.refunded) {
// Effect: mark the stream as depleted.
@> _streams[streamId].isDepleted = true;
​
// Effect: make the stream not cancelable anymore, because a depleted stream cannot be canceled.
_streams[streamId].isCancelable = false;
}
​
// Retrieve the ERC-20 asset from storage.
IERC20 asset = _streams[streamId].asset;
​
// Interaction: perform the ERC-20 transfer.
asset.safeTransfer({ to: to, value: amount });
​
// Log the withdrawal.
emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount);
}

However, if the event is never emitted, the NFT status will temporarily still display as Settled, which could lead a buyer to purchase a stream based on misleading information, opening opportunities for attackers to scam.

Additionally, the two external calls in the SablierV2Lockup::withdraw function do not include a gas limit, allowing the first call to consume enough gas to prevent the second call from occurring.

// Interaction: if `msg.sender` is not the recipient and the recipient is a contract, try to invoke the
// withdraw hook on it without reverting if the hook is not implemented, and also without bubbling up
// any potential revert.
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
​
// Interaction: if `msg.sender` is not the sender, the sender is a contract and is different from the
// recipient, try to invoke the withdraw hook on it without reverting if the hook is not implemented, and also
// without bubbling up any potential revert.
if (msg.sender != sender && sender.code.length > 0 && sender != recipient) {
try ISablierV2Sender(sender).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }

Impact

If the MetadataUpdate event is not emitted, it might lead to users purchasing NFTs with misleading information, causing financial losses.

Tools Used

Manual Review

Recommendations

  1. emit the MetadataUpdate event before the external calls.

  2. Set a gas limit for the first external call to ensure it does not prevent the second call from being executed.

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

0xnevi Judge
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
vesla0x1 Auditor
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.