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

Missing Validation for Broker Parameters Allows Broker to Set Fee Higher than Maximum

TxGuard

Summary

A vulnerability was identified in the SablierFlow smart contract where there is missing validation for broker parameters, specifically the broker fee percentage in the depositViaBroker function. This allows brokers to set fees exceeding the maximum allowable limit or even manipulate fees to cause arithmetic overflows. Such issues can lead to financial losses for users and compromise the integrity of the contract.

Vulnerability Details

In the SablierFlow contract, the depositViaBroker function allows deposits to be made through a broker, who can charge a fee specified as a percentage (UD60x18 format). There is a supposed maximum fee percentage (maxFeePercentage), set to 10% (1e17 in UD60x18 format), intended to cap the broker's fee.

However, the contract lacks proper validation to enforce this maximum fee. As a result:

  • Brokers can set a fee equal to or exceeding the maximum limit without triggering a revert.

  • There is a risk of arithmetic overflows when large amounts and high fees are combined, potentially leading to incorrect fee calculations or contract failure.

Proof of Concept (PoC)

  1. Modify the mock file: tests/mocks/ERC20Mock.sol with this content:

    // SPDX-License-Identifier: UNLICENSED
    pragma solidity >=0.8.22;
    ​
    import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
    ​
    contract ERC20Mock is ERC20 {
    uint8 internal immutable DECIMAL;
    ​
    constructor(string memory name_, string memory symbol_, uint8 decimals_) ERC20(name_, symbol_) {
    DECIMAL = decimals_;
    }
    ​
    function decimals() public view override returns (uint8) {
    return DECIMAL;
    }
    ​
    function mint(address to, uint256 amount) public {
    _mint(to, amount);
    }
    }
    ​
    ​

  2. Create the main test file: tests/SablierFlowBrokerFeeTest.t.sol and run the tests using command: forge test --mt testInternalFeeCalculationConsistency -vvvv

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.22;
​
import "forge-std/src/Test.sol";
import "../src/SablierFlow.sol";
import "./mocks/ERC20Mock.sol";
import { IFlowNFTDescriptor } from "../src/interfaces/IFlowNFTDescriptor.sol";
import { IERC721Metadata } from "@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol";
import { UD60x18, ud60x18 } from "@prb/math/src/UD60x18.sol";
​
// Mock NFT descriptor to fulfill contract dependencies
contract MockNFTDescriptor is IFlowNFTDescriptor {
function tokenURI(IERC721Metadata /*sablierFlow*/, uint256 /*streamId*/) external pure override returns (string memory) {
return "MockURI";
}
}
​
contract SablierFlowBrokerFeeTest is Test {
SablierFlow sablierFlow;
ERC20Mock token;
MockNFTDescriptor nftDescriptor;
​
address sender = address(0x123);
address recipient = address(0x456);
address brokerAccount = address(0x789);
UD60x18 maxFeePercentage = ud60x18(1e17); // 10% in UD60x18 format
​
function setUp() public {
// Deploy the mock token and NFT descriptor
token = new ERC20Mock("Mock Token", "MTK", 18);
nftDescriptor = new MockNFTDescriptor();
console.log("Mock Token deployed at address:", address(token));
console.log("Mock NFT Descriptor deployed at address:", address(nftDescriptor));
​
// Deploy the SablierFlow contract
sablierFlow = new SablierFlow(address(this), nftDescriptor);
console.log("SablierFlow contract deployed at address:", address(sablierFlow));
​
// Mint tokens to the sender
token.mint(sender, 1_000_000 * 1e18);
console.log("Minted 1,000,000 tokens to sender at address:", sender);
​
// Sender approves the SablierFlow contract to spend tokens
vm.startPrank(sender);
token.approve(address(sablierFlow), type(uint256).max);
console.log("Sender approved SablierFlow to spend unlimited tokens");
vm.stopPrank();
}
​
function testEdgeCaseMaxFee() public {
console.log("\\\\nTesting edge case with broker fee just below the maximum allowed...");
​
uint128 amount = 1_000 * 1e18; // Deposit amount: 1,000 tokens
UD21x18 ratePerSecond = UD21x18.wrap(1e18); // Rate per second: 1 token
// Broker fee set to just below the maximum (10% - 1 wei)
Broker memory broker = Broker(brokerAccount, maxFeePercentage.sub(ud60x18(1)));
​
vm.startPrank(sender);
console.log("Prank started as sender:", sender);
​
// Create a new stream
uint256 streamId = sablierFlow.create(sender, recipient, ratePerSecond, token, true);
console.log("Stream created with ID:", streamId);
​
// Deposit via broker with fee just below the max allowed
sablierFlow.depositViaBroker(streamId, amount, sender, recipient, broker);
console.log("Deposited", amount, "tokens via broker with fee just below maximum");
​
// Check broker's token balance
uint256 brokerBalance = token.balanceOf(brokerAccount);
uint256 expectedBrokerFee = (amount * (maxFeePercentage.sub(ud60x18(1)).unwrap())) / 1e18;
console.log("Broker's balance:", brokerBalance);
console.log("Expected broker fee:", expectedBrokerFee);
​
// Assert that the broker received the correct fee
assertEq(brokerBalance, expectedBrokerFee, "Broker balance should match the fee close to max.");
console.log("Assertion passed: Broker received correct fee");
​
vm.stopPrank();
console.log("Prank stopped as sender");
}
​
function testBoundaryMaxAllowableFee() public {
console.log("\\\\nTesting broker fee at the maximum allowable limit...");
​
uint128 amount = 2_000 * 1e18; // Deposit amount: 2,000 tokens
// Broker fee set exactly at the maximum allowed (10%)
Broker memory broker = Broker(brokerAccount, maxFeePercentage);
​
vm.startPrank(sender);
console.log("Prank started as sender:", sender);
​
// Create a new stream
uint256 streamId = sablierFlow.create(sender, recipient, UD21x18.wrap(1e18), token, true);
console.log("Stream created with ID:", streamId);
​
// Expect the transaction to revert due to exceeding max fee
vm.expectRevert();
sablierFlow.depositViaBroker(streamId, amount, sender, recipient, broker);
console.log("Attempted to deposit via broker with maximum allowable fee");
​
vm.stopPrank();
console.log("Prank stopped as sender");
}
​
function testOverflowRiskWithHighFee() public {
console.log("\\\\nTesting overflow risk with high broker fee...");
​
uint128 highAmount = type(uint128).max / 2; // Very large deposit amount
// Broker fee set to an extremely high value (e.g., 50%)
Broker memory broker = Broker(brokerAccount, maxFeePercentage.mul(ud60x18(5)));
​
vm.startPrank(sender);
console.log("Prank started as sender:", sender);
​
// Create a new stream
uint256 streamId = sablierFlow.create(sender, recipient, UD21x18.wrap(1e18), token, true);
console.log("Stream created with ID:", streamId);
​
// Expect the transaction to revert due to overflow risk
vm.expectRevert();
sablierFlow.depositViaBroker(streamId, highAmount, sender, recipient, broker);
console.log("Attempted to deposit a very large amount via broker with high fee");
​
vm.stopPrank();
console.log("Prank stopped as sender");
}
​
function testInternalFeeCalculationConsistency() public {
console.log("\\\\nTesting internal fee calculation consistency...");
​
uint128 amount = 1_000 * 1e18; // Deposit amount: 1,000 tokens
// Broker fee set to maximum allowed (10%)
Broker memory broker = Broker(brokerAccount, maxFeePercentage);
​
vm.startPrank(sender);
console.log("Prank started as sender:", sender);
​
// Create a new stream
uint256 streamId = sablierFlow.create(sender, recipient, UD21x18.wrap(1e18), token, true);
console.log("Stream created with ID:", streamId);
​
// Expect the transaction to revert due to fee exceeding the maximum
vm.expectRevert();
sablierFlow.depositViaBroker(streamId, amount, sender, recipient, broker);
console.log("Attempted to deposit via broker with fee at maximum allowed");
​
vm.stopPrank();
console.log("Prank stopped as sender");
}
}
​
​

  1. Now the test fails, meaning the revert didn’t happpen and the transaction for fee exceeding the maximum went through successfully

Explanation:

  • Setup Phase:

    • Contract Deployment:

      • Deploys ERC20Mock token and MockNFTDescriptor to satisfy dependencies.

      • Deploys the SablierFlow contract.

      • Logs the addresses for clarity.

    • Token Minting and Approval:

      • Mints 1,000,000 tokens to the sender.

      • sender approves the SablierFlow contract to spend tokens on their behalf.

      • Logs actions for transparency.

  • Test Cases:

    1. testEdgeCaseMaxFee:

      • Sets the broker fee just below the maximum allowed (10% - 1 wei).

      • Creates a stream and deposits via the broker.

      • Checks that the broker receives the correct fee.

      • Logs all steps and validates that no revert occurs.

    2. testBoundaryMaxAllowableFee:

      • Sets the broker fee exactly at the maximum allowed (10%).

      • Expects the transaction to revert, but due to missing validation, it might not.

      • Logs the attempt and the expectation.

    3. testOverflowRiskWithHighFee:

      • Attempts to deposit a very large amount with an excessively high broker fee (e.g., 50%).

      • Expects the transaction to revert due to overflow risk.

      • Highlights the lack of validation for preventing such scenarios.

    4. testInternalFeeCalculationConsistency:

      • Tests fee calculation when the broker fee is at the maximum allowed.

      • Expects a revert due to fee limits, emphasizing inconsistencies in internal calculations.

Underlying Issue:

The SablierFlow contract does not enforce validation on the broker.fee parameter within the depositViaBroker function. This omission allows brokers to:

  • Set fees equal to or exceeding the maximum limit without reversion.

  • Input excessively high fees that can cause arithmetic overflows during fee calculation.

The lack of checks against maxFeePercentage and absence of overflow protections pose significant risks to the contract's security and user funds.

Impact

  • Financial Loss for Users:

    • Users may be overcharged by brokers setting excessive fees.

    • Funds intended for the recipient may be unfairly diverted to brokers.

  • Contract Vulnerability:

    • Potential for arithmetic overflows leading to incorrect fee calculations.

    • May cause the contract to behave unexpectedly or fail.

  • Reputation Damage:

    • Trust in the SablierFlow contract could be compromised.

    • Users may lose confidence due to potential exploitation.

Tools Used

  • Foundry: A fast, portable, and modular toolkit for Ethereum application development.

  • Solidity Compiler (solc): Version 0.8.22 used for compiling the smart contracts.

  • Forge Std Library: Provides utilities for testing, including console.log for detailed output.

  • PRB Math Library: Used for fixed-point math operations (UD60x18 and UD21x18 types).

Recommendations

  • Implement Parameter Validation:

    • Enforce checks in the depositViaBroker function to ensure broker.fee does not exceed maxFeePercentage.

      require(broker.fee <= maxFeePercentage, "Broker fee exceeds maximum allowed percentage");
      ​
      ​

    • Validate that broker.fee is a reasonable value (e.g., non-negative).

  • Add Overflow Protections:

    • Use safe math operations when calculating fees to prevent overflows.

      uint256 brokerFeeAmount = amount.mul(broker.fee.unwrap()).div(1e18);
      ​
      ​

    • Consider using OpenZeppelin's SafeMath library or Solidity's built-in overflow checks.

  • Enhance Unit Testing:

    • Include tests for edge cases where broker.fee is at, below, and above the maximum allowed.

    • Test for arithmetic overflows with large amounts and high fees.

  • Update Documentation:

    • Clearly document the acceptable range for broker fees.

    • Provide guidelines for brokers and users on fee expectations.

  • Code Review and Auditing:

    • Perform a thorough code review focusing on parameter validations.

    • Engage third-party auditors to assess the contract for similar vulnerabilities.

By addressing these issues, the SablierFlow contract will:

  • Prevent brokers from setting excessive or harmful fees.

  • Protect users from potential financial losses.

  • Enhance the overall security and reliability of the contract.

Note: Always ensure that changes are thoroughly tested in a secure environment before deploying to production networks.

Updates

Lead Judging Commences

inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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