Remove runtime validation of ERC1967 slots in proxies

[Jaime-Iglesias]

Following up on the forum discussion - thx for the answer and the note about abstract contracts and constructors Frangio!

The tl;dr is that I wanted to start a constructive discussion around the current implementation of ERC1967.

Particularly, I feel that not validating the storage slots at the ERC1967Upgrade contract level and simply delegating the validation on the contracts implementing it can lead to people forgetting about the validation or to people deliberately "forgetting" - to, perhaps, change one of the slots to create a storage clash.

I understand this last scenario might be out of the scope of the intention of the library but I also like to think about the library as a place that people can reference to "validate" code they find in the wild so I think the validation could serve as a way to raise awareness about the perils of using unstructured storage.

Looking forward to hearing your thoughts!

[Amxx]

Hello @Jaime-Iglesias

What do you mean when you say

validation the storage slots

What would this "validation" look like? Is it checking the content of the slot? Is it recomputing the slot from the "hash(...) -1"?

[Amxx]

Also, What do you mean by

people deliberately "forgetting" to change one of the slots

What is there to change, that people would forget?

[Amxx]

Just to be sure, you understand that two contracts A and B, living at different addresses, can use the same slot without conflict, right?

If A writes to slot "x" and B read from the same slot "x", B will NOT be seeing the value stored by A, because each contract is in its own "independent" space.

[Jaime-Iglesias]

Hey @Amxx,

Thanks for the response,

Hello @Jaime-Iglesias

What do you mean when you say

validation the storage slots

What would this "validation" look like? Is it checking the content of the slot? Is it recomputing the slot from the "hash(...) -1"?

I mean validating the slot was computed from "hash of ... - 1".

Also, What do you mean by

people deliberately "forgetting" to change one of the slots

What is there to change, that people would forget?

I think my message was not very clear there, apologies. I meant that a malicious actor implementing ERC1967Upgrade could deliberately decide not to validate the slot computation because they have actually changed the value to create a storage clash - ofc this scenario might be out of the scope of the library, since it would imply that the code is not directly imported but copied and modified, but having the validation in the library could serve as a reference for people and raise awareness about that posibility.

Just to be sure, you understand that two contracts A and B, living at different addresses, can use the same slot without conflict, right?

If A writes to slot "x" and B read from the same slot "x", B will NOT be seeing the value stored by A, because each contract is in its own "independent" space.

Not sure I follow here...In this case A would be "a contract implementing ERC1967Upgrade" and B would be "ERC1967Upgrade which is abstract and thus could not be instantiated" unless I'm missing the point of what you are trying to arrive at.

Furthermore, to my point - the comments on the code state "validated in the constructor" so I think it's fair to assume implementers of ERC1967Upgrade should be validating the slot computation or that looks like it would be the intention in the first place :D

openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Upgrade.sol

Line 26 in 783ac75

* validated in the constructor.

[Amxx]

Furthermore, to my point - the comments on the code state "validated in the constructor" so I think it's fair to assume implementers of ERC1967Upgrade should be validating the slot computation or that looks like it would be the intention in the first place :D

This refers to this check here:

openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol

Line 23 in 783ac75

assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));

IMO, the issue you are raising is more related to the code packaging/distribution. Someone able to tamper with the source code of this library could add all sorts of backdoors. If it is possible to change the hardcoded slot value, it would also be possible to remove any security check we add that double-checks the value.

[Jaime-Iglesias]

Furthermore, to my point - the comments on the code state "validated in the constructor" so I think it's fair to assume implementers of ERC1967Upgrade should be validating the slot computation or that looks like it would be the intention in the first place :D

This refers to this check here:

openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol

Line 23 in 783ac75

assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));

IMO, the issue you are raising is more related to the code packaging/distribution. Someone able to tamper with the source code of this library could add all sorts of backdoors. If it is possible to change the hardcoded slot value, it would also be possible to remove any security check we add that double-checks the value.

So, although I can get behind the last statement, then this raises the question - why does ERC1967Proxy perform the validation if it's in-fact not needed?

So what I'm trying to say is, if the current implementation values performing the validation as stated here

openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol

Line 23 in 783ac75

assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));

Then why delegate said validation to the implementer when that is error-prone (i.e people forgetting)

Note that, as I mentioned, I think the validation should be done and should not be delegated to implementing contracts (for the reasons exposed) just want to dig into the reasoning behind how the current implementation works and why.

Hope that makes it more clear :D

[frangio]

IMO, the issue you are raising is more related to the code packaging/distribution.

How so?

We have this check in ERC1967Proxy. It's leftover from the original code before refactoring. If we no longer think this check is necessary then we should remove it from ERC1967Proxy, and I think also from TransparentUpgradeableProxy.

I would be ok with removing the check and just hardcoding the constant with a comment explaining what it is.

[Amxx]

What I understand is that issue would arise if someone was to modify the slot that is hardcoded in ERC1967Upgrade.sol. For that to happen, someone would have to tamper with this repo's code, possibly at the npm level.

For a check to work, the "attacker" would have to change the hardcoded value in ERC1967Upgrade.sol and NOT change the check's code. I find that unrealistic.

If it was up to me, we would remove the check in ERC1967Proxy (and any other proxy that does it). Not replace it. Remove it.

[Jaime-Iglesias]

I'm honestly still in favour of performing the check. Even if it's just as an additional security/regression check in case that, for example, a future version of the library accidentally changes one of the hardcoded values.

Of course I would perform these checks at the ERC1967Upgrade constructor and not in contracts inheriting from it.

[Amxx]

IMO ERC1967Upgrade should not be a contract. It is, for all intent and purpose, a library.

AFAIK, the only reason for it to be a contract is so that the event's description gets included in the final contract's ABI. Solidity is going to make that irrelevant.

[frangio]

Even if it's just as an additional security/regression check in case that, for example, a future version of the library accidentally changes one of the hardcoded values.

This is what the test suite is for though! This should be covered by the tests.

[Jaime-Iglesias]

Even if it's just as an additional security/regression check in case that, for example, a future version of the library accidentally changes one of the hardcoded values.

This is what the test suite is for though! This should be covered by the tests.

That makes sense.