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

Ignoring ERC-20 transfer return value in RebateFiHook::withdrawTokens function allows silent failed withdrawals (tokens permanently stuck)

ghufranhassan1

Root + Impact

Description

  • The function should check the return value of ERC-20 transfers and revert if the transfer fails, ensuring successful transfers or clear failure notifications.

  • The function ignores the boolean return value from ERC-20 transfer() calls, allowing withdrawals to silently fail while still emitting success events, which can permanently trap tokens in the contract.

function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
@> IERC20(token).transfer(to, amount);// no return value
emit TokensWithdrawn(to, token , amount); // Ordering of events not correct
}

Risk

Likelihood:

  • Because withdrawTokens does not inspect the return value, Solidity considers the external call succeeded (no revert) even though the token contract signaled failure. The caller (the owner) will assume tokens were transferred out while the token balance remains in the hook, making future withdrawals of that token impossible and causing permanent loss of withdraw capability for that token.


Impact:

  • There is no way for owner to know that the withdraw failed

  • Events indicate successful withdrawal when it actually failed


Proof of Concept

Add following mock contract below the test file RebateFiHookTest.t.sol

​
​
// Mock token that returns false on transfer (simulates blacklisted address or paused token)
contract MockFailingToken {
mapping(address => uint256) public balanceOf;
function transfer(address to, uint256 amount) external returns (bool) {
// Always return false to simulate transfer failure
return false;
}
// For setup - mint tokens to addresses
function mint(address to, uint256 amount) external {
balanceOf[to] += amount;
}
// Allow the test contract to transfer tokens to hook for setup
function transfer(address to, uint256 amount) external {
require(balanceOf[msg.sender] >= amount, "Insufficient balance");
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
}
}
​
​

Add the below function in the test file RebateFiHookTest.t.sol and then run forge test --match-test test_WithdrawTokens_SilentFailure_Vulnerability -vvv

function test_WithdrawTokens_SilentFailure_Vulnerability() public {
// Deploy a mock token that always fails transfers
MockFailingToken failingToken = new MockFailingToken();
// Setup: Send some tokens to the hook contract
failingToken.mint(address(rebateHook), 1 ether);
uint256 hookBalanceBefore = failingToken.balanceOf(address(rebateHook));
// The vulnerability: withdrawTokens will ignore the transfer return value
// Even though transfer returns false, the function continues and emits success event
// Use vm.expectEmit to check that the event is emitted despite transfer failure
vm.expectEmit(true, true, true, true);
emit TokensWithdrawn(address(failingToken), address(this), 0.5 ether);
// This should revert due to transfer failure, but doesn't due to the vulnerability
rebateHook.withdrawTokens(address(failingToken), address(this), 0.5 ether);
// PROOF OF VULNERABILITY: Tokens are still stuck in the hook contract
uint256 hookBalanceAfter = failingToken.balanceOf(address(rebateHook));
assertEq(hookBalanceAfter, hookBalanceBefore, "Tokens should still be stuck in hook despite 'successful' withdrawal");
// The event was emitted making it appear the withdrawal succeeded, but tokens are permanently stuck
}



Recommended Mitigation

This can be improved by adding the return value that either success or transfer failed and also by modifying the event emission.

- remove this code
+ add this code
​
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
bool success = IERC20(token).transfer(to, amount);
+ require(success, "Transfer failed");
- emit TokensWithdrawn(to, token , amount); // Fixed parameter order
+ emit TokensWithdrawn(token, to, amount);
}
Updates

Lead Judging Commences

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