OpenZeppelin / openzeppelin-foundry-upgrades

Foundry library for deploying and managing upgradeable contracts
MIT License
169 stars 23 forks source link

ProxyAdmin::UPGRADE_INTERFACE_VERSION() not present for LegacyUpgrades #64

Closed tomw1808 closed 3 weeks ago

tomw1808 commented 1 month ago

I think connected to #53 - since then the core tries to call a function on the ProxyAdmin which doesn't exist in previous versions < 5 - so doing anything with LegacyUpgrades using OpenZeppelin contracts 4.9 error out with

[167] ProxyAdmin::UPGRADE_INTERFACE_VERSION()
    └─ ← [Revert] EvmError: Revert

this is not present in https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.9/contracts/proxy/transparent/ProxyAdmin.sol

ericglau commented 1 month ago

The call to UPGRADE_INTERFACE_VERSION() is checked to see if it was successful in https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/blob/4cd15fc50b141c77d8cc9ff8efb44d00e841a299/src/internal/Core.sol#L306-L311

Would you be able to share your code or a minimal reproducible repository that encounters the issue?

tomw1808 commented 1 month ago

Interesting, and you are right, its actually checked. However when I run it using forge script and try to update, I guess it executes a simulated transaction and when anything reverts there it reverts?! (just guessing)...

Here is a minimal example repo https://github.com/tomw1808/foundry-proxy-v4-upgrade

If I'm not mistaken: git clone --recurse-submodules https://github.com/tomw1808/foundry-proxy-v4-upgrade

then open anvil and then run

forge script ./script/ErrorProxyContracts4.sol --rpc-url http://localhost:8545 --broadcast --sender 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 --unlocked -vvvv --force

Looking at the trace I'm not sure what exactly is going on, since it actually continues the execution, but then doesn't send anything, just errors out:

## Setting up 1 EVM.
==========================
Simulated On-chain Traces:

  [362115] → new ProxyAdmin@0x5FbDB2315678afecb367f032d93F642f64180aa3
    ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266)
    └─ ← [Return] 1690 bytes of code

  [49499] → new Counter@0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
    └─ ← [Return] 247 bytes of code

  [490331] → new TransparentUpgradeableProxy@0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
    ├─ emit Upgraded(implementation: Counter: [0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512])
    ├─ emit AdminChanged(previousAdmin: 0x0000000000000000000000000000000000000000, newAdmin: ProxyAdmin: [0x5FbDB2315678afecb367f032d93F642f64180aa3])
    └─ ← [Return] 2196 bytes of code

  [29473] TransparentUpgradeableProxy::increment()
    ├─ [22340] Counter::increment() [delegatecall]
    │   └─ ← [Stop] 
    └─ ← [Return] 

  [49499] → new Counter@0xDc64a140Aa3E981100a9becA4E685f962f0cF6C9
    └─ ← [Return] 247 bytes of code

  [167] ProxyAdmin::UPGRADE_INTERFACE_VERSION()
    └─ ← [Revert] EvmError: Revert

  [17282] ProxyAdmin::upgrade(TransparentUpgradeableProxy: [0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0], Counter: [0xDc64a140Aa3E981100a9becA4E685f962f0cF6C9])
    ├─ [11808] TransparentUpgradeableProxy::upgradeTo(Counter: [0xDc64a140Aa3E981100a9becA4E685f962f0cF6C9])
    │   ├─ emit Upgraded(implementation: Counter: [0xDc64a140Aa3E981100a9becA4E685f962f0cF6C9])
    │   └─ ← [Return] 
    └─ ← [Stop] 

  [12373] TransparentUpgradeableProxy::increment()
    ├─ [5240] Counter::increment() [delegatecall]
    │   └─ ← [Stop] 
    └─ ← [Return] 

Error: 
Simulated execution failed.
tomw1808 commented 1 month ago

Also wanted to add here, I'm escaping this error by doing the Upgrades.validateUpgrade(...) manually before simply updating the implementation using the proxyAdmin, not the nicest way, but sans other options I am out of ideas...

ericglau commented 1 month ago

It looks like you're right that Foundry fails the simulation when there is any revert, even if the revert is expected. We'll need to find another way to check this signature without allowing it to detect a revert.

As a workaround, if you are only using OpenZeppelin Contracts v4, you could just temporarily change the _getUpgradeInterfaceVersion function in Core.sol to directly return an empty string. For example:

    function _getUpgradeInterfaceVersion(address addr) private returns (string memory) {
        // (bool success, bytes memory returndata) = addr.call(abi.encodeWithSignature("UPGRADE_INTERFACE_VERSION()"));
        // if (success) {
        //     return abi.decode(returndata, (string));
        // } else {
        //     return "";
        // }
        return "";
    }
RonTuretzky commented 3 weeks ago

bumping this issue as I'm also facing it