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

ERC20 `transfer` may not revert on failed transaction so tokens may be locked in a contract forever

valya

Root + Impact

Description

  • The RebateFiHook contract does not specify eligible ERC20 tokens assuming that any ERC20 token may be used as a pool token. There are weird ERC20 tokens that will not revert on failed transfer, but will return false instead. Some tokens (e.g. Tether Gold) will return false even when transfer was succesful.

  • There is no check of returned value for ERC20 token transfer in RebateFiHook::withdrawTokens function. Even if the check was there, it is known that some tokens (e.g. Tether Gold) will return false even when transfer was succesful.

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

Risk

Likelihood:

  • The issue will occur when attempting to withdraw ERC20 tokens that return false on transfer or do not return any bool by design.

Impact:

  • Attempt to transfer ERC20 tokens with missing return values will revert even though a transfer itself was succesful.

  • Attempt to transfer ERC20 tokens with returned false will not revert RebateFiHook::withdrawTokens function and the TokensWithdrawn event will be emitted but no withdrawal occurred.

  • These weird tokens will be locked in the RebateFiHook contract.

Proof of Concept

Here is a test attempting to withdraw USDT and MockERC20:

  1. USDT is missing return bool, so ERC20's transfer will fail even when the transfer was succesful, i.e. the ERC20's Transfer event was emitted. This occurs because ERC20 interface expects a bool return value from USDT transfer function.

  2. MockERC20 token returns false on ERC20 transfer but RebateFiHook::withdrawTokens does not revert and emits TokensWithdrawn event, pretending that withdraw was succesful. The balances of RebateFiHook and owner stay the same after withdrawal, i.e to transfer occured.

Add the following test TestReFiSwapRebateHook::test_WithdrawWeirdTokenand and MockERC20FailsOnTransfer to the TestReFiSwapRebateHook.sol and run it on mainnet fork:

​
event Transfer(address indexed from, address indexed to, uint256 amount);
event TokensWithdrawn(address indexed token, address indexed to, uint256 amount);
​
function test_WithdrawWeirdToken() public {
///////////////////////////
////// USDT withdraw //////
///////////////////////////
address usdt = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
uint256 usdtTransferAmount = 1000 * 1e6;
uint256 usdtWithdrawAmount = usdtTransferAmount / 2;
deal(usdt, address(rebateHook), usdtTransferAmount);
​
vm.expectRevert();
vm.expectEmit();
emit IERC20.Transfer(address(rebateHook), address(this), usdtWithdrawAmount);
​
rebateHook.withdrawTokens(usdt, address(this), usdtWithdrawAmount);
​
////////////////////////////////////////
/// MockERC20 with false on transfer ///
////////////////////////////////////////
MockERC20FailsOnTransfer mockFailOnTransferToken = new MockERC20FailsOnTransfer("Weird", "W");
uint256 dealAmount = 1 ether;
uint256 withdrawAmount = dealAmount / 2;
​
deal(address(mockFailOnTransferToken), address(rebateHook), dealAmount);
uint256 hookBalanceOfWeirdBefore = IERC20(address(mockFailOnTransferToken)).balanceOf(address(rebateHook));
uint256 ownerBalanceOfWeirdBefore = IERC20(address(mockFailOnTransferToken)).balanceOf(address(this));
​
vm.expectEmit();
emit TokensWithdrawn(address(this), address(mockFailOnTransferToken), withdrawAmount);
rebateHook.withdrawTokens(address(mockFailOnTransferToken), address(this), withdrawAmount);
​
uint256 hookBalanceOfWeirdAfter = IERC20(address(mockFailOnTransferToken)).balanceOf(address(rebateHook));
uint256 ownerBalanceOfWeirdAfter = IERC20(address(mockFailOnTransferToken)).balanceOf(address(this));
​
assertEq(hookBalanceOfWeirdBefore, hookBalanceOfWeirdAfter);
assertEq(ownerBalanceOfWeirdBefore, ownerBalanceOfWeirdAfter);
}
​
contract MockERC20FailsOnTransfer is ERC20 {
constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) {}
​
function transfer(address, uint256) public pure override returns (bool) {
return false;
}
}

Recommended Mitigation

Use a safe transfer library like OpenZeppelin SafeERC20 to ensure consistent handling of ERC20 return values and abstract over inconsistent ERC20 implementations.

+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!