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

Lack of Token Standard Enforcement in SablierFlow Causes Security Risks and Service Interruptions

TxGuard

Summary

The SablierFlow contract does not adequately handle interactions with non-compliant ERC20 tokens. These tokens may deviate from the standard ERC20 behavior by reverting on transfers, failing to return the expected boolean value, or executing malicious code during token operations. This vulnerability can lead to unexpected failures, denial of service, or security risks like unauthorized withdrawals.

Vulnerability Details

Description

The SablierFlow contract assumes that all ERC20 tokens it interacts with strictly adhere to the ERC20 standard, particularly regarding the transfer and transferFrom functions returning a boolean value indicating success. However, some tokens deviate from this standard by:

  • Reverting on Transfers: Tokens that revert transactions during transfer or transferFrom calls instead of returning false.

  • Failing to Return Boolean Values: Tokens that do not return any value or return values other than bool.

  • Executing Malicious Code: Tokens that include harmful logic within their transfer functions, such as reentrant calls to the SablierFlow contract.

The provided test cases demonstrate how these non-compliant tokens can cause the SablierFlow contract to fail or behave unexpectedly.

Proof of Concept

Test Cases Demonstrating the Vulnerability

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.22;
​
// Import necessary dependencies
import "forge-std/src/Test.sol";
import "../src/SablierFlow.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract NonCompliantERC20 is ERC20 {
constructor() ERC20("Non-Compliant Token", "NCT") {
// Mint 1 million tokens to the deployer (test contract)
_mint(msg.sender, 1e24);
}
​
// Override the transfer function to always revert
function transfer(address, uint256) public pure override returns (bool) {
revert("Non-compliant token: transfer failed");
}
}
​
contract SablierFlowNonCompliantTokenTest is Test {
SablierFlow public sablierFlow;
NonCompliantERC20 public nonCompliantToken;
address public sender;
address public recipient;
​
function setUp() public {
// Deploy the SablierFlow contract
sablierFlow = new SablierFlow(address(this), IFlowNFTDescriptor(address(0)));
emit log_named_address("SablierFlow deployed at", address(sablierFlow));
​
// Deploy the NonCompliantERC20 token
nonCompliantToken = new NonCompliantERC20();
emit log_named_address("NonCompliantERC20 deployed at", address(nonCompliantToken));
​
// Define sender and recipient addresses
sender = address(0x123);
recipient = address(0x456);
​
// Transfer tokens to the sender for testing
nonCompliantToken.transfer(sender, 1e22);
emit log_named_uint("Sender's token balance", nonCompliantToken.balanceOf(sender));
}
​
function testNonCompliantTokenFailure() public {
emit log("=== Starting testNonCompliantTokenFailure ===");
​
// Start impersonating the sender
vm.startPrank(sender);
emit log("Impersonating sender");
​
// Sender approves SablierFlow to spend tokens
nonCompliantToken.approve(address(sablierFlow), type(uint256).max);
emit log("Sender approved SablierFlow to spend tokens");
​
// Attempt to create a stream using the non-compliant token
try sablierFlow.create(
sender,
recipient,
UD21x18.wrap(1e18), // ratePerSecond is 1 token per second
IERC20(address(nonCompliantToken)),
true
) {
// If the create function does not revert, the test should fail
emit log("Expected failure with non-compliant token, but create succeeded");
fail();
} catch Error(string memory reason) {
// Log the revert reason
emit log_named_string("Revert reason", reason);
assertEq(reason, "Non-compliant token: transfer failed");
}
​
// Stop impersonating the sender
vm.stopPrank();
emit log("Stopped impersonating sender");
​
emit log("=== Completed testNonCompliantTokenFailure ===");
}
}
​

1. Non-Compliant ERC20 Token That Reverts on Transfer

contract NonCompliantERC20 is ERC20 {
constructor() ERC20("Non-Compliant Token", "NCT") {
_mint(msg.sender, 1e24); // Mint 1 million tokens to deployer
}
​
function transfer(address, uint256) public pure override returns (bool) {
revert("Non-compliant token: transfer failed");
}
}
​
​

  • Issue: When the SablierFlow contract attempts to transfer tokens, the transaction reverts, causing a denial of service for users interacting with this token.

Test Case:

function testNonCompliantTokenFailure() public {
NonCompliantERC20 nonCompliantToken = new NonCompliantERC20();
nonCompliantToken.transfer(sender, 1e22); // Transfer tokens to sender for test
​
vm.startPrank(sender);
nonCompliantToken.approve(address(sablierFlow), type(uint256).max);
try sablierFlow.create(sender, recipient, UD21x18.wrap(1e18), IERC20(address(nonCompliantToken)), true) {
revert("Expected failure with non-compliant token");
} catch {}
vm.stopPrank();
}
​
​

  • Expected Result: The create function call reverts due to the non-compliant token's behavior, leading to transaction failure.

2. Malicious ERC20 Token Triggering Unauthorized Withdrawal

contract MaliciousERC20 is ERC20 {
SablierFlow public targetContract;
uint256 public streamId;
address public testAddress;
​
constructor(address _targetContract) ERC20("Malicious Token", "MLT") {
targetContract = SablierFlow(_targetContract);
_mint(msg.sender, 1e24); // Mint 1 million tokens to deployer
}
​
function setTestParams(uint256 _streamId, address _testAddress) external {
streamId = _streamId;
testAddress = _testAddress;
}
​
function transfer(address, uint256) public override returns (bool) {
// Attempt reentrant call to withdraw tokens
try targetContract.withdraw(streamId, testAddress, 1e18) {
return true;
} catch {
revert("Malicious transfer triggered withdrawal attempt");
}
}
}
​
​

  • Issue: The malicious token attempts to exploit the transfer function to perform unauthorized withdrawals via reentrancy.

Test Case:

function testMaliciousTokenWithdrawalFailure() public {
MaliciousERC20 maliciousToken = new MaliciousERC20(address(sablierFlow));
​
// Set parameters for malicious behavior
maliciousToken.setTestParams(streamId, address(this));
maliciousToken.transfer(sender, 1e22); // Transfer tokens to sender
​
// Create a stream with a standard token
vm.startPrank(sender);
streamId = sablierFlow.create(sender, recipient, UD21x18.wrap(1e18), IERC20(address(token)), true);
sablierFlow.deposit(streamId, 1e19, sender, recipient);
​
// Attempt malicious transfer
try maliciousToken.transfer(address(0x3), 1e18) {
revert("Expected malicious transfer to trigger withdrawal failure");
} catch Error(string memory reason) {
emit log_string(reason);
assertEq(reason, "Malicious transfer triggered withdrawal attempt");
}
vm.stopPrank();
}
​
​

  • Expected Result: The reentrant call is detected, and the transaction reverts to prevent unauthorized withdrawal.

3. Handling Potential Overflows with High Rates

function testOverflowsWithHighRates() public {
vm.startPrank(sender);
streamId = sablierFlow.create(sender, recipient, UD21x18.wrap(type(uint128).max), IERC20(address(token)), true);
​
// Attempting a deposit with max rate to test overflow safety
try sablierFlow.deposit(streamId, type(uint128).max, sender, recipient) {
revert("Expected overflow handling on max rate deposit");
} catch {}
​
// Verify no funds were incorrectly transferred
uint128 finalBalance = uint128(token.balanceOf(recipient));
assertEq(finalBalance, 0);
vm.stopPrank();
}
​
​

  • Issue: Testing for integer overflows when using extremely high rates. The contract should handle such scenarios gracefully without compromising funds.

Root Cause

  • Lack of Safe Token Interaction: The contract does not use safe methods to interact with ERC20 tokens, failing to handle cases where tokens do not return true or revert unexpectedly.

  • Absence of Reentrancy Protection: Critical functions like withdraw are not protected against reentrant calls, making them vulnerable to malicious tokens.

  • Assumption of ERC20 Compliance: The contract assumes all tokens strictly follow the ERC20 standard, which is not always the case.

Impact

  • Denial of Service: Users cannot interact with the SablierFlow contract using non-compliant tokens, disrupting service.

  • Security Risks: Malicious tokens can exploit the contract's assumptions to perform unauthorized actions, such as withdrawing funds.

  • Loss of Funds: Unauthorized withdrawals or locked funds due to failed transactions can result in financial losses.

  • Reputational Damage: Users may lose trust in the platform due to these vulnerabilities.

Tools Used

  • Foundry: A smart contract development and testing framework.

  • Solidity Compiler: Version 0.8.22.

  • OpenZeppelin Contracts: Standard implementations of ERC20 and security utilities.

  • Forge Test Suite: For writing and executing comprehensive test cases.

Recommendations

  1. Use SafeERC20 Library

    Utilize OpenZeppelin's SafeERC20 library to handle token interactions safely. It properly checks return values and reverts if calls fail.

    import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
    using SafeERC20 for IERC20;
    ​
    // Example usage in the contract
    function _transferTokens(address token, address to, uint256 amount) internal {
    IERC20(token).safeTransfer(to, amount);
    }
    ​
    ​

  2. Implement Reentrancy Guards

    Add reentrancy protection to functions that modify state and interact with external contracts, such as withdraw, deposit, and create.

    import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
    ​
    contract SablierFlow is ReentrancyGuard {
    // ...
    ​
    function withdraw(uint256 streamId, address recipient, uint256 amount) external nonReentrant {
    // Withdrawal logic
    }
    }
    ​
    ​

  3. Validate Token Compliance

    • Token Whitelisting: Maintain a whitelist of approved tokens that are known to be ERC20-compliant.

    • Interface Checks: Before interacting with a token, perform checks to ensure it implements required functions correctly.

  4. Check Return Values and Handle Failures

    Ensure that all external calls to tokens verify the success of the operation and handle any failures appropriately.

    function transferTokens(address token, address to, uint256 amount) internal {
    bool success = IERC20(token).transfer(to, amount);
    require(success, "Token transfer failed");
    }
    ​
    ​

  5. Limit External Calls in Critical Functions

    Avoid making external calls within functions that are sensitive to reentrancy, or ensure that state changes occur before external calls.

  6. Comprehensive Testing

    • Include Edge Cases: Test the contract with tokens that have unusual behaviors, such as non-standard return values or reversion on transfers.

    • Simulate Attacks: Write tests that attempt common attack vectors, like reentrancy and overflow attacks.

  7. User Education and Documentation

    • Inform Users: Clearly communicate to users which tokens are supported and the risks of using non-compliant tokens.

    • Provide Guidelines: Offer guidance on verifying token compliance before attempting to use them with the contract.

  8. Audit and Security Reviews

    • Regular Audits: Have the contract periodically audited by reputable security firms.

    • Community Review: Encourage community developers to review the code and report potential issues.

By implementing these recommendations, the SablierFlow contract can enhance its security posture, ensure reliable operation with compliant tokens, and protect against vulnerabilities introduced by non-compliant or malicious ERC20 tokens.

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.22;
​
import "forge-std/src/Test.sol";
import "../src/SablierFlow.sol";
import "./mocks/ERC20Mock.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "./mocks/MockFlowNFTDescriptor.sol";
​
// Non-Compliant ERC20 Token Contract
contract NonCompliantERC20 is ERC20 {
constructor() ERC20("Non-Compliant Token", "NCT") {
_mint(msg.sender, 1e24); // Mint tokens to deployer
}
​
// Override transfer, transferFrom, and approve to always revert
function transfer(address, uint256) public pure override returns (bool) {
revert("Non-compliant token: transfer failed");
}
​
function transferFrom(address, address, uint256) public pure override returns (bool) {
revert("Non-compliant token: transferFrom failed");
}
​
function approve(address, uint256) public pure override returns (bool) {
revert("Non-compliant token: approve failed");
}
}
​
// Test Contract for Non-Compliant ERC20 Tokens with SablierFlow
contract SablierFlowNonCompliantTokenTest is Test {
SablierFlow public sablierFlow;
NonCompliantERC20 public nonCompliantToken;
MockFlowNFTDescriptor public nftDescriptor;
address public sender;
address public recipient;
​
event TestStarted();
​
function setUp() public {
emit TestStarted();
​
// Deploy necessary contracts
nftDescriptor = new MockFlowNFTDescriptor();
sablierFlow = new SablierFlow(address(this), IFlowNFTDescriptor(address(nftDescriptor)));
emit log_named_address("SablierFlow deployed at", address(sablierFlow));
​
nonCompliantToken = new NonCompliantERC20();
emit log_named_address("NonCompliantERC20 deployed at", address(nonCompliantToken));
​
sender = address(0x123);
recipient = address(0x456);
​
// Set balance directly for the `sender`
uint256 balance = 1e22;
vm.store(
address(nonCompliantToken),
keccak256(abi.encode(sender, uint256(0))), // Calculate the storage slot for balance
bytes32(balance)
);
​
// Verify that the balance was correctly set
uint256 actualBalance = nonCompliantToken.balanceOf(sender);
emit log_named_uint("Mocked sender's token balance", actualBalance);
assertEq(actualBalance, balance, "Sender balance should be set to 1e22");
}
​
function testNonCompliantTokenFailure() public {
emit log("=== Starting testNonCompliantTokenFailure ===");
​
// Begin impersonating the sender
vm.startPrank(sender);
emit log("Impersonating sender");
​
// Attempt to approve should fail
vm.expectRevert("Non-compliant token: approve failed");
nonCompliantToken.approve(address(sablierFlow), 1e18);
​
// Modify the storage to simulate an approval (since approve always reverts)
// Calculate the storage slot for allowances[sender][sablierFlow]
bytes32 allowanceSlot = keccak256(
abi.encode(
address(sablierFlow), // spender
keccak256(abi.encode(sender, uint256(1))) // owner and slot 1 (allowances mapping)
)
);
vm.store(address(nonCompliantToken), allowanceSlot, bytes32(uint256(1e18)));
​
// Now the create call should fail due to transferFrom failure
vm.expectRevert("Non-compliant token: transferFrom failed");
sablierFlow.create(
sender,
recipient,
UD21x18.wrap(1e18), // Wrap 1e18 into UD21x18
IERC20(address(nonCompliantToken)),
true
);
​
vm.stopPrank();
emit log("=== Completed testNonCompliantTokenFailure ===");
}
}
​
​
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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