RebateFi Hook
First Flight #53
Beginner FriendlyDeFi
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Unchecked ERC20 Transfer Return Value in withdrawTokens

mostafapahlevani93

Description

The normal behavior should verify that token transfers succeed, as some ERC20 tokens return false on failure instead of reverting. The withdrawTokens() function should check the return value or use SafeERC20.

The withdrawTokens() function uses the raw ERC20 transfer() method without checking its return value. Some ERC20 tokens (like USDT, BNB, OMG, and others) return false on transfer failure instead of reverting. This means the owner could believe tokens were withdrawn successfully when they actually failed.

function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
IERC20(token).transfer(to, amount); // ❌ Return value not checked
emit TokensWithdrawn(to, token , amount);
}

Problematic Token Examples:

  • USDT on Ethereum mainnet (no return value - returns void)

  • BNB token (returns false on some failures)

  • OMG token (returns false on failures)

  • Many older ERC20 implementations

Risk

Likelihood:

  • Occurs when owner attempts to withdraw tokens that don't follow strict ERC20 standard

  • USDT is one of the most common tokens and doesn't return a value from transfer

  • Hook contract may accumulate various tokens from fees or airdrops

  • Owner may attempt to withdraw multiple token types including non-standard ones

  • Silent failures can occur without owner awareness

Impact:

  • Owner believes tokens were withdrawn but they remain in contract

  • Tokens become permanently locked in contract if withdrawal consistently fails

  • No way to recover stuck tokens if transfer silently fails

  • Loss of accumulated fees or tokens sent to contract

  • Protocol revenue cannot be extracted

  • Trust issues if owner reports successful withdrawal but tokens never arrive

  • Accounting discrepancies between expected and actual balances

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;
​
import "forge-std/Test.sol";
import "forge-std/console.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
// Mock token that returns false on transfer failure
contract FalseReturningToken {
mapping(address => uint256) public balances;
bool public shouldFail;
constructor() {
balances[address(this)] = 1000 ether;
}
function setShouldFail(bool _fail) external {
shouldFail = _fail;
}
function transfer(address to, uint256 amount) external returns (bool) {
if (shouldFail) {
return false; // Return false instead of reverting
}
balances[msg.sender] -= amount;
balances[to] += amount;
return true;
}
function balanceOf(address account) external view returns (uint256) {
return balances[account];
}
}
​
// Simplified version of RebateFiHook to demonstrate bug
contract VulnerableWithdraw {
address public owner;
constructor() {
owner = msg.sender;
}
modifier onlyOwner() {
require(msg.sender == owner, "Not owner");
_;
}
// Buggy implementation - doesn't check return value
function withdrawTokens_BUGGY(address token, address to, uint256 amount)
external
onlyOwner
returns (bool)
{
IERC20(token).transfer(to, amount); // No return value check
return true; // Always returns true
}
// Safe implementation using require
function withdrawTokens_SAFE_V1(address token, address to, uint256 amount)
external
onlyOwner
returns (bool)
{
require(IERC20(token).transfer(to, amount), "Transfer failed");
return true;
}
// Safe implementation using return value
function withdrawTokens_SAFE_V2(address token, address to, uint256 amount)
external
onlyOwner
returns (bool)
{
bool success = IERC20(token).transfer(to, amount);
require(success, "Transfer failed");
return true;
}
}
​
contract UncheckedTransferTest is Test {
FalseReturningToken token;
VulnerableWithdraw hook;
address owner = address(this);
address recipient = address(0xBEEF);
function setUp() public {
token = new FalseReturningToken();
hook = new VulnerableWithdraw();
// Transfer tokens to hook contract
token.transfer(address(hook), 100 ether);
}
function test_BuggyWithdraw_SilentFailure() public {
// Setup: Make token transfers fail
token.setShouldFail(true);
uint256 hookBalanceBefore = token.balanceOf(address(hook));
uint256 recipientBalanceBefore = token.balanceOf(recipient);
console.log("\n=== Before Withdrawal ===");
console.log("Hook balance:", hookBalanceBefore);
console.log("Recipient balance:", recipientBalanceBefore);
// Attempt withdrawal using buggy function
bool result = hook.withdrawTokens_BUGGY(address(token), recipient, 50 ether);
uint256 hookBalanceAfter = token.balanceOf(address(hook));
uint256 recipientBalanceAfter = token.balanceOf(recipient);
console.log("\n=== After Buggy Withdrawal ===");
console.log("Withdrawal returned:", result);
console.log("Hook balance:", hookBalanceAfter);
console.log("Recipient balance:", recipientBalanceAfter);
console.log("Tokens actually transferred:", recipientBalanceAfter - recipientBalanceBefore);
// Prove the bug: function returns true but transfer failed
assertEq(result, true, "Function returns success");
assertEq(hookBalanceAfter, hookBalanceBefore, "Hook balance unchanged");
assertEq(recipientBalanceAfter, recipientBalanceBefore, "Recipient received nothing");
console.log("\n❌ BUG CONFIRMED: Function returned true but no tokens transferred!");
}
function test_SafeWithdraw_V1_CatchesFailure() public {
token.setShouldFail(true);
console.log("\n=== Testing Safe V1 (require check) ===");
// Safe version should revert
vm.expectRevert("Transfer failed");
hook.withdrawTokens_SAFE_V1(address(token), recipient, 50 ether);
console.log("✅ Safe V1 correctly reverted on failed transfer");
}
function test_SafeWithdraw_V2_CatchesFailure() public {
token.setShouldFail(true);
console.log("\n=== Testing Safe V2 (return value check) ===");
// Safe version should revert
vm.expectRevert("Transfer failed");
hook.withdrawTokens_SAFE_V2(address(token), recipient, 50 ether);
console.log("✅ Safe V2 correctly reverted on failed transfer");
}
function test_SafeWithdraw_SucceedsWhenTransferWorks() public {
token.setShouldFail(false);
uint256 hookBalanceBefore = token.balanceOf(address(hook));
uint256 recipientBalanceBefore = token.balanceOf(recipient);
console.log("\n=== Testing Safe Withdrawal With Working Token ===");
console.log("Hook balance before:", hookBalanceBefore);
console.log("Recipient balance before:", recipientBalanceBefore);
bool result = hook.withdrawTokens_SAFE_V1(address(token), recipient, 50 ether);
uint256 hookBalanceAfter = token.balanceOf(address(hook));
uint256 recipientBalanceAfter = token.balanceOf(recipient);
console.log("Hook balance after:", hookBalanceAfter);
console.log("Recipient balance after:", recipientBalanceAfter);
assertEq(result, true, "Function returns success");
assertEq(hookBalanceAfter, hookBalanceBefore - 50 ether, "Hook balance decreased");
assertEq(recipientBalanceAfter, recipientBalanceBefore + 50 ether, "Recipient received tokens");
console.log("✅ Safe withdrawal succeeded with working token");
}
function test_RealWorld_USDT_Scenario() public {
console.log("\n=== Real-World USDT Scenario ===");
console.log("USDT on Ethereum has no return value (returns void)");
console.log("Standard IERC20 interface expects bool return");
console.log("Calling transfer on USDT will cause issues with strict interfaces");
console.log("");
console.log("Vulnerable code: IERC20(token).transfer(to, amount);");
console.log("Problem: If token is USDT, this may fail silently or revert unexpectedly");
console.log("");
console.log("Solution: Use OpenZeppelin's SafeERC20 library");
console.log("SafeERC20.safeTransfer handles tokens with and without return values");
}
}

Running the PoC:

forge test --match-contract UncheckedTransferTest -vv

Expected Output:

[PASS] test_BuggyWithdraw_SilentFailure()
=== Before Withdrawal ===
Hook balance: 100000000000000000000
Recipient balance: 0
=== After Buggy Withdrawal ===
Withdrawal returned: true
Hook balance: 100000000000000000000
Recipient balance: 0
Tokens actually transferred: 0
❌ BUG CONFIRMED: Function returned true but no tokens transferred!
​
[PASS] test_SafeWithdraw_V1_CatchesFailure()
=== Testing Safe V1 (require check) ===
✅ Safe V1 correctly reverted on failed transfer
​
[PASS] test_SafeWithdraw_V2_CatchesFailure()
=== Testing Safe V2 (return value check) ===
✅ Safe V2 correctly reverted on failed transfer

Recommended Mitigation

Use OpenZeppelin's SafeERC20 (Recommended)

+ 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(to, token, amount);
}
}

Add tests for:

  1. Withdrawal of standard ERC20 tokens

  2. Withdrawal of tokens that return false on failure

  3. Withdrawal of tokens with no return value (USDT-style)

  4. Withdrawal with insufficient balance

  5. Withdrawal to zero address (should fail)

Updates

Lead Judging Commences

chaossr Lead Judge 8 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Not using safe transfer for ERC20.

Support

FAQs

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

Give us feedback!