To improve compatibility of this proxy with existing and future solutions, I think it would be much better to use the slot defined in EIP-1967: Standard Proxy Storage Slots. @pi0neerpat Can you explain the reasons why this was explicitly decided against? We’re trying to move said EIP to Final state and it would be good to have UUPS on board.
We really do like this model and want to provide an implementation of it in OpenZeppelin Contracts but the choice of an incompatible storage slot sticks out as a problem for us. For inclusion in OpenZeppelin it would also be necessary to move this EIP to Final.
We are on board too with this new 1967 storage slot naming. Working on it here and will report back here once we are finished https://github.com/ethereum/EIPs/pull/2750
Currently the purpose of proxiableUUID is not well explained.
It seems to me it would be made more useful, if it would be about differentiating different types of logic contract as opposes to the slot of the implementation:
abstract contract Proxiable {
...
/**
* @dev Proxiable UUID marker function.
* This would help to avoid wrong logic contract to be used for upgrading.
*/
function proxiableUUID() public pure virtual returns (bytes32);
/**
* @dev Update code address function.
* It is internal, so the derived contract could setup its own permission logic.
*/
function _updateCodeAddress(address newAddress) internal {
require(
proxiableUUID() == Proxiable(newAddress).proxiableUUID(),
"Proxiable: NOT_COMPATIBLE"
);
ProxyUtils.setImplementation(newAddress);
...
While in actual implementation of a specific contract:
function proxiableUUID() public pure override returns (bytes32) {
return keccak256("org.rdai.contracts.RToken.implementation");
}
Yes the language here is vague about what the uuid could be intentionally. Would adding some more examples here be helpful? Or would you want to see the section changed to explain more about how uuid should be used?
And that is suggesting proxiableUUID is basically the slot ID, which I find un-useful. If I wanted to distinguish different type of logic contract, I would rather use the UUID scheme I proposed in the previous reply. Is that close to what you have had in mind, or I have deviated from the original idea?
A wonderful overview with superb Remix demo of Universal Upgradable Proxy Standard by @pi0neerpat & @gbarros on Peep An EIP. Recording available to follow
Hello,
While experimenting with proxy patterns, and trying to come up with an implementation that would be nice and modular, we realize there is an issue with the proxiableUUID as a security mechanism.
proxiableUUID() returns a specific value that is supposed to help distinguish between a UUPS compatible logic, and any other smart contract. The point being that is we update a proxy to point to a non-UUPS compatible contract, this would not include/expose the necessary upgrate mechanisms, and this would break further upgradeability.
However, there is an entier class of contract that do expose the proxiableUUID logic but do not contain the corresponding logic: UUPS proxies.
A UUPS proxy will forward any call to the implementation, and since the implementation is requiered to implement the UUID mechanism, the proxy will actually mimic that. However, upgrading your proxy to use another proxy as your logic will instancty and irrevocably break your proxy.
Thus, we feel like the security mechanism described in ERC-1822 is not good, and should either be removed or modified.
@pi0neerpat How do you feel about removing the proxiableUUID mechanism from the EIP? The resulting EIP would only document the UUPS pattern and not impose any restrictions on the ABI of implementation contracts. I’m not sure if this is compatible with the original goals of the EIP.
A couple of issues with the restricting logic functions sections:
contract Owned is Proxiable {
// ensures no one can manipulate this contract once it is deployed
address public owner = address(1);
// DOS vulnerability
function constructor1() public{
// ensures this can be called only once per *proxy* contract deployed
// *typo* require should check address(1) below
require(owner == address(0));
owner = msg.sender;
}
function updateCode(address newCode) onlyOwner public {
updateCodeAddress(newCode);
}
modifier onlyOwner() {
require(msg.sender == owner, "x");
_;
}
}
The quasi constructor can be DOS’ed by malicious miners, so it would be far preferable to pass in the owner address immediately. Minor, the require check has a typo.
