In ERC20 tranferFrom why _transfer before checking allowance?

In the vanilla ERC20 there is this:

function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);
        _approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amount, "ERC20: transfer amount exceeds allowance"));
        return true;
    }

Why transfer first and then check that msgSender is allowed to transfer? Should we not first check that and then transfer in order to save time and gas?

I guess my question follows from me not having a good model of the operational semantics of solidity. What I would have written, would be the following: 1) test and if success update allowances, 2) update _balances. Here it is done in the opposite order, which means that in the case _allowances[sender][_msgSender] fails 1) one has to undo the transfer, 2) and sender paid avoidable gas.

1 Like

Hi @1vd21,

Welcome to the community :wave:

Thanks for the question. The order of _transfer and _approve was discussed in the following issue:

Specifically:

In the case of ERC20 , the order is irrelevant because _transfer and _approve are independent and never call any other contract: they are executed atomically.

Let me know if you need more information.

My first idea was that this could indeed reduce gas usage of reverting transferFrom, with no consequence to successful call. And that, since it is a non-breaking change, it should be shipped for the sake of gas optimisation.

However

the _transfer of transferFrom can also revert, due to the balance’s safemath, if the senders balance is not enough. changing the order of these two operation would thus decrease the revert cost when approval is not enough, but increase it when balance is not enough. So the question is, which one is most likely, and should be optimized for.

While I think it’s a very bad thing, a lot of dapps ask their user to approve a maximum value (2**256-1) once and for all. After this one transaction, transferFrom can by called with virtually no restrictions. These transfers are thus more likelly going to fail due to insufficient balance, then insufficient approval.

The best thing to do would probably be to scan for all reverting erc20 transfers, and mesure the ratio of failures caused by approval against failures caused by balance… Which sounds like a lot of work a small gain.

If you really feel like things should be reordered for your contracts, you can use the fact that the transferFrom function is virtual, and overload it:

contract ERC20Optimised is ERC20 {
    function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
        ERC20._approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amount, "ERC20: transfer amount exceeds allowance"));
        ERC20._transfer(sender, recipient, amount);
        return true;
    }
}
3 Likes

Thanks for this very clear analysis. I completely agree. I did not know about that prevailing practice you mention where most dapps ask users for max allowance. Indeed if most fails are of the “balance” type rather than of the “allowance” type, it is socially optimal to test the balance first. And at the agent level, one could argue that having one spend more gas when one is trying to transfer funds -without being allowed- to is a welcome penalty.

2 Likes