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

withdrawTokens ignores ERC20 transfer return value, causing silent failures and incorrect event emission for non-reverting tokens

efimxff

withdrawTokens ignores transfer return value, causing silent failures and incorrect event emission for non-reverting tokens

Description

  • Describe the normal behavior in one or more sentences

  • Explain the specific issue or problem in one or more sentences

function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
@> IERC20(token).transfer(to, amount);
emit TokensWithdrawn(to, token , amount);
}

Risk

Likelihood:

  • Occurs when withdrawing non-reverting ERC20 tokens

Owner performs routine token withdrawals/recovery. Transfer fails (insufficient balance, paused token, etc.) but function succeeds

Impact:

  • Tokens remain stuck in contract despite successful function execution, TokensWithdrawn event misleads off-chain systems and users

  • Owner may not realize withdrawal failed until checking balances manually

  • Repeated attempts waste gas without recovering tokens

Proof of Concept

This test demonstrates how withdrawTokens silently fails when using non-reverting ERC20 tokens. A mock token is created that returns false instead of reverting on transfer failure, simulating real-world tokens with non-standard behavior. When the owner attempts to withdraw tokens, the function completes successfully and emits the TokensWithdrawn event, but the actual transfer fails. The tokens remain stuck in the hook contract while the owner believes the withdrawal succeeded based on the emitted event and successful transaction.

function test_WithdrawTokens_SilentFailureWithNonRevertingToken() public {
// Deploy a mock non-reverting token (like USDT)
MockNonRevertingERC20 fakeToken = new MockNonRevertingERC20(
"Fake",
"FAKE",
18
);
​
// Give hook some tokens
fakeToken.mint(address(rebateHook), 1000 ether);
​
// Configure token to return false on transfer (simulating failure)
fakeToken.setShouldFail(true);
​
uint256 hookBalanceBefore = fakeToken.balanceOf(address(rebateHook));
address owner = address(this);
uint256 ownerBalanceBefore = fakeToken.balanceOf(owner);
​
// Expect the event to be emitted (even though transfer fails)
vm.expectEmit(true, true, true, true);
emit ReFiSwapRebateHook.TokensWithdrawn(
owner,
address(fakeToken),
100 ether
);
​
// Owner attempts withdrawal - succeeds despite transfer failure
rebateHook.withdrawTokens(address(fakeToken), owner, 100 ether);
​
// Tokens remain in hook contract (transfer failed silently)
assertEq(
fakeToken.balanceOf(address(rebateHook)),
hookBalanceBefore,
"Tokens stuck in contract"
);
assertEq(
fakeToken.balanceOf(owner),
ownerBalanceBefore,
"Owner received no tokens"
);
}
​
contract MockNonRevertingERC20 {
string public name;
string public symbol;
uint8 public decimals;
mapping(address => uint256) public balanceOf;
bool public shouldFail;
​
constructor(string memory _name, string memory _symbol, uint8 _decimals) {
name = _name;
symbol = _symbol;
decimals = _decimals;
}
​
function mint(address to, uint256 amount) external {
balanceOf[to] += amount;
}
​
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
}
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
return true;
}
}

Recommended Mitigation

Use OpenZeppelin's SafeERC20 library which provides safeTransfer() to handle both reverting and non-reverting ERC20 tokens correctly. The safeTransfer() function checks the return value and reverts if the transfer fails, ensuring tokens are actually transferred before emitting the event. This prevents silent failures where non-compliant tokens (like USDT) return false instead of reverting on failure, which would otherwise allow the function to complete successfully despite no tokens being transferred.

​
// .....
+ 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);
}
Updates

Lead Judging Commences

chaossr Lead Judge 12 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!