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

Validation Failure in Edge Cases of Broker Fee Calculation

lordofterra

Summary

The checkAndCalculateBrokerFee function handles the calculation of fees and the net deposit amount. Edge cases arise from the use of integer arithmetic, which can lead to rounding down when dividing to calculate the fee amount. Solidity's integer division truncates any remainder, which, depending on the fee percentage and total amount, could result in a fee amount that is less than expected. Additionally, without proper overflow checks (which are built into Solidity 0.8.x), very large numbers could cause calculation errors.

function checkAndCalculateBrokerFee(
uint256 totalAmount,
uint256 brokerFee,
uint256 maxBrokerFee
)
internal
pure
returns (Lockup.CreateAmounts memory)
{
require(brokerFee <= maxBrokerFee, "Broker fee exceeds maximum allowed fee");
require(totalAmount > 0, "Total amount must be positive");
uint256 feeAmount = (totalAmount * brokerFee) / 1e18;
uint256 depositAmount = totalAmount - feeAmount;
require(feeAmount <= totalAmount, "Fee amount exceeds total amount");
​
return Lockup.CreateAmounts({
deposit: depositAmount,
brokerFee: feeAmount
});
}

Proof of Concept

  1. A user initiates a transaction with a total amount intended for a lockup or stream.

  2. The broker fee percentage is set close to the maxBrokerFee limit.

  3. Due to the fee calculation using integer division, the actual fee amount might round down, leading to the protocol collecting less than intended.

  4. Alternatively, if the fee percentage is low, the division could result in a fee amount that rounds to zero, effectively charging no fee.

  5. In cases where the total amount is very large, the multiplication by the fee percentage could potentially cause an overflow without proper safeguards, resulting in an incorrect fee amount.

Impact

Edge cases in fee calculation, such as rounding errors or overflow, could lead to incorrect fee amounts being charged. This could result in either the user paying more than expected (reducing the deposit amount) or the protocol collecting less in fees than intended.

Tools Used

Manual review

Recommendations

  1. Consider using a higher precision for the fee percentage or implementing a minimum fee amount to prevent the fee from rounding down to zero.

Here's an updated checkAndCalculateBrokerFee function with a few recommendations applied:

function checkAndCalculateBrokerFee(
uint128 totalAmount,
UD60x18 brokerFee,
UD60x18 maxBrokerFee
)
internal
pure
returns (Lockup.CreateAmounts memory amounts)
{
// Ensure the total amount is positive.
require(totalAmount > 0, "Total amount must be positive");
​
// Ensure the broker fee is within the acceptable limit.
require(brokerFee.le(maxBrokerFee), "Broker fee exceeds maximum allowed fee");
​
// Calculate the broker fee amount.
// The cast to uint128 is safe because the maximum fee is hard coded.
uint256 feeAmount = ud(totalAmount).mul(brokerFee).intoUint256();
require(feeAmount <= totalAmount, "Fee amount exceeds total amount");
​
// Calculate the deposit amount (the amount to stream, net of the broker fee).
uint128 depositAmount = totalAmount - uint128(feeAmount);
​
// Ensure the deposit amount is non-negative.
require(depositAmount >= 0, "Deposit amount is negative");
​
return Lockup.CreateAmounts({
deposit: depositAmount,
brokerFee: uint128(feeAmount)
});
}
​

Changes made:

  1. Replaced the initial check for totalAmount == 0 with a require statement to ensure the total amount is positive.

  2. Changed the if statement checking the broker fee against the maximum to a require for consistency and to provide an error message.

  3. Added a require to ensure the calculated fee amount does not exceed the total amount, which prevents the creation of a lockup with a negative deposit amount

  4. Removed the assert statement as it is redundant after the require checks.

  5. Updated the return statement to create a Lockup.CreateAmounts struct with the calculated depositAmount and brokerFee.

Updates

Lead Judging Commences

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

Support

FAQs

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