Submission Details
Severity:
Depositing a token that takes fee on transfer will result in incorrect balance in the sablier contract
Summary
According to the docs sablier supportsall erc20 token which means it also supports fee on transfer tokens
Sablier protocol does not check to confirm if the amount deposited is equal the amount recieved
Assume TokenA is a fee on transfer token with 10% fee on transfer
Assume Alice wants to stream 3000TokenA to Bob , Alice deposit 3000Token A the sablier contract,sablier contract update its balance to 3000TokenA but the balance on the token A contract is 2700TokenA because of the 10% fee on transfer
Vulnerability Details
v2-core\src\SablierV2LockupDynamic.sol , v2-core\src\SablierV2LockupLinear.sol , v2-core\src\SablierV2LockupTranched.sol
sablier contracts above does not check to confirm if the amount deposited equals the amount recieved in the
SablierV2LockupDynamic::_create(LockupDynamic.CreateWithTimestamps memory params) ,
SablierV2LockupLinear::_create(LockupDynamic.CreateWithTimestamps memory params)
SablierV2LockupTranched::_create(LockupDynamic.CreateWithTimestamps memory params)
Impact
-
If nobody else deposited TokenA and Alice wants to cancel his stream the transaction will be reverted because 3000TokenA will be transfered to Alice but the balance of sablier contract in tokenA contract is 2700TokenA "SablierV2Lockup::cancel(uint256 streamId)"
-
If the sablier conrtract balance was 5000TokenA and Alice cancels his 3000TokenA, 3000TokenA will be transfered instead of 2700tokenA which means a loss of 300TokenA for the sablier protocol
Tools Used
Manual Review
Recommendations
An extra check should be added to verify that the token transfered to the sablier contract is same recieved
function _create(LockupDynamic.CreateWithTimestamps memory params) internal returns (uint256 streamId) {
✔ uint256 initialBalance = params.asset.balanceOf(address(this));
// Interaction: transfer the deposit amount.
✔ params.asset.safeTransferFrom({ from: msg.sender, to: address(this), value: params.totalAmount });
✔ uint256 finalBalance = params.asset.balanceOf(address(this));
✔ uint256 AmountIn = finalBalance - initialBalance;
// Check: verify the broker fee and calculate the amounts.
✔ Lockup.CreateAmounts memory createAmounts =
Helpers.checkAndCalculateBrokerFee(AmountIn, params.broker.fee, MAX_BROKER_FEE);
❌ Lockup.CreateAmounts memory createAmounts =
Helpers.checkAndCalculateBrokerFee(params.totalAmount, params.broker.fee, MAX_BROKER_FEE);
// Check: validate the user-provided parameters.
Helpers.checkCreateLockupDynamic(createAmounts.deposit, params.segments, MAX_SEGMENT_COUNT, params.startTime);
// Load the stream ID in a variable.
streamId = nextStreamId;
// Effect: create the stream.
Lockup.Stream storage stream = _streams[streamId];
stream.amounts.deposited = createAmounts.deposit;
stream.asset = params.asset;
stream.isCancelable = params.cancelable;
stream.isStream = true;
stream.isTransferable = params.transferable;
stream.sender = params.sender;
stream.startTime = params.startTime;
unchecked {
// The segment count cannot be zero at this point.
uint256 segmentCount = params.segments.length;
stream.endTime = params.segments[segmentCount - 1].timestamp;
// Effect: store the segments. Since Solidity lacks a syntax for copying arrays of structs directly from
// memory to storage, a manual approach is necessary. See https://github.com/ethereum/solidity/issues/12783.
for (uint256 i = 0; i < segmentCount; ++i) {
_segments[streamId].push(params.segments[i]);
}
// Effect: bump the next stream ID.
// Using unchecked arithmetic because these calculations cannot realistically overflow, ever.
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 ISablierV2LockupDynamic.CreateLockupDynamicStream({
streamId: streamId,
funder: msg.sender,
sender: params.sender,
recipient: params.recipient,
amounts: createAmounts,
asset: params.asset,
cancelable: params.cancelable,
transferable: params.transferable,
segments: params.segments,
timestamps: LockupDynamic.Timestamps({ start: stream.startTime, end: stream.endTime }),
broker: params.broker.account
});
}
Updates
Lead Judging Commences
inallhonesty about 1 year ago
Submission Judgement Published Reason: Known issue
Assigned finding tags:
Known - Contest Details
https://www.codehawks.com/contests/clvb9njmy00012dqjyaavpl44
Prize pool breakdown
Total prize
53,440 USDC
nSLOC
2,65520 USDC / LOC
45,000 USDC
3,100 USDC
5,340 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.