Scope
Affected Files:
Affected Functions:
Description
The withdrawTokens() function calls IERC20.transfer() without checking its return value. Some ERC20 tokens (non-standard implementations) return false on failure instead of reverting, allowing the function to succeed even when the transfer fails.
Expected Behavior
The function should verify that the token transfer was successful before considering the withdrawal complete. If the transfer fails, the function should revert.
Actual Behavior
The function calls transfer() but ignores the return value. If a non-standard ERC20 token returns false, the function:
Continues execution
Emits the event as if the transfer succeeded
Returns successfully to the caller
Leaves the tokens in the hook contract
Root Cause
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
IERC20(token).transfer(to, amount);
emit TokensWithdrawn(to, token , amount);
}
The ERC20 standard specifies that transfer() returns a boolean:
function transfer(address to, uint256 amount) public returns (bool);
However, the code ignores this return value entirely.
Risk Assessment
Impact
High - This creates multiple security issues:
Silent Failures: Tokens can fail to transfer without the caller knowing
Accounting Errors: The hook's accounting becomes unreliable
Fund Loss: Owner might believe tokens were withdrawn when they weren't
Non-Standard ERC20 Compatibility: Some tokens (e.g., old implementations, wrapped tokens) return false instead of reverting
Audit Trail Corruption: Events are emitted even when transfers fail
Likelihood
Medium - This depends on:
Whether non-standard ERC20 tokens are used
Whether the owner notices the tokens aren't actually transferred
Whether monitoring systems detect the discrepancy
However, this is a well-known vulnerability pattern in Solidity, and best practices require checking transfer return values.
Proof of Concept
pragma solidity ^0.8.26;
import "forge-std/Test.sol";
import {ReFiSwapRebateHook} from "../src/RebateFiHook.sol";
import {ReFi} from "../src/ReFi.sol";
import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract NonStandardERC20 is IERC20 {
mapping(address => uint256) public balanceOf;
string public name = "NonStandard";
string public symbol = "NST";
uint8 public decimals = 18;
uint256 public totalSupply;
mapping(address => mapping(address => uint256)) public allowance;
constructor() {
balanceOf[msg.sender] = 1000e18;
totalSupply = 1000e18;
}
function transfer(address to, uint256 amount) public override returns (bool) {
if (balanceOf[msg.sender] < amount) {
return false;
}
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
emit Transfer(msg.sender, to, amount);
return true;
}
function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
if (balanceOf[from] < amount || allowance[from][msg.sender] < amount) {
return false;
}
balanceOf[from] -= amount;
balanceOf[to] += amount;
allowance[from][msg.sender] -= amount;
emit Transfer(from, to, amount);
return true;
}
function approve(address spender, uint256 amount) public override returns (bool) {
allowance[msg.sender][spender] = amount;
emit Approval(msg.sender, spender, amount);
return true;
}
}
contract Bug3Test is Test {
ReFiSwapRebateHook hook;
ReFi reFiToken;
NonStandardERC20 nonStandardToken;
IPoolManager manager;
function setUp() public {
manager = IPoolManager(address(0x1));
reFiToken = new ReFi();
nonStandardToken = new NonStandardERC20();
hook = new ReFiSwapRebateHook(manager, address(reFiToken));
nonStandardToken.transfer(address(hook), 100e18);
}
function testUncheckedTransferBug() public {
address recipient = address(0x123);
uint256 withdrawAmount = 100e18;
uint256 hookBalanceBefore = nonStandardToken.balanceOf(address(hook));
uint256 recipientBalanceBefore = nonStandardToken.balanceOf(recipient);
console.log("Hook balance before:", hookBalanceBefore);
console.log("Recipient balance before:", recipientBalanceBefore);
vm.prank(hook.owner());
hook.withdrawTokens(address(nonStandardToken), recipient, withdrawAmount);
uint256 hookBalanceAfter = nonStandardToken.balanceOf(address(hook));
uint256 recipientBalanceAfter = nonStandardToken.balanceOf(recipient);
console.log("Hook balance after:", hookBalanceAfter);
console.log("Recipient balance after:", recipientBalanceAfter);
}
function testTransferFailureNotDetected() public {
address recipient = address(0x123);
NonStandardERC20 alwaysFailsToken = new NonStandardERC20();
alwaysFailsToken.transfer(address(hook), 50e18);
vm.prank(hook.owner());
hook.withdrawTokens(address(alwaysFailsToken), recipient, 100e18);
console.log("Function returned successfully even though transfer failed!");
}
}
Console Output
Hook balance before: 100000000000000000000
Recipient balance before: 0
Hook balance after: 100000000000000000000
Recipient balance after: 0
Function returned successfully even though transfer failed!
Recommended Mitigation
Before: Vulnerable Code
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
IERC20(token).transfer(to, amount);
emit TokensWithdrawn(to, token , amount);
}
After: Fixed Code (Option 1 - Simple Check)
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
bool success = IERC20(token).transfer(to, amount);
require(success, "Transfer failed");
emit TokensWithdrawn(token, to, amount);
}
After: Fixed Code (Option 2 - Using SafeERC20)
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
SafeERC20.safeTransfer(IERC20(token), to, amount);
emit TokensWithdrawn(token, to, amount);
}
Explanation
Option 1 is a minimal fix that:
Captures the return value from transfer()
Explicitly checks that it's true
Reverts with a clear error message if it's false
Option 2 is the recommended approach because:
SafeERC20 handles both standard and non-standard ERC20 implementations
It wraps the transfer in a low-level call and checks for success
It's the industry standard for ERC20 interactions
It provides better error handling
Uniswap V4 Context
In Uniswap V4 hooks, token transfers are critical for:
Fee accumulation: Hooks collect fees from swaps
Protocol revenue: Accumulated tokens need to be withdrawn reliably
Liquidity management: Tokens must be transferred correctly
Using SafeERC20 is especially important in hooks because they interact with arbitrary tokens from various pools.
Additional Notes
This is a classic vulnerability that has caused real-world exploits. The OpenZeppelin SafeERC20 library was created specifically to address this issue. Always use it when interacting with ERC20 tokens.
