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

Unsafe token transfer in withdrawTokens

s3mvl4d

Description

  • Administrative token withdrawals should be performed using safe transfer patterns that revert on failure across both standard and non‑standard ERC‑20 implementations. This ensures that the contract does not silently continue after a failed transfer (e.g., tokens that return false instead of reverting).

  • The withdrawTokens function uses a raw IERC20(token).transfer(to, amount) and ignores the return value. For ERC‑20 tokens that return false (instead of reverting) or have non‑standard behavior, this can result in silent failure—no tokens moved while the contract emits a success event and continues execution.

​
// Root cause in the codebase with @> marks to highlight the relevant section
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
@> IERC20(token).transfer(to, amount); // <-- raw transfer; return value not checked
emit TokensWithdrawn(to, token , amount);
}
​

Risk

Likelihood: Low

  • Arises during routine treasury operations whenever the hook holds and withdraws ERC‑20 tokens that are non‑standard or malleable (e.g., tokens that return false on failure, require additional checks, or implement fee‑on‑transfer semantics).

  • Common in heterogeneous DeFi environments where third‑party tokens may not strictly adhere to the ERC‑20 spec.

Impact: Medium

  • Silent loss of funds movement / accounting mismatch: The contract may emit TokensWithdrawn and proceed as if the transfer succeeded, while the recipient receives no tokens. Off‑chain accounting and monitoring are then corrupted.

  • Operational confusion & incident risk: Treasury scripts and dashboards relying on events will show successful withdrawals, complicating incident response and reconciliation.

Proof of Concept

  • A conceptual test illustrating silent failure with a mock token that returns false (rather than reverting):

​
contract NonStandardERC20 is IERC20 {
// ...minimal impl...
function transfer(address to, uint256 amount) external returns (bool) {
// Simulate failure without revert:
return false; // <-- caller must check return value
}
}
​
function test_WithdrawTokens_SilentFailure_With_NonStandardToken() public {
NonStandardERC20 bad = new NonStandardERC20();
// Send some tokens to the hook (balance bookkeeping is internal to mock)
// Attempt withdrawal
vm.expectEmit(true, true, true, true);
emit ReFiSwapRebateHook.TokensWithdrawn(address(bad), user1, 1 ether);
​
// Under current implementation, call does NOT revert even though transfer returns false
rebateHook.withdrawTokens(address(bad), user1, 1 ether);
​
// Off-chain sees a successful event, but user1 did not receive tokens.
// A safe transfer should have reverted instead.
}
​

Recommended Mitigation

  • Use SafeERC20.safeTransfer (OpenZeppelin) to revert on failure and prevent silent misbehavior. Also fix the event parameter order while you’re here.

​
+ 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);
- emit TokensWithdrawn(to, token , amount);
+ IERC20(token).safeTransfer(to, amount);
+ 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!