Submission Details
Severity:
Unsafe ERC20 Transfer Without Return Value Check
Root + Impact
The withdrawTokens function uses IERC20.transfer() without checking the return value, which can silently fail for non-standard tokens that return false instead of reverting.
Description
-
Some ERC20 tokens (e.g., USDT on some chains) don't revert on failed transfers but return
false. -
The current implementation ignores the return value, potentially leaving tokens in the contract when withdrawal appears successful.
// Root cause in RebateFiHook.sol line 74
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
@> IERC20(token).transfer(to, amount); // @> Return value not checked
emit TokensWithdrawn(to, token, amount);
}
Risk
Likelihood:
-
Occurs when withdrawing non-standard ERC20 tokens
-
USDT and other major tokens exhibit this behavior
Impact:
-
Tokens may remain stuck in contract while event indicates successful withdrawal
-
Owner believes withdrawal succeeded when it failed
-
Requires re-attempting withdrawal, causing confusion
Proof of Concept
The following test demonstrates the vulnerability with non-standard ERC20 tokens. When a token returns false instead of reverting on failed transfer, the withdrawTokens function continues execution and emits a success event, even though no tokens were actually transferred.
// SPDX-License-Identifier: MIT
import {Test, console} from "forge-std/Test.sol";
import {ReFiSwapRebateHook} from "../src/RebateFiHook.sol";
import {Deployers} from "@uniswap/v4-core/test/utils/Deployers.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
// Mock non-standard ERC20 that returns false instead of reverting
contract NonStandardToken is IERC20 {
mapping(address => uint256) public balances;
function mint(address to, uint256 amount) external {
balances[to] += amount;
}
function transfer(address to, uint256 amount) external returns (bool) {
if (balances[msg.sender] < amount) {
return false; // Returns false instead of reverting!
}
balances[msg.sender] -= amount;
balances[to] += amount;
return true;
}
function balanceOf(address account) external view returns (uint256) {
return balances[account];
}
// Other IERC20 functions (simplified)
function totalSupply() external pure returns (uint256) { return 0; }
function allowance(address, address) external pure returns (uint256) { return 0; }
function approve(address, uint256) external pure returns (bool) { return true; }
function transferFrom(address, address, uint256) external pure returns (bool) { return true; }
}
contract UnsafeTransferPoCTest is Test, Deployers {
ReFiSwapRebateHook public hook;
NonStandardToken public badToken;
address owner;
address recipient = address(0xBEEF);
event TokensWithdrawn(address indexed token, address indexed to, uint256 amount);
function setUp() public {
deployFreshManagerAndRouters();
// Deploy with a mock ReFi token address
hook = new ReFiSwapRebateHook(manager, address(0x1234));
owner = hook.owner();
badToken = new NonStandardToken();
// Mint tokens to hook - but NOT enough for the withdrawal
badToken.mint(address(hook), 50 ether);
}
function test_PoC_SilentFailureWithNonStandardToken() public {
uint256 hookBalanceBefore = badToken.balanceOf(address(hook));
uint256 recipientBalanceBefore = badToken.balanceOf(recipient);
console.log("=== Before Withdrawal ===");
console.log("Hook balance:", hookBalanceBefore / 1e18, "tokens");
console.log("Recipient balance:", recipientBalanceBefore / 1e18, "tokens");
// Try to withdraw MORE than hook has
uint256 withdrawAmount = 100 ether; // Hook only has 50!
vm.prank(owner);
// Event is emitted despite transfer failing!
vm.expectEmit(true, true, false, true);
emit TokensWithdrawn(recipient, address(badToken), withdrawAmount);
// This should revert but doesn't!
hook.withdrawTokens(address(badToken), recipient, withdrawAmount);
uint256 hookBalanceAfter = badToken.balanceOf(address(hook));
uint256 recipientBalanceAfter = badToken.balanceOf(recipient);
console.log("");
console.log("=== After 'Withdrawal' ===");
console.log("Hook balance:", hookBalanceAfter / 1e18, "tokens");
console.log("Recipient balance:", recipientBalanceAfter / 1e18, "tokens");
console.log("");
console.log("BUG: Event emitted but NO tokens moved!");
console.log("Owner thinks withdrawal succeeded, but it silently failed.");
// Balances unchanged - transfer silently failed
assertEq(hookBalanceAfter, hookBalanceBefore, "Hook balance unchanged");
assertEq(recipientBalanceAfter, recipientBalanceBefore, "Recipient got nothing");
}
function test_PoC_USDTLikeBehavior() public {
// USDT on some chains doesn't revert, just returns false
// This demonstrates the same vulnerability pattern
console.log("=== USDT-like Token Behavior ===");
console.log("1. Hook has 50 tokens");
console.log("2. Owner calls withdrawTokens(badToken, recipient, 100)");
console.log("3. badToken.transfer() returns false (insufficient balance)");
console.log("4. withdrawTokens() ignores return value");
console.log("5. TokensWithdrawn event emitted");
console.log("6. Owner sees 'success' but recipient has 0 tokens");
console.log("");
console.log("With SafeERC20.safeTransfer(), this would revert.");
}
}
Recommended Mitigation
Use OpenZeppelin's SafeERC20 library:
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
contract ReFiSwapRebateHook is BaseHook, Ownable {
+ using SafeERC20 for IERC20;
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
- IERC20(token).transfer(to, amount);
+ IERC20(token).safeTransfer(to, amount);
emit TokensWithdrawn(token, to, amount);
}
}
Updates
Lead Judging Commences
chaossr 12 days ago
Submission Judgement Published Assigned finding tags:
Not using safe transfer for ERC20.
Rewards breakdown
nSLOC
150This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.