transferFrom calls public virtual allowance function breaking overridden behaviour

[NeoXtreem]

PR #3170 introduced a breaking change which is also a bug in that transferFrom() now calls (via _spendAllowance()) the overridable virtual function allowance() instead of accessing _allowances directly.

This is problematic where concrete implementations of ERC20 or ERC777 override virtual functions to adjust amounts so that the amounts exposed in the derived contract are a function of the amounts stored in the base contracts, and vice versa.

💻 Environment

@openzeppelin/contracts-upgradeable 4.5.1
Truffle

📝 Details

The general design of the ERC20 and ERC777 base contracts follows the pattern that public functions are virtual, with the default implementation that they call the corresponding internal virtual function. For example, approve() calls _approve(), and burn() calls _burn(). Other functions in the base contract use the internal implementations of these functions so that the behaviour is deterministic. Concrete implementations may choose to override the internal function to change the internal behaviour. But they may also choose to override the public function so that internal behaviour is unaffected.

This design is now broken by calling the public function allowance() in the transformFrom() function.

Given the thin nature of the allowance() function, the proper approach would be to follow a similar pattern to the public virtual balance() function and its underlying _balances variable. All reads of _balances are done directly. No function calls balance(). Similarly, no function should call allowance(). All reads of _allowances should be done directly.

🔢 Code to reproduce bug

    function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
        return super.transfer(recipient, f_(amount));
    }

    function allowance(address holder, address spender) public view virtual override returns (uint256) {
        return f(super.allowance(holder, spender));
    }

    function approve(address spender, uint256 value) public virtual override returns (bool) {
        return super.approve(spender, f_(value));
    }

    function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
        return super.transferFrom(sender, recipient, f_(amount));
    }

f() is a function that modifies the amount passed to it and returns the result. f_() is the complement of that function.

[Amxx]

Hello @NeoXtreem

This is something we used to have, and that we specifically changed to allow creative use of overrides.

IMO, the value returned by allowance() is what users (and other systems) expect to be the value used by transferFrom. This contract, for example uses this mechanism to allow a privileged address to perform token transfers.

I'm concerned by the fact that, in the code the give, if f: x → 2*x and f_: x → x/2, then calling transfer(target, value) will perform a transfer of value while doing transferFrom(sender, receiver, value) will perform a transfer of value/2

Maybe you would override the transfer function, but then there might be an even better approach for you:

    function allowance(address holder, address spender) public view virtual override returns (uint256) {
        return f(super.allowance(holder, spender));
    }

    function _approve(address owner, address spender, uint256 value) internal virtual override {
        super._approve(owner, spender, f_(value));
    }
    
    // If ERC20
    function _transfer(address from, address to, address spender, uint256 value) internal virtual override {
        super._transfer(from, to f_(value));
    }

    // If ERC777
    function _send(
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    ) internal virtual {
        super._send(from, to, f_(amount), userData, operatorData, requireReceptionAck);
    }

I'm still worried by the inconsistency you'd get between the Approval events and the Transfer events, but tat is another matter

IMO, no function but _approve() and allowance() should read/write to _allowances, and all other functions should go by there two.

[NeoXtreem]

Hi @Amxx,

I omitted transfer() from my example code for brevity as the issue concerns allowance(), but I've edited it now so it's there. However, I should've mentioned that my example concerns ERC777, so overriding _transfer() is not a solution, and _move() is not virtual.

Your solution also does not work as it doesn't change the behaviour of the concrete implementation which is already calling _approve() as it's for the purpose of using transferFrom() to transfer tokens to/from addresses other than msg.sender. See code below:

function _foo(address from, address to, uint256 amount) internal {
    _approve(from, _msgSender(), amount); // Would need to be super._approve if your override is used
    super.transferFrom(from, to, amount);
}

By mandating that _spendAllowance() should call allowance() instead of directly accessing _allowances, not only are you introducing a breaking change in a non-major update (this is against standard practices), you are also introducing a third and conflicting design pattern into the base class:

• Design pattern 1 is that the public virtual function has a corresponding internal virtual function (e.g., approve()/_approve() and burn()/_burn()) so that the concrete implementation may choose which behaviour to override.
• Design pattern 2 is where the public virtual function is thin, and there is no corresponding internal virtual function. Any internal behaviour simply explicitly executes the same code as the public virtual function such as accessing the variable (e.g., balance()).
• You have now introduced a third design pattern where there is a public virtual function, and internal behaviour calls that function leaving concrete implementations in a situation where they have no choice other than to ensure internal behaviour and public behaviour are the same.

This is why I made the changes I did in in #3212. It complies with pattern 2. If you want to achieve the possibility to do what the example contract you referenced does, then that is possible with design pattern 1, and very easy to support in the base class by simply adding an _allowance() internal virtual function which allowance() calls. Then all the example contract needs to do is override _allowance().

So, I don't think there's room for opinion here. The design should remain consistent, otherwise you are potentially breaking any smart contract that transforms amounts between the base contract and the public interface on the derived contract. And you are also limiting the ability for concrete implementations to choose how to override behaviour unlike all the other virtual functions in the base class that follow design pattern 1 above. Your change basically breaks SoC design principle.

If it helps, my contract is this one which is the base class for this one. You can see the relevant code in allowance(), approve(), and transferFrom().

_getStandardAmount() is like f(), and _getReflectedAmount() is like f_(). But transferFrom() results in a call to _transferFromReflected() which calls _approve() and super.transferFrom(). This should just be expected to work, but your change now results in a call to my overridden allowance() function which calls f() on the amount after that amount had just had f_() called on it. And it stands to reason that allowance() should indeed be overridden and call f() before passing the result to super.allowance() because that amount needs to be transformed per the design around f() and f_() in the concrete implementation.

IMO, no function but _approve() and allowance() should read/write to _allowances, and all other functions should go by there two.

In short, if this statement were true, then you should also say the same for _balances which is read/written by _mint(), _burn(), transfer() and transferFrom() (via _move()), and _send(), all virtual functions. This is design pattern 2 above. Excluding your new function (_spendTokens()) from this design breaks the overall design and also any contracts upgrading to it, not to mention inhibiting use of the base contract (as described above). _spendTokens() is part of transferFrom(); therefore, access to _allowances should follow the same design pattern as _balances which is accessed directly in _move().

I sincerely hope this clarifies the change, or leans you towards me adding _allowance(), otherwise I fear it could be detrimental to future use of transferFrom functionality under ERC-20 token standard where people want to use OpenZeppelin Contracts to separate internal behaviour and public behaviour using transform functions on the virtual overrides.

Thanks,
Jason

PS: Re the inconsistency on Approval and Transfer events, I think you mean between each of those events and the amounts passed into the functions. Yes, that has caused problems which is why I also call the Transfer event again in the derived contract (I should also do that for Approval in the next upgrade). I wish there was a way to suppress the events in the base contract. But that's indeed another matter. 🙂
PPS: Your suggested code needs to remove the return keyword on the _approve() override.

[Amxx]

I omitted transfer() from my example code for brevity as the issue concerns allowance(), but I've edited it now so it's there. However, I should've mentioned that my example concerns ERC777, so overriding _transfer() is not a solution, and _move() is not virtual.

I'll read the rest of your message and try when my mind is clear, because I think answering your question first requires me to fully understand your usecase. Still I wanted to point that, on ERC777, you could override

function _send(
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    ) internal virtual {

which would be the equivalent to overriding _transfer() in ERC20

[Amxx]

function _foo(address from, address to, uint256 amount) internal {
_approve(from, _msgSender(), amount); // Would need to be super._approve if your override is used
super.transferFrom(from, to, amount);
}

I don't get the purpose of this function, regardless of the super call ... This is bypassing any missing approval to do a transfer ... so why not just do _send() ?

[Amxx]

I had a quick look at the SchnoodleV8Base and I find it confusing.

In particular, you override transfer() (and transferFrom) by doing
uint256 reflectedAmount = _getReflectedAmount(amount);

But then you also override _send to do the same thing.

Maybe you missed that transfer is internally calling _send, so that would be applying the _getReflectedAmount function twice. Is that really what you want.

I still think that _transferFromReflected should just be a call to either _send(), or super._send().
Edit: in fact there is already _sendReflected ... why have a _transferFromReflected ? line 94 looks like you are asking for trouble.

Unrelated but still wanted to share:

• you _mint override is not really useful. Maybe you want to handle the totalSupply += in here and not have to deal with that in the constructor ?
• I'm confused by the MAX - (MAX % totalSupply()) ... you may want to document that.
• since your contract is already deeply customized, I believe that it would be cleaner to inherit ERC777Upgradeable instead of ERC777PresetFixedSupplyUpgradeable and doing the initialization work here (its just one line)

[NeoXtreem]

@Amxx I think you've completely missed the mark here. Neither transfer() nor transferFrom() calls _send() which voids most of your last 3 messages. The Schnoodle smart contract has been in production for 7 months and there hasn't been a single issue with token amounts. And it is fully unit tested. This unit test tests transfer(), this unit test tests transferFrom(), and this unit test tests send(). This variable (deltaRate) factors in the fee (feeRate) to ensure the contract is behaving as expected which is asserted here. If you run those unit tests, they succeed, which, based on the code in _testTransfer(), proves that it is impossible for _getReflectedAmount() to have been called twice which I hope is obvious now that you understand that _send() is not called as you thought it was.

I gather from your comments that you may not be familiar with the EIP-777 token standard, so please allow me to explain. It essentially adds separate functionality that prevents transfers to contract addresses unless they're registered on the ERC-1820 registry on the respective chain. That involves the send() function which was so named to avoid confusion with the transfer() and transferFrom() functions which are there merely for ERC-20 backward-compatibility. Therefore, line 94 is absolutely not "asking for trouble". If you follow where this is actually called (via a callback in payFund()), you'll see that it is only ever used for an internal transfer to _eleemosynaryAccount to effectuate a fee back from the recipient after a transfer to the recipient. This of course requires an internal approval each time.

So, my concern here is that you are sidestepping the real issue which is that you have introduced a breaking change that I have offered a fix for, and you are opposing it with incorrect assertions that are in many ways unrelated anyway.

Please could you revise this again based on the correction of your misunderstanding of _send()? And I mean that with all respect. It's not a problem you missed this, but it does reset this discussion back to my reply yesterday effectively.

I would also like @frangio to look at this as he reviewed your change.

In response to your other comments:

• Yes, the _mint() override is benign, and that's a good idea about handling _totalSupply in here.
• MAX - (MAX % totalSupply() is instrumental to the reflective algorithm, and is what gives the reflected TS stored in the base contract the maximum possible value it can have as a function of the actual TS.
• I understand that the presets are now deprecated, and I would indeed use the Contracts Wizard to generate a contract now, but this code is already in production, so the inheritance stack can't be changed easily without causing storage layout complications. You can see I recently used the wizard to generate this contract which is not yet released. That said, ERC777PresetFixedSupplyUpgradeable does the job and there's no part of it that isn't utilised. It offers a fixed supply, upgradeability, and ERC-777 functionality, all of which are used. Without it, I would have to call _mint() myself in the derived contract. It's a thin use case, and ERC777PresetFixedSupplyUpgradeable is a very thin wrapper class, but it's a line of code saved nonetheless, and demonstrates that the fixed supply functionality is provided by a trusted and known base contract.

[Amxx]

I think you've completely missed the mark here. Neither transfer() nor transferFrom() calls _send() which voids most of your last 3 messages

Are we looking at the same code?

• your transferFrom calls super.transferFrom
• that goes to ERC777.transferFrom which calls _send (EDIT: my bad, this is in master, but not yet released :/)
• you override _send, so that is what gets called

When you point out the change to the allowance read/write, it seems you also missed a lot of the other recent changes we made to ERC777. They were made hoping it would help design contract like yours, but since you already did that for a previous variation of the contracts, the migration is indeed non-trivial.

Our main objective is to provide external consistency in the behavior of interfaces, but we still change some internal code to achieve what we consider improvement ... and we do that by following community requests. It as always been clear that overriding function is a complex thing that require a good understanding of the code being overridden. (particularly) In that case it is encouraged to freeze your dependency, and to check the changelog for changes, and to run your unit tests when upgrading.

There hasn't been any security issues with ERC777 over the past year, so if the changes me made to it are not of your liking, you should probably stick with an old version of it. Your seem knowledgeable to cherry-pick the changes that you like, and not take the one you don't like. At that point you include so many customization that it might make sens to have your own version anyway.

[frangio]

not only are you introducing a breaking change in a non-major update (this is against standard practices)

I want to assure you that we are very mindful of breaking changes. We strongly avoid them, except for situations where after a lot of consideration we decide to do them because we feel the benefits outweigh the costs.

When it comes to virtual functions, however, we are forced to have weaker stability guarantees. It is extremely hard to make any change without introducing potential breakage for users who define overrides. We could have a very restricted set of virtual functions as extension points for users who wish to customize and ensure their backwards compatibility, but experience has shown us that these are never enough, developers want to customize things we have no way of foreseeing, and after a long time we decided that simply every function would be virtual.

This has the side effect that what would otherwise be implementation details are exposed as a kind of interface. But if we want to continue improving the code when we find ways to do so, we need to be able to change implementation details, otherwise the code becomes ossified and can never be improved (or we do major versions every month, which is not good for users anyway).

This is why ever since we introduced virtual everywhere we have explained the following (#2154 (comment)):

the responsibility to ensure correctness when functions are overriden will be on the users and not on the OpenZeppelin maintainers. People who wish to override should consult the source code to fully understand the impact it will have, and should consider whether they need to override additional functions to achieve the desired behavior.

Admittedly, we should feature this warning in the documentation, and we should additionally make it explicit that we cannot guarantee lack of breaking changes when functions are overriden. When a developers updates the dependency from one version to the next, they need to look at the changes to understand if they will have an impact on the custom overrides.

This is where we are now. You have detected that the new implementation breaks your previous assumptions, and it's good that you reported it, because it lets us know of the creative ways in which users are extending the contracts, and we can consider the use case and see if there is something we should be doing differently.

We disagree with the premise that this is a breaking change that we have to roll back. What you're trying to achieve may be doable with the latest version, maybe not in the same way you were doing it. If it's not, and if we want to enable your use case, we should try to do so in a way that also supports the use case we had in mind when we made this change in the first place.

[frangio]

I liked the approach suggested by @Amxx but the issue you pointed out in transferFrom is correct. But it is possible to patch that by also overriding _spendAllowance. The simplest patch is to do

    function allowance(address holder, address spender) public view override returns (uint256) {
        return f(super.allowance(holder, spender));
    }

    function _approve(address holder, address spender, uint256 value) internal override {
        return super._approve(holder, spender, f_(value));
    }

    function _spendAllowance(address owner, address spender, uint256 amount) internal override {
        super._spendAllowance(owner, spender, f(amount));
    }
    
    function _send(
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    ) internal override {
        return super._send(from, to, f_(amount), userData, operatorData, requireReceptionAck);
    }

Alternatively, you can copy the implementation of _spendAllowance and insert super. This is probably better since you wouldn't be applying f_ and f successively.

    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        uint256 currentAllowance = super.allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC777: insufficient allowance");
            unchecked {
                super._approve(owner, spender, currentAllowance - amount);
            }
        }
    }

[NeoXtreem]

@frangio Thanks for your replies. Much appreciated.

I'm happy to say your patch works. Couple of things, though:

• Remove return from the _approve() override (I guess this was just a typo by @Amxx).
• There is no _transfer() to override (I guess this is unreleased?), so I must still override transfer() and transferFrom() as I was already doing.

I still find it odd that the public virtual allowance() is called in the base contract's _spendAllowance() function when the internal virtual version of all other functions are being called everywhere else (e.g., _approve(), _burn(), etc.). And where such an internal virtual version of a function doesn't exist, which is only ever the accessing of a variable, that variable is directly accessed instead (e.g., _balances, _totalSupply).

By following a different design pattern for _spendAllowance(), it's now inconsistent, and perverts the concrete design - i.e., I now have to use f() on an amount passed into the superclass function just because it will compare the amount to a value returned by the overridden allowance() function that applies f_(). It makes the concrete design confusing and inconsistent unless the reader investigates what is actually happening in the base _spendAllowance() function. In fact, LSP is broken by this design as the behaviour is different for my concrete contract overriding allowance() compared to another derived contract that might not, or that does not apply a transform to the amount.

I still think it would've been far better to add an internal virtual _allowance() function (consistent with _approve(), _burn(), etc.), or accepted my fix which is consistent with accesses of _balances and _totalSupply.

... we should additionally make it explicit that we cannot guarantee lack of breaking changes when functions are overriden. When a developers updates the dependency from one version to the next, they need to look at the changes to understand if they will have an impact on the custom overrides.

Extending a class is normal practice for users of your library. If you have virtual functions, then you should expect they will be overridden, and breaking changes should not be made on a non-major upgrade unless absolutely necessary (to fix a bug or security hole). If you wish to encapsulate behaviour that should not be overridden while still allowing some functionality to be overridden, then provide interfaces for this (ISP). Generally, code should follow OCP, which means code will not change short of a refactor. I understand this is more difficult in Solidity, but then at least adhere to LSP so that base contract behaviour doesn't change if a user decides to override a public function. At least give them a choice to change the behaviour or not by offering them public virtual and internal virtual functions like you do with many of the other functions in the base contract.

In any case, thank you both for responding and assisting on this. I hope we all got something out of it. 🙂

[frangio]

There is no _transfer() to override (I guess this is unreleased?), so I must still override transfer() and transferFrom() as I was already doing.

I was suggesting to override _send, because the version in master actually implements transfer and transferFrom by calling _send after a recent refactor that didn't make it onto the last release. I guess this will be another breaking change from your perspective when you update to the next minor.

breaking changes should not be made on a non-major upgrade unless absolutely necessary

We agree on this in principle, but as I tried to explain earlier, following this discipline strictly would leave us with an ossified codebase, never improved, and this is not something we think would benefit users. There are some virtual functions that are meant to be overriden (e.g. _beforeTokenTransfer), and for these we can guarantee backwards compatibility, but for all the other virtual functions (which are literally all functions) we simply can't guarantee that overrides will not break in minor updates due to otherwise simple refactors.

All of this said, we will consider the topics mentioned in this discussion going forward. Perhaps we should establish a guideline that internal functions should not call public functions.

[NeoXtreem]

@frangio Your earlier reply indicated your suggested code was aggregate to @Amxx's suggested code which included a _transfer() override. But I understand now. And I was already overriding _send() as @Amxx already noticed I was doing in his earlier reply, but which he thought was confusing because I also override transfer() and transferFrom(), before he realised that this is not an issue in the currently released version. You can see that override here. So, once the next release comes out with send() being called from transfer() and transferFrom(), it shouldn't be too much of an issue - I'll just remove the transfer() and transferFrom() overrides.

breaking changes should not be made on a non-major upgrade unless absolutely necessary

We agree on this in principle, but as I tried to explain earlier, following this discipline strictly would leave us with an ossified codebase, never improved, and this is not something we think would benefit users. There are some virtual functions that are meant to be overriden (e.g. _beforeTokenTransfer), and for these we can guarantee backwards compatibility, but for all the other virtual functions (which are literally all functions) we simply can't guarantee that overrides will not break in minor updates due to otherwise simple refactors.

I understand what you're saying here, but that's why I suggested adding _allowance() like you have with other functions that have public virtual and internal virtual implementations. This would solve the above problem (by using a pattern you're already using!), and your code would be fully compliant with both ISP and LSP. You effectively give the user a choice whether to change the behaviour or not by offering them public virtual and internal virtual functions in all cases. This way, you can introduce breaking changes without affecting the behaviour of concrete derivations. That's pretty much how your code has been up to now. Like I keep saying, _allowance() would've solved all of this, and still allowed you to make the functional change - adding _spendAllowance().

All of this said, we will consider the topics mentioned in this discussion going forward. Perhaps we should establish a guideline that internal functions should not call public functions.

I think that guideline would be perfect, and that would then force you to add the _allowance() function I suggested, or similar.