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

Incorrect Use of SafeERC20 for ERC20 Transfers

baouch21

Summery

The bug is from from the incorrect function call signature for the safeTransferFrom function provided by the SafeERC20
The function should be called with positional arguments, not named arguments.

Vulnerability Details

Here is the Vulnerable Line:

params.asset.safeTransferFrom({ from: msg.sender, to: address(this), value: createAmounts.deposit });

The function call should be

params.asset.safeTransferFrom(msg.sender, address(this), createAmounts.deposit);

For details the SafeERC20 provides wrappers around ERC20 operations that throw on failure when the token contract returns false.
And Tokens that return no value and instead revert or throw on failure are also supported, non-reverting calls are assumed to be successful.
And The incorrect use of the safeTransferFrom function with named arguments will cause a compilation error because in Solidity does not support the named arguments for function calls here is a scenario that show how the issue is arise and how affected the contract :

  • let’s say we have Alice wants to create a stream of 3,000 DAI to Bob for the month of January.

  • so the functions that gone be use is createWithDurations() and _create()
    Here :

function createWithDurations(LockupLinear.CreateWithDurations calldata params)
external
override
noDelegateCall
returns (uint256 streamId)
{
// Set the current block timestamp as the stream's start time.
LockupLinear.Timestamps memory timestamps;
timestamps.start = uint40(block.timestamp);
​
// Calculate the cliff time and the end time.
unchecked {
if (params.durations.cliff > 0) {
timestamps.cliff = timestamps.start + params.durations.cliff;
}
timestamps.end = timestamps.start + params.durations.total;
}
​
// Create the stream
streamId = _create(
LockupLinear.CreateWithTimestamps({
sender: params.sender,
recipient: params.recipient,
totalAmount: params.totalAmount,
asset: params.asset,
cancelable: params.cancelable,
transferable: params.transferable,
timestamps: timestamps,
broker: params.broker
})
);
}
​

And here is :

​
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.
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(msg.sender, address(this), createAmounts.deposit);
​
// Interaction: pay the broker fee, if not zero.
if (createAmounts.brokerFee > 0) {
params.asset.safeTransferFrom(msg.sender, params.broker.account, 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
});
}

  • The function call to safeTransferFrom with named arguments will fail to compile. This prevents the creation of the stream and the transfer of funds from Alice to the contract.

  • as result Alice cannot create a stream for Bob, and no tokens are transferred, leading to a failure in the primary functionality of the contract.

  • scenario can be used for attack :

  • let’s say Alice wants to stream funds to Bob
    So Alice initiates the createWithDurations function to create a stream of 3,000 DAI for Bob for January.

  • so the function Execution proceeds to set timestamps and calls _create.

  • When _create function attempts to call safeTransferFrom with the incorrect argument format, it results in a compilation error

  • as result the contract cannot be deployed or executed, and this preventing any streams from being created or funds transferred.

Impact

The incorrect use of the safeTransferFrom function with named arguments causes a compilation error, And this means that the contract cannot be deployed or executed. As a result, none of the intended functionalities, as creating streams, transferring funds, or streaming payments, can be executed

Tools Used

Manual review

Recommendations

the correct function signature is as this

params.asset.safeTransferFrom(msg.sender, address(this), createAmounts.deposit);
Updates

Lead Judging Commences

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.