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

Stream sender is unable to cancel a stream with a pausable asset that is paused

0xspryon

Summary

When the stream sender cancels a stream, we call the asset transfer method to reimburse the amount that is yet to be streamed to the sender. If this call reverts, then the call to cancel the stream reverts as well.

Vulnerability Details

There exists ERC20 tokens that are pausable.
For example USDT on ethereum ( verifiable on etherscan )
If for whatever reason the stream asset is paused, then the stream Sender is unable to cancel the stream until the asset is unpaused.

POC

Replace the content of `v2-core/test/mocks/erc20/ERC20Mock.sol` with the below code block
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.22;
​
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
import "forge-std/src/console.sol";
​
contract ERC20Mock is ERC20, ERC20Pausable {
​
constructor(string memory name, string memory symbol) ERC20(name, symbol) { }
​
function pause() public {
_pause();
console.log("Contract paused");
}
​
function transfer(address to, uint256 value) override public whenNotPaused returns (bool) {
return super.transfer(to, value) ;
}
​
function _update(address from, address to, uint256 value)
internal
override(ERC20, ERC20Pausable)
{
​
super._update(from, to, value);
}
}
Add the below codeblock in `v2-core/test/integration/concrete/lockup/cancel/cancel.t.sol` Notice the importation of the ERC20Mock file.
import { ERC20Mock } from "../../../../../test/mocks/erc20/ERC20Mock.sol";
​
function test_CancelAssetPaused()
external
whenNotDelegateCalled
givenNotNull
givenStreamWarm
whenCallerAuthorized
givenStreamCancelable
givenStatusStreaming
givenRecipientContract
givenRecipientImplementsHook
whenRecipientDoesNotRevert
whenNoRecipientReentrancy
{
// Create the stream.
uint256 streamId = createDefaultStreamWithRecipient(address(goodRecipient));
​
// Pause the stream Asset
ERC20Mock(address(lockup.getAsset(defaultStreamId))).pause();
​
// Cancel the stream.
vm.expectRevert();
lockup.cancel(streamId);
​
// Assert that the stream's status is still "STREAMING".
Lockup.Status actualStatus = lockup.statusOf(streamId);
Lockup.Status expectedStatus = Lockup.Status.STREAMING;
assertEq(actualStatus, expectedStatus);
​
// Assert that the stream is still cancelable.
bool isCancelable = lockup.isCancelable(streamId);
assertTrue(isCancelable, "isCancelable");
​
// Assert that the refunded amount has not been updated.
uint128 actualRefundedAmount = lockup.getRefundedAmount(streamId);
uint128 expectedRefundedAmount = 0;
assertEq(actualRefundedAmount, expectedRefundedAmount, "refundedAmount");
}

run the below command in the terminal
forge test --mt test_CancelAssetPaused -vvv

Impact

The stream sender is temporarily unable to cancel the stream which leads to a loss of funds as the stream will continue streaming the assets to the Stream Receiver. The impact of the loss of funds depends on how long the asset has been paused for, relative to how fast the asset is being streamed. For the sake of an example, imagine the asset is paused just as the stream is about to get past the Stream's cliff period and the sender wishes to cancel the stream.

likelihood: medium as assets are paused only under cases of force majeure but taking into account that Sablier supports all ERC20 assets on all EVM compatible chains, the probability of a pausing on an asset happening is not low.

impact: high as all cancelable asset streams effectively become uncancelable while the asset is paused. Knowing USDT's stature as one of the well respected and transacted stablecoins, this will translate to a lot of streams(without taking into account the other pausable assets on all the evm compatible chains where Sablier is/would be deployed).

Tools Used

Fuzzing with foundry

Recommendations

Similar to how when the Stream is canceled the Stream Receiver is expected to withdraw the streamed assets by himself in a call seperate to the call to cancel the stream, we should seperate the transfer of the Senders funds from the cancel funtion into a seperate function

Updates

Lead Judging Commences

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

Push over pull for `_cancel` in context of Pausable tokens

0xnevi Judge
about 1 year ago
0xspryon Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
0xspryon Submitter
about 1 year ago
0xspryon Submitter
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Push over pull for `_cancel` in context of Pausable tokens

Support

FAQs

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