Check tx.origin in the functions where the _value is taken from the approval

[rjkz808]

🎉 Description

So, in the transferFrom, burnFrom or other functions in the ERC20 token smart-contracts we need to check the tx.origin balance

• 🐛 This is a bug report.
• 📈 This is a feature request.

💻 Environment

Next, we need to know what your environment looks like.

• Which version of OpenZeppelin are you using?

• openzeppelin-solidity ^1.12.0

• What network are you deploying to? Ganache? Ropsten?

• I'm using Ganache, but you can deploy to any existing network

• How are you deploying your OpenZeppelin-backed contracts? truffle? Remix? Let us know!

• You can use either Truffle or Remix

📝 Details

If you want for example add a refund to the allowance crowdsale, you cannot normally do this, before the msg.sender approves the crowdsale contract to spend all it's tokens. When you call the transfer the msg.sender always will be the address(this).

🔢 Code To Reproduce Issue [ Good To Have ]

Just try to write something like this

contract ExampleCrowdsale {
  using SafeERC20 for ERC20

  // I've skipped a part of crowdsale

  function buyTokens(address _beneficiary) public payable {
    require(_beneficiary != address(0));
    require(msg.value > 0);
    require(hasStarted() && !hasEnded());
    require(weiRaised.add(msg.value) <= cap);
    require(_getTokenAmountWithBonus(msg.value) <= saleAmount);

    weiRaised = weiRaised.add(msg.value);
    token.safeTransferFrom(
      owner,
      _beneficiary,
      _getTokenAmountWithBonus(msg.value)
    );

    emit TokenPurchase(
      msg.sender, 
      _beneficiary, 
      msg.value, 
      _getTokenAmountWithBonus(msg.value)
    );
  }
}

then add the refund function

function refund() public {
  require(!goalReached() && isFinalized);
  msg.sender.transfer(paidAmountOf[msg.sender]);
  token.safeTransfer(address(this), token.balanceOf(msg.sender));
}

but it will always revert. We can do something like

function refund() public {
  require(!goalReached() && isFinalized);
  require(token.allowance(msg.sender, address(this) == token.balanceOf(msg.sender)));
  msg.sender.transfer(paidAmountOf[msg.sender]);
  token.safeTransferFrom(msg.sender, address(this), token.balanceOf(msg.sender));
}

but it requires one more transaction from the user.

👍 Other Information

Solution

You need to add 4 lines to the transferFrom and other ERC20 functions where the value is taken from the approval

function transferFrom(
  address _from,
  address _to,
  uint256 _value
)
  public
  returns (bool)
{
  require(_value <= balances[_from]);
  require(_to != address(0));
  
  if (_from != tx.origin) {
    require(_value <= allowed[_from][msg.sender]);
    allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
  }
  balances[_from] = balances[_from].sub(_value);
  balances[_to] = balances[_to].add(_value);
  emit Transfer(_from, _to, _value);
  return true;
}

[frangio]

Hi @rjkz808! Thanks for the suggestion.

This is clearly a deviation from ERC20 semantics. I don't think a function such as transferFrom should be altered in this way.

But more importantly, usage of tx.origin is often a security anti-pattern, and it is the case here. With such a token, any transaction initiated by a token holder can result in an arbitrary contract taking tokens from their balance. So not only would the crowdsale be able to transferFrom the token holder, but any contract. For example, if the token holder makes an ERC721 transfer, this can trigger an onERC721Received function on the receiver, and the receiver can then steal ERC20 tokens from the sender.

I'm closing the issue but I'd be glad to continue the discussion.

[barakman]

@rjkz808:

Read bullet number 6 in this answer on ethereum.stackexchange.