Use of _transfer in RaiseBoxFaucet::claimFaucetTokens risks sending ERC20 tokens to non-ERC20-compliant contracts, leading to permanent loss of tokens

Description

• The contract uses OpenZeppelin's ERC20::_transfer function to send tokens to users in the claimFaucetTokens function.

  function claimFaucetTokens() public {

  ...

  @> _transfer(address(this), faucetClaimer, faucetDrip);

  ...

  }

• The _transfer function does not check whether the recipient address is a contract or an externally owned account (EOA). If the recipient is a contract that is not compatible with the ERC20 standard interface, the tokens will be sent to the contract address but will be stuck forever, since it doesn't have the functionality to move the tokens from the contract, and thus, leading to permanent loss of tokens.

• There's a really good write-up on this issue by Immunefi: Introduction to ERC Token Standards — Part 1

Risk

Likelihood: High

• While many users interact with smart contracts through EOAs, there is a significant number of users who interact with contracts through other smart contracts (e.g., multisig wallets, DeFi protocols). If such a contract is not ERC20-compliant, it could lead to loss of tokens.

Impact: High

• This vulnerability can lead to permanent loss of tokens, and users won't be able to transfer/recover them anyway.

• Moreover, this particular issue with the ERC20 standard has been known for a long time and has caused a significant amount of loss in the past. Here's a glimpse of it: ERC-20 Losses Calculator, taken from this Medium article: Trader loses $26M of ezETH tokens, media says “user mistake” — hacker says ERC20 standard is insecure

Proof of Concept

• In order to demonstrate this issue, we need a simple non-ERC20-compliant contract:

  contract SimpleWallet {

  // This contract is non-compliant with the ERC20 standard

  address public owner;

  ​

  constructor() {

  owner = msg.sender;

  }

  ​

  // Function to claim tokens

  function claimTokens(address raiseBox) public {

  RaiseBoxFaucet(raiseBox).claimFaucetTokens();

  }

  }

• Here's how the issue unfolds:

  1. User creates SimpleWallet contract to manage their funds.

  2. SimpleWallet::claimTokens function is called, which in turn calls RaiseBoxFaucet::claimFaucetTokens to claim tokens.

  3. RaiseBoxFaucet contract sends tokens to the SimpleWallet contract address using _transfer.

  4. Since SimpleWallet does not implement the ERC20 standard, the tokens are sent to the contract address but are stuck there, leading to permanent loss of tokens.

Recommended Mitigation

ERC20 standard has been insecure for a long time; it's just that many protocols use it because of its simplicity and wide adoption. However, if a protocol wants to be more secure, it should consider other alternatives (also mentioned in the above two articles): ERC223, ERC677, and ERC1363 as they provide a better way to handle token transfers, similar to how NFTs (ERC721) get handled by onERC721Received.