Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Broker Fee Zero Amount Check and Dynamic Range Validation

0xcodex

Summary

The checkAndCalculateBrokerFee function permits a maxFee parameter of 0, allowing transactions to proceed even when the broker’s fee is set to zero. Although the test passes successfully, it reveals that there are no validations in place to prevent broker.fee from being zero. Allowing a zero fee could lead to scenarios where no compensation is allocated to the broker, which may be unintended and lead to misuse or undervaluation of the broker's role. Furthermore, defining a minimum fee range as a percentage of totalAmount would dynamically align broker fees with transaction sizes, avoiding excessively low fees relative to transaction size.

Vulnerability Details

The lack of a validation check for broker.fee being zero allows transactions to proceed without allocating any fees to the broker. While this may be intentional in certain scenarios, it presents potential issues: brokers may receive no minimum compensation, which could discourage them from fulfilling their roles, and without tying broker.fee to the totalAmount, fees can be set arbitrarily low, leading to disproportionately small fees for larger transactions.

Impact

This vulnerability has two main impacts:

  1. Zero-Fee Broker Transactions: With no checks to ensure a non-zero fee, transactions could be structured with no broker compensation. In cases where brokers are required to maintain or facilitate complex operations, zero-fee transactions may discourage participation, degrade service quality, or create incentive misalignment.

  2. Misalignment Between Broker Fee and Transaction Size: Allowing fees independent of transaction size creates a potential for disproportionate fees, either too high or too low, especially for large transactions. This could lead to fee structures that are misaligned with broker contributions, potentially impacting overall transaction flow and broker incentives.

The below tests will show that the broker.fee can take in 0 amount or any other amounts.

PoC

import { Test, console } from "node_modules/forge-std/src/Test.sol";
import { Helpers } from "src/libraries/Helpers.sol";
import { ud, UD60x18 } from "@prb/math/src/UD60x18.sol";
import { SablierFlow } from "src/SablierFlow.sol";
import { ERC20Mock } from "./mocks/ERC20Mock.sol";
import { Errors } from "src/libraries/Errors.sol";
import { Broker } from "src/types/DataTypes.sol";
​
contract HelpersExposer {
using Helpers for uint128;
​
function checkAndCalculateBrokerFeeExposed(
uint128 totalAmount,
Broker memory broker,
UD60x18 maxFee
)
public
pure
returns (uint128 brokerFeeAmount, uint128 depositAmount)
{
return Helpers.checkAndCalculateBrokerFee(totalAmount, broker, maxFee);
}
}
​
contract CalculateAmountsTest is Test {
HelpersExposer exposer;
​
function setUp() public {
exposer = new HelpersExposer();
}
​
function testMaxFeeZero() public view{
uint128 totalAmount = 1000;
Broker memory broker = Broker({ fee: ud(0), account: address(0x1) });
UD60x18 maxFee = ud(0);
​
(uint128 brokerFeeAmount, uint128 depositAmount) =
exposer.checkAndCalculateBrokerFeeExposed(totalAmount, broker, maxFee);
​
assertEq(brokerFeeAmount, 0, "Broker fee should be 0 when maxFee is 0 and broker.fee is 0");
assertEq(depositAmount, totalAmount, "Deposit amount should be equal to totalAmount when broker.fee is 0");
}
​
function testInconsistentFeePercentage() public view{
uint128 totalAmount = 1000;
Broker memory broker = Broker({
// @notice you can uncomment any to run the test.
//fee: ud(0.05e18), //5%
//fee: ud(0.07e18), //7%
fee: ud(0.03e18), //3%
account: address(0x1)
});
UD60x18 maxFee = ud(0.1e18);
​
(uint128 brokerFeeAmount, uint128 depositAmount) = exposer.checkAndCalculateBrokerFeeExposed(totalAmount, broker, maxFee);
//@notice uncomment to match with above.
//uint128 expectedBrokerFee = (totalAmount * 5) / 100;
//uint128 expectedBrokerFee = (totalAmount * 7) / 100;
uint128 expectedBrokerFee = (totalAmount * 3) / 100;
uint128 expectedDepositAmount = totalAmount - expectedBrokerFee;
​
assertEq(brokerFeeAmount, expectedBrokerFee, "Incorrect broker fee amount");
assertEq(depositAmount, expectedDepositAmount, "Incorrect deposit amount after broker fee deduction");
​
}
}

Tools Used

Manual Review

Recommendations

To address these issues, we recommend implementing the following improvements:

  1. Implement a Minimum Broker Fee Check: Ensure broker.fee is greater than zero by adding a validation check within checkAndCalculateBrokerFee:

    require(broker.fee > 0, "Broker fee cannot be zero");

    This will prevent transactions from proceeding without a broker fee.

  2. Define Broker Fee as a Percentage Range of totalAmount: Link the broker fee to totalAmount to ensure fees are proportionate to transaction size. This could be achieved by setting a minimum and maximum percentage range, such as between 0.5% and 2% of totalAmount. Example:

    require(broker.fee >= minPercentage && broker.fee <= maxPercentage, "Broker fee out of range");

    Where minPercentage and maxPercentage are constants or configurable parameters that define the valid range of broker fees as a percentage of totalAmount. This approach aligns broker fees with transaction size, enhancing flexibility and fairness.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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