The ERC20 specification is one of the oldest token standards on Ethereum. When it was first created the creators themselves could not foresee all of the issues that would come from the standard being so loosely defined.
In smart contract auditing, you understand that due to these loose standards tons of bugs can occur when interfacing with ERC20. Protocols that are meant to work with ERC20 tokens can face a number of different issues that at first glance aren't that obvious.
Fee on Transfer Tokens
Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance. However, a malicious user could also take advantage of this to steal funds from the pool.
Let's take a look at the deposit() and withdraw() functions.
function deposit(uint _amount) external {
TKN.transferFrom(msg.sender, address(this), _amount);
updateFor(msg.sender);
balances[msg.sender] += _amount;
}
/// @notice withdraw tokens from stake
/// @param _amount the amount to withdraw
function withdraw(uint _amount) external {
updateFor(msg.sender);
balances[msg.sender] -= _amount;
TKN.transfer(msg.sender, _amount);
}
Let's look at the following example.
Alice sends 100 of FEE token to the contract when calling addToPool().
FEE token contract takes 10% of the tokens (10 FEE).
90 FEE tokens actually get deposit in contract.
_updatePoolBalance(poolId, pools[poolId].poolBalance + amount);will equal 100.Attacker then sends 100 FEE tokens to the contract
The contract now has 180 FEE tokens but each user has an accounting of 100 FEE.
The attacker then tries to redeem his collateral for the full amount 100 FEE tokens.
The contract will transfer 100 FEE tokens to Bob taking 10 of Alice's tokens with him.
Bob can then deposit back into the pool and repeat this until he drains all of Alice's funds.
When Alice attempts to withdraw the transaction will revert due to insufficient funds.
To mitigate against this attack measure the contract balance before and after the call to transfer()/transferFrom() and use the difference between the two as the amount, rather than the amount stated.
function deposit(uint _amount) external {
uint256 balanceBefore = IERC20(TKN).balanceOf(address(this));
TKN.transferFrom(msg.sender, address(this), _amount);
uint256 balanceAfter = IERC20(TKN).balanceOf(address(this));
uint256 amount = balanceBefore - balanceAfter;
updateFor(msg.sender);
balances[msg.sender] += amount;
}
Note this implementation is vulnerable to reentrancy so be sure to add a reentrancy guard to this function.
Rebasing Tokens
Rebasing tokens can really get really messy if not accounted for when writing Smart Contracts.
In the deposit function below users expect that when they deposit tokens to the contract, they get back all rewards earned, not just a flat rate.
With the contracts of this project, deposited tokens will grow in value, but the user will only return the pre-calculated amount set in the storage variable.
balances[msg.sender] -= _amount;
Amounts go solely to the owner/creator or will remain locked in the contract if no withdraw excess tokens function is added to the contract for the owner.
/// @notice withdraw tokens from stake
/// @param _amount the amount to withdraw
function deposit(uint _amount) external {
TKN.transferFrom(msg.sender, address(this), _amount);
updateFor(msg.sender);
balances[msg.sender] += _amount;
}
If rebasing tokens are used as the collateral token, rewards accrue to the contract and cannot be withdrawn by either the user or the owner, and remain locked forever.
To fix this the dev should provide a function for the pool owner to withdraw excess deposited tokens and repay any associated taxes. Or create a blocklist for rebasing tokens.
Funds can be lost if any participant is blacklisted
Popular tokens such as USDC may implement a blocklist of addresses that are not allowed to send or receive that token. This will cause a function to revert when attempting to transfer to one of these addresses.
If any of the participants is blacklisted (due to illegal activities, using Tornado Cash for privacy reasons, etc.), depending on the implementation this could lock funds in a contract indefinitely.
Review the following code.
function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
if (totalFee > tokenBalance) {
revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
}
s_state = State.Resolved;
emit Resolved(i_buyer, i_seller);
if (buyerAward > 0) {
i_tokenContract.safeTransfer(i_buyer, buyerAward);
}
if (i_arbiterFee > 0) {
i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
}
tokenBalance = i_tokenContract.balanceOf(address(this));
if (tokenBalance > 0) {
i_tokenContract.safeTransfer(i_seller, tokenBalance);
}
}
When a resolved dispute is called tokens are sent to the buyer, seller, and arbiter. However, if any of these tokens are only on the USDC blacklist the transaction will fail leaving the seller with none of their tokens being transferred and tokens getting locked in the contract. Considering USDC is a commonly used ERC20 for these types of services it's safe to assume that this could cause some tokens to get stuck in escrow.
Blacklisting is a standard practice used in popular tokens such as the stablecoin USDC. In this case, a participant with malicious intent could deliberately get their address blacklisted, thereby preventing others from withdrawing funds.
Solution to the Issue:
Individual Withdrawals:
- Instead of directly transferring funds to each address, maintain a state variable that tracks the amount each address is entitled to. Once the escrow concludes, introduce a withdrawal function. This allows participants to individually claim their funds. If one participant is blacklisted, it won't hinder others from retrieving their share.
Address Redirection:
- To address the challenge of blacklisted participants being unable to withdraw, introduce a function that lets participants designate an alternate withdrawal address. This way, even if their primary address is blacklisted, they can redirect and access their funds through another address.
Not Approving to Zero First Could Cause Token Transfer to Fail
As a result, ERC20 tokens like USDT, which don't return a boolean for the approve method, cannot be supported as payout tokens.
Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. For example, Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.
Instance - claimFees()
The following function must approve() by zero first, and then the approve() function can be called. Otherwise, the claimFees() function will revert every time it handles such kinds of tokens.
function claimFees() public {
address[] memory assets = bondNFT.getAssets();
for (uint i=0; i < assets.length; i++) {
uint balanceBefore = IERC20(assets[i]).balanceOf(address(this));
IFee(feetoken).claim(assets[i]);
uint balanceAfter = IERC20(assets[i]).balanceOf(address(this));
IERC20(assets[i]).approve(address(bondToken), type(uint256).max);// @audit this could fail always with some tokens,
bondToken.distribute(assets[i], balanceAfter - balanceBefore);
}
}
Recommendation: Switch to using safeApprove in place of the standard approve to ensure compatibility with all ERC20 tokens.