Don't make `ERC2771Context` `_trustedForwarder` `immutable`

[SKYBITDev3]

🧐 Motivation The article Arbitrary Address Spoofing Attack: ERC2771Context Multicall Public Disclosure published 3d ago says:

Some custom ERC2771Context implementations allow setting a trusted forwarder. Doing so can prevent any gasless transaction from being executed, limiting any possible exploit.

There was some discussion about this at https://forum.openzeppelin.com/t/in-erc2771context-trustedforwarder-shouldnt-be-immutable/37523

📝 Details Replace https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6ba452dea4258afe77726293435f10baf2bed265/contracts/metatx/ERC2771Context.sol#L22-L23

and https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/dd26c99a2f66793ac22f3b5f3ef006b2f05af9de/contracts/metatx/ERC2771ContextUpgradeable.sol#L23-L24

with

address private _trustedForwarder;

[Amxx]

Hello @SKYBITDev3

The forwarder is set to be immutable for two reasons:

• loading the value from storage (as you propose) is significantly more espensive. It cost 2000 gas to "warm up" the slot, and 100 gas per access. Adding +2K gas to all ERC20 / ERC721 transfers is not something we feel like doing.
• moving this value to storage's only value is the ability to update it. This brings advantages (in the case of a bug) but also risks. Moreover, it bring the question of governance. Having the ability to change that value is a tremendous responsability, and developpers should not take this lightly.

If a developper wants, its easy for them to create a new Context contract and use it in their codebase.

abstract contract ERC2771ContextStorage is ERC2771Context(address(0)) {
    address private _trustedForwarderStorage;

    constructor(address initialTrustedForwarder) {
        _trustedForwarderStorage = initialTrustedForwarder;
    }

    function trustedForwarder() public view virtual override returns (address) {
        return _trustedForwarderStorage;
    }

    // some custom administration logic.
}

We are not providing this version because we don't want to create confusion has to which one should be used. We also feel like having an admin that can change the value is to big of a risk to have. In light of the recent events you may disagree.

Our goal is to remove Context at some point. We did not do that in 5.0 because usage of ERC2771 is still very much a thing, particularly on L2s and sidechains. Hopefully Solidity will provide us with a better way to override msg.sender in the future.

[SKYBITDev3]

having an admin that can change the value is to big of a risk to have. In light of the recent events you may disagree.

Such superuser risks are nothing new, as the same can be said about upgradeable contracts which have been around for a while and have been increasingly used.

The Contracts Wizard generates this code when Roles and UUPS upgradeability are selected:

function _authorizeUpgrade(address newImplementation)
        internal
        onlyRole(UPGRADER_ROLE)
        override
    {}

Access Control used with multisig mitigates the risks, as multiple people need to agree for a sensitive function to execute.

So similarly, a function that updates the forwarder address can be protected with an admin role:

    function setTrustedForwarder(address _forwarder) public onlyRole(ADMIN_ROLE) {

... and that role (strongly) should be granted exclusively to a multisig (and the role revoked from the initial admin).

The TimelockController reduces the risks even further as it adds a protective delay before execution, so that action could be taken if it was found that the call was malicious.

So with these security options there shouldn't be so much worry about removing immutable from _trustedForwarder.

You could add notes that advise granting roles exclusively to a multisig in order to protect sensitive functions e.g. that perform an upgrade or update the forwarder address, withdraw tokens from the contract, etc.

Gas savings pale in significance to security issues, with the ability to easily change the forwarder when needed being much more valuable like what has happened over the past week.

[Amxx]

Gas savings pale in significance to security issues

People blame us for every single unit of gas that can be saved. Up to the point that we stopped emitting some (very usefull) events that are not strictly required by ERC. Maybe doing the change you propose would get some positive feedback in the short term ... and in 3 month time people will want us to change again to save gas.

---

One more thing is that such a change would be breaking. If you upgrade from an implementation that uses an immutable variable, to one that use storage, storage will be set to 0 and you'll need a reinitializer. We have a strong policy of not doing such breaking change in minor versions ... and I suspect by the next major we will remove Context altogether.

So again, our only real option would be having 2 versions, which we don't want to do for the reason stated above.

If we were to provide a storage based version, it would probably use a mapping, to that multiple forwarder can be supported in parallele.

[Amxx]

@ernestognw Do you think we should have a storage based version of ERC2771Context ?

[ernestognw]

@ernestognw Do you think we should have a storage based version of ERC2771Context ?

No. But I think we can add a virtual getter for it.

We always kept it immutable given the risks of enabling a new forwarder arbitrarily and the gas overhead of loading the forwarder address from storage. The intention was that a contract will always have only 1 forwarder and if that's secure, then the ERC2771Context implementer will remain secure.

Turned out the Multicall mistake made ERC2771Context insecure, but it doesn't have anything to do with the forwarder. In my opinion, the original assumption of 1 immutable forwarder per contract remains valid (and its benefits). But I do recognize the need of customizing it (or revoke it).

[SKYBITDev3]

1 immutable forwarder per contract remains valid (and its benefits). But I do recognize the need of customizing it (or revoke it)

So how can the forwarder address be customized or revoked when needed if it's immutable?

[ernestognw]

So how can the forwarder address be customized or revoked when needed if it's immutable?

Adding a virtual function

contract ERC2771Forwarder {
  address private immutable _trustedForwarder;

  ...

  function trustedForwarder() public view virtual returns (address) {
    return _trustedForwarder;
  }
}

This is non-breaking and allows for customization after an upgrade.

[SKYBITDev3]

OK, though how about for non-upgradeable contracts? e.g.:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/metatx/ERC2771Context.sol";

contract MyToken is ERC20, AccessControl, ERC2771Context {
    constructor(address adminAddress, address forwarderAddress) ERC20("MyToken", "MTK")  ERC2771Context(forwarderAddress) {
        _grantRole(DEFAULT_ADMIN_ROLE, adminAddress);
    }

    function _contextSuffixLength()
        internal
        view
        override(Context, ERC2771Context)
        returns (uint256)
    {
        return super._contextSuffixLength();
    }

    function _msgSender()
        internal
        view
        override(Context, ERC2771Context)
        returns (address)
    {
        return super._msgSender();
    }

    function _msgData()
        internal
        view
        override(Context, ERC2771Context)
        returns (bytes calldata)
    {
        return super._msgData();
    }

    function setTrustedForwarder(
        address _forwarder
    ) public onlyRole(DEFAULT_ADMIN_ROLE) {
        _setTrustedForwarder(_forwarder);
    }
}

[ernestognw]

There's no _setTrustedForwarder and an immutable forwarder is a good guarantee in my opinion. If the trustedForwarder getter is virtual, then anyone can extend it to use an internal _setTrustedForwarder setter, but I don't think that should go in OpenZeppelin Contracts