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

Low Report

pina

[L-01] Incorrect Arguments in SablierV2Lockup::BatchMetadataUpdate Event

Relevant GitHub Links

https://github.com/Cyfrin/2024-05-Sablier/blob/449257087fead2877a72fed5ce042549d81bd5a2/v2-core/src/abstracts/SablierV2Lockup.sol#L328-L328

Vulnerability Details

When SablierV2Lockup::setNFTDescriptor is called without any streams being created, it emits an incorrect argument in the event.

function setNFTDescriptor(ISablierV2NFTDescriptor newNFTDescriptor) external override onlyAdmin {
// Effect: set the NFT descriptor.
ISablierV2NFTDescriptor oldNftDescriptor = nftDescriptor;
nftDescriptor = newNFTDescriptor;
​
// Log the change of the NFT descriptor.
emit ISablierV2Lockup.SetNFTDescriptor({
admin: msg.sender,
oldNFTDescriptor: oldNftDescriptor,
newNFTDescriptor: newNFTDescriptor
});
​
// Refresh the NFT metadata for all streams.
@> emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}

The scenario is:

  • Initial Condition : nextStreamId = 1.

  • No Mint Condition: No NFTs are minted, hence nextStreamId remains 1.

  • SablierV2Lockup::setNFTDescriptor is called.

  • Event Emit: The event BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 }) will emit with _fromTokenId = 1 and _toTokenId = 0.

This situation could be interpreted as an update from token ID 1 to 0, which doesn't align with the reality that no tokens were minted or updated.

Recommendations

To handle this more effectively, you can conditionally modify the event emission based on the value of nextStreamId. Here are two approaches:

  • Approach 1: Conditional Event Emit Based on nextStreamId

You can conditionally emit the event only if nextStreamId is greater than 1, indicating that at least one token has been minted.

if (nextStreamId > 1) {
emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}

This approach ensures that the event is only emitted when there are actual tokens whose metadata might need updating.

  • Approach 2: Adjust Indices in Event Emit

If you need to emit the event regardless of whether any tokens are minted, perhaps for logging consistency or other operational reasons, you can adjust the indices to reflect a more logical output.

emit BatchMetadataUpdate({
_fromTokenId: nextStreamId > 1 ? 1 : 0,
_toTokenId: nextStreamId > 1 ? nextStreamId - 1 : 0
});

With this code, if no tokens have been minted (nextStreamId remains 1), the event will emit with both _fromTokenId and _toTokenId as 0, indicating no updates. If tokens are minted, it will emit the correct range.

[NC-01] Import duplication

https://github.com/Cyfrin/2024-05-Sablier/blob/449257087fead2877a72fed5ce042549d81bd5a2/v2-core/src/SablierV2LockupLinear.sol#L9-L10

Importing the same contract multiple times does not introduce any functional or security issues, but it should be avoided for better code clarity and maintenance.

[NC-02] Implement SablierV2Lookup::updateMetadata(streamid) modifier

https://github.com/Cyfrin/2024-05-Sablier/blob/449257087fead2877a72fed5ce042549d81bd5a2/v2-core/src/abstracts/SablierV2Lockup.sol#L256-L256
https://github.com/Cyfrin/2024-05-Sablier/blob/449257087fead2877a72fed5ce042549d81bd5a2/v2-core/src/abstracts/SablierV2Lockup.sol#L605-L605

- function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) {
+ function cancel(uint256 streamId) public override noDelegateCall notNull(streamId) updateMetadata(streamId) {
​
// _cancel function
- emit MetadataUpdate({ _tokenId: streamId });
​
Updates

Lead Judging Commences

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

0xnevi Judge
about 1 year ago
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

Support

FAQs

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