In ERC2771Context _trustedForwarder shouldn't be immutable

Smart Contracts
1

See:

whereas in OpenGSN's version _trustedForwarder isn't immutable:

... and has the function _setTrustedForwarder to update it:

I think it'd be better to remove immutable so that we can have a function to set a new trusted forwarder

Why should a trusted forwarder be hardcoded? Like everything else in life, something or someone that was once trusted may one day no longer be trustworthy, so it should be possible to adapt to such changes.

So without immutable , I'd even make a public setTrustedForwarder function in my contract (and secure it with modifier onlyRole(DEFAULT_ADMIN_ROLE)).

2

I noticed in a previous version that _trustedForwarder was not immutable:

Instead of flip-flopping just leave it without immutable.

With the many problems that it has caused and may cause in future, minimizing gas cost isn't a good reason to make it immutable.

3

First of all, because a setter function would constitute a centralized aspect of the contract.

Second, because a non-immutable requires storage-reading, which increases the gas cost.

You can inherit OZ contract, and override function trustedForwarder() to return your non-immutable variable instead.

1 Like
4

Resharing this answer here:

It's exactly as @barakman said. We prioritized reducing the gas overhead, because reading storage is very expensive and the trusted forwarder has to be read for essentially every function call.

5

Making _trustedForwarder immutable means the address of the trusted forwarder is hardcoded at the time of the deployment of the ERC20 token contract. After some years and thousands of people holding and using the token, say that forwarder gets hacked or no longer works for whatever reason, then what would be the outcome with the token being permanently stuck with the address of a defunct forwarder?

6

If the token contact is upgradeable then upgrade it, setting a new valid value for forwarder address.

If it's not upgradeable then maybe meta transactions woud have to be disabled for the token.

Try to switch to OpenGSN's implementation of ERC-2771, as they probably have more real-world experience with meta transactions than OpenZeppelin. I think making addresses of external entities immutable is not suitable for real-world applications, because external entities can change without your control in the real world.

7

I think the consequences of not being able to change the address of a forwarder that has become invalid is worse than some higher gas.

8

It's funny how the variable is called "trusted" forwarder. In an ideal world, it'd be OK to make the address of a forwarder that is forever-trustworthy immutable.

But in the real world, most things cannot or should not be "forever-trustworthy". So the variable should just be called "forwarder" instead of "trustedForwarder", and the value should be updateable at any time in the future by the contract admin (which would be more trustworthy than an external entity).

9

@frangio, in light of the vulnerabilities discovered recently as described in Arbitrary Address Spoofing Attack: ERC2771Context Multicall Public Disclosure published 3d ago, please remove immutable as proposed by @mow in August.

I've created a new issue for it at https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4791