Submission Details
Severity:
Missing Return Value Check for ERC20 Transfer
Root + Impact
Description
-
Describe the normal behavior in one or more sentences
-
Explain the specific issue or problem in one or more sentences
The `withdrawTokens` function does not check the return value of the ERC20 `transfer` call, which could fail silently for non-standard ERC20 tokens that don't revert on failure.
**Description**:
* The normal behavior expects ERC20 transfers to either revert on failure (ERC20 standard) or return a boolean indicating success (some implementations).
* The specific issue is that while most ERC20 tokens revert on failure, some implementations return `false` instead. Not checking the return value means failed transfers might go unnoticed, though this is less common in modern implementations.
**Root cause in the codebase**:
```solidity
// @> src/RebateFiHook.sol:74
IERC20(token).transfer(to, amount);
```
The return value is not checked. While OpenZeppelin's ERC20 reverts on failure, the interface `IERC20` doesn't guarantee this behavior.
Risk
Likelihood:
-
* This occurs when interacting with non-standard ERC20 tokens that return false on failure
* Most modern tokens (OpenZeppelin-based) revert, making this less likely but still possible
Impact:
-
* Failed transfers might not be detected if the token returns false instead of reverting
* Owner might believe withdrawal succeeded when it actually failed
* Funds could appear withdrawn in events but remain in contract
Proof of Concept
The following demonstrates how non-standard ERC20 tokens can cause silent failures:
```solidity
// Non-standard ERC20 implementation that returns false on failure
contract NonStandardERC20 {
mapping(address => uint256) public balanceOf;
function transfer(address to, uint256 amount) external returns (bool) {
if (balanceOf[msg.sender] < amount) {
return false; // Returns false instead of reverting (non-standard)
}
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
return true;
}
}
// Scenario: Hook contract interacts with non-standard token
address nonStandardToken = address(new NonStandardERC20());
uint256 withdrawAmount = 100 ether;
// Current buggy implementation:
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
IERC20(token).transfer(to, amount); // Return value ignored!
emit TokensWithdrawn(token, to, amount);
}
// Execution flow:
// 1. Owner calls withdrawTokens(nonStandardToken, owner, 100 ether)
// 2. Contract has insufficient balance (e.g., only 50 ether)
// 3. transfer() is called, checks balance, returns false
// 4. Return value is NOT checked, execution continues
// 5. Event TokensWithdrawn(nonStandardToken, owner, 100 ether) is emitted
// 6. Function completes "successfully" from owner's perspective
// 7. But tokens were NOT transferred (transfer returned false)
// 8. Tokens remain in contract, owner believes withdrawal succeeded
// Impact:
// - Event log shows withdrawal occurred, but it didn't
// - Owner may think funds were withdrawn when they weren't
// - Accounting systems will have incorrect data
// - Funds remain locked in contract
```
**Step-by-step execution:**
1. Owner calls `withdrawTokens(nonStandardToken, owner, 100 ether)`
2. Contract has only 50 ether balance
3. `transfer()` is called, checks balance, returns `false`
4. Return value is ignored, execution continues
5. Event `TokensWithdrawn` is emitted with amount = 100 ether
6. Function completes without reverting
7. Owner believes withdrawal succeeded
8. Tokens remain in contract (transfer failed silently)
Recommended Mitigation
Use OpenZeppelin's `SafeERC20` library which handles both reverting and non-reverting tokens:
```diff
// src/RebateFiHook.sol:14
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
// src/RebateFiHook.sol:20
+ using SafeERC20 for IERC20;
// src/RebateFiHook.sol:73-75
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);
+ emit TokensWithdrawn(token, to, amount);
}
```
**Alternative approach (if SafeERC20 is not desired):**
```diff
// src/RebateFiHook.sol:48-50
+ error TransferFailed(address token, address to, uint256 amount);
// src/RebateFiHook.sol:73-75
function withdrawTokens(address token, address to, uint256 amount) external onlyOwner {
+ bool success = IERC20(token).transfer(to, amount);
+ if (!success) {
+ revert TransferFailed(token, to, amount);
+ }
- IERC20(token).transfer(to, amount);
- emit TokensWithdrawn(to, token , amount);
+ emit TokensWithdrawn(token, to, amount);
}
```
**Explanation:**
- `SafeERC20.safeTransfer()` handles both reverting and non-reverting ERC20 tokens
- For reverting tokens: reverts on failure (standard behavior)
- For non-reverting tokens: checks return value and reverts if false
- Ensures transfers always succeed or revert, never fail silently
- Recommended approach as it's battle-tested and handles edge cases
- Alternative approach manually checks return value but requires more code
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.