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

```SablierV2Lockup::_streams``` mapping isn't updated after the stream burning leads to the stream existing even after a burn

EngineAtWork

Summary

The SablierV2Lockup::burn function deletes an NFT associated with a depleted stream but fails to update the _streams mapping accordingly. This results in the SablierV2Lockup::isStream() function incorrectly reporting that a stream exists even after its associated NFT has been burned.

Vulnerability Details

function burn(uint256 streamId) external override noDelegateCall notNull(streamId) {
// Check: only depleted streams can be burned.
if (!_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamNotDepleted(streamId);
}
​
// Check:
// 1. NFT exists (see {IERC721.getApproved}).
// 2. `msg.sender` is either the owner of the NFT or an approved third party.
if (!_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Effect: burn the NFT.
@> _burn({ tokenId: streamId });
}

Impact

The SablierV2Lockup::burn function deletes an NFT associated with a depleted stream but fails to update the _streams mapping accordingly. This results in the SablierV2Lockup::isStream() function incorrectly reporting that a stream exists even after its associated NFT has been burned.

Functions relying on isStream() to validate stream existence operates under false premises and all the get functions return inconsistent results (for ex. SablierV2Lockup::getStartTime, SablierV2Lockup::getEndTime).

To run the test:

In the file /burn/burn.t.sol add:

import { console } from "forge-std/src/Test.sol";
​
function test_streamsMappingNotDeleted()
external
whenNotDelegateCalled
givenNotNull
givenStreamHasBeenDepleted(streamId)
whenCallerAuthorized
{
bool isStreamBefore = lockup.isStream(streamId);
console.log("isStream before burning:", isStreamBefore);
​
// Burn the NFT so that it no longer exists.
lockup.burn(streamId);
​
bool isStreamAfter = lockup.isStream(streamId);
console.log("isStream after burning:", isStreamAfter);
​
//The stream already exists in the mapping after the burn.
assertEq(isStreamBefore, isStreamAfter);
}

Run the command: bun run test --match-test test_streamsMappingNotDeleted -vv

Logs:
[PASS] test_streamsMappingNotDeleted() (gas: 92883)
Logs:
isStream before burning: true
isStream after burning: true
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.69ms (1.71ms CPU time)

Tools Used

Manual review

Recommendations

Update the _streams mapping in the burn function to accurately reflect the deletion of the stream.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
0xnevi Judge
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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