Sablier

Sablier
DeFiFoundry
53,440 USDC
View results
Submission Details
Severity: low
Invalid

Improper Use of assert for Input Validation

Summary

The checkAndCalculateBrokerFee function uses assert instead of require to check that the total amount is greater than the broker fee amount. This practice is not recommended because assert is intended for internal consistency checks and invariants, while require is designed for input validation and conditions that depend on external factors. Using assert in this context can lead to higher gas consumption if the condition fails.

Vulnerability Details

Consider the following code snippet from the checkAndCalculateBrokerFee function:

// Assert that the total amount is strictly greater than the broker fee amount.
assert(totalAmount > amounts.brokerFee);

Impact

The primary impact of using assert instead of require is related to gas consumption:

  1. Higher Gas Costs on Failure: When an assert statement fails, it consumes all remaining gas, whereas a failed require statement refunds the remaining gas. This could result in higher gas costs for users if the condition fails unexpectedly.

  2. Misleading Error Handling: assert is typically used for conditions that should never fail, such as internal invariants. Using assert for input validation can be misleading, suggesting that the failure is due to a bug in the code rather than invalid user input.

Tools Used

Manual review

Recommendations

Replace assert with require to properly handle the validation and ensure that the remaining gas is refunded if the condition fails:

Recommended code changes:

function checkAndCalculateBrokerFee(
uint128 totalAmount,
UD60x18 brokerFee,
UD60x18 maxBrokerFee
)
internal
pure
returns (Lockup.CreateAmounts memory amounts)
{
// When the total amount is zero, the broker fee is also zero.
if (totalAmount == 0) {
return Lockup.CreateAmounts(0, 0);
}
// Check: the broker fee is not greater than `maxBrokerFee`.
if (brokerFee.gt(maxBrokerFee)) {
revert Errors.SablierV2Lockup_BrokerFeeTooHigh(brokerFee, maxBrokerFee);
}
// Calculate the broker fee amount.
// Handle precision errors and ensure the calculation is safe.
uint256 brokerFeeAmount = ud(totalAmount).mul(brokerFee).intoUint256();
// Cast to uint128 safely, ensuring no overflow.
require(brokerFeeAmount <= type(uint128).max, "Broker fee amount overflow");
amounts.brokerFee = uint128(brokerFeeAmount);
// Ensure that the total amount is strictly greater than the broker fee amount.
require(totalAmount > amounts.brokerFee, "Total amount must be greater than broker fee");
// Calculate the deposit amount (the amount to stream, net of the broker fee).
amounts.deposit = totalAmount - amounts.brokerFee;
}

Description of Changes

  1. Replaced the assert statement with a require statement to ensure that the total amount is greater than the broker fee amount.
    This change ensures that the transaction fails gracefully with a refund of remaining gas if the condition is not met.

  2. Included a check to ensure that the broker fee amount does not exceed the maximum value for uint128.
    This addition helps prevent overflow issues and ensures the correctness of the broker fee calculation.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Info/Gas/Invalid as per Docs

https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

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