[executor] implement upgradability (#1198)

* get all test to work

* executor upgradable

* update comments

* address feedback

* fix tests

Author: Dev Kalra

target_chains/ethereum/contracts/contracts/executor/Executor.sol

@@ -42,13 +42,13 @@ contract Executor {
     uint16 private ownerEmitterChainId;
     bytes32 private ownerEmitterAddress;
 
-    constructor(
+    function _initialize(
         address _wormhole,
         uint64 _lastExecutedSequence,
         uint16 _chainId,
         uint16 _ownerEmitterChainId,
         bytes32 _ownerEmitterAddress
-    ) {
+    ) internal {
         require(_wormhole != address(0), "_wormhole is zero address");
 
         wormhole = IWormhole(_wormhole);

target_chains/ethereum/contracts/contracts/executor/ExecutorErrors.sol

@@ -16,4 +16,6 @@ library ExecutorErrors {
     error InvalidGovernanceTarget();
     // The target address for the contract call is not a contract
     error InvalidContractTarget();
+    // The governance message is not valid
+    error InvalidMagicValue();
 }

target_chains/ethereum/contracts/contracts/executor/ExecutorUpgradable.sol

@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: Apache 2
+pragma solidity ^0.8.0;
+
+import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
+
+import "./Executor.sol";
+import "./ExecutorErrors.sol";
+
+contract ExecutorUpgradable is
+    Initializable,
+    Ownable2StepUpgradeable,
+    UUPSUpgradeable,
+    Executor
+{
+    event ContractUpgraded(
+        address oldImplementation,
+        address newImplementation
+    );
+
+    function initialize(
+        address wormhole,
+        uint64 lastExecutedSequence,
+        uint16 chainId,
+        uint16 ownerEmitterChainId,
+        bytes32 ownerEmitterAddress
+    ) public initializer {
+        require(wormhole != address(0), "wormhole is zero address");
+
+        __Ownable_init();
+        __UUPSUpgradeable_init();
+
+        Executor._initialize(
+            wormhole,
+            lastExecutedSequence,
+            chainId,
+            ownerEmitterChainId,
+            ownerEmitterAddress
+        );
+
+        // Transfer ownership to the contract itself.
+        _transferOwnership(address(this));
+    }
+
+    /// Ensures the contract cannot be uninitialized and taken over.
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor() initializer {}
+
+    // Only allow the owner to upgrade the proxy to a new implementation.
+    function _authorizeUpgrade(address) internal override onlyOwner {}
+
+    // Upgrade the contract to the given newImplementation. The `newImplementation`
+    // should implement the method  `entropyUpgradableMagic`, see below. If the method
+    // is not implemented or if the magic is different from the current contract, this call
+    // will revert.
+    function upgradeTo(address newImplementation) external override onlyProxy {
+        address oldImplementation = _getImplementation();
+        _authorizeUpgrade(newImplementation);
+        _upgradeToAndCallUUPS(newImplementation, new bytes(0), false);
+
+        magicCheck();
+
+        emit ContractUpgraded(oldImplementation, _getImplementation());
+    }
+
+    // Upgrade the contract to the given newImplementation and call it with the given data.
+    // The `newImplementation` should implement the method  `entropyUpgradableMagic`, see
+    // below. If the method is not implemented or if the magic is different from the current
+    // contract, this call will revert.
+    function upgradeToAndCall(
+        address newImplementation,
+        bytes memory data
+    ) external payable override onlyProxy {
+        address oldImplementation = _getImplementation();
+        _authorizeUpgrade(newImplementation);
+        _upgradeToAndCallUUPS(newImplementation, data, true);
+
+        magicCheck();
+
+        emit ContractUpgraded(oldImplementation, _getImplementation());
+    }
+
+    function magicCheck() internal view {
+        // Calling a method using `this.<method>` will cause a contract call that will use
+        // the new contract. This call will fail if the method does not exists or the magic
+        // is different.
+        if (this.entropyUpgradableMagic() != 0x66697288)
+            revert ExecutorErrors.InvalidMagicValue();
+    }
+
+    function entropyUpgradableMagic() public pure returns (uint32) {
+        return 0x66697288;
+    }
+}

target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol

@@ -4,15 +4,15 @@ pragma solidity ^0.8.0;
 
 import "./utils/EntropyTestUtils.t.sol";
 import "../contracts/entropy/EntropyUpgradable.sol";
-import "./utils/EntropyTestContracts/EntropyDifferentMagic.t.sol";
+import "./utils/InvalidMagic.t.sol";
 import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol";
 
 contract EntropyAuthorized is Test, EntropyTestUtils {
     ERC1967Proxy public proxy;
     EntropyUpgradable public random;
     EntropyUpgradable public random2;
-    EntropyDifferentMagic public randomDifferentMagic;
+    InvalidMagic public randomDifferentMagic;
 
     address public owner = address(1);
     address public admin = address(2);
@@ -36,7 +36,7 @@ contract EntropyAuthorized is Test, EntropyTestUtils {
         random = EntropyUpgradable(address(proxy));
         // to test for upgrade
         random2 = new EntropyUpgradable();
-        randomDifferentMagic = new EntropyDifferentMagic();
+        randomDifferentMagic = new InvalidMagic();
 
         random.initialize(owner, admin, pythFeeInWei, provider1, false);
     }

target_chains/ethereum/contracts/forge-test/Executor.t.sol

@@ -4,13 +4,16 @@ pragma solidity ^0.8.0;
 
 import "forge-std/Test.sol";
 import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol";
-import "../contracts/executor/Executor.sol";
+import "../contracts/executor/ExecutorUpgradable.sol";
 import "./utils/WormholeTestUtils.t.sol";
+import "./utils/InvalidMagic.t.sol";
 
 contract ExecutorTest is Test, WormholeTestUtils {
     Wormhole public wormhole;
-    Executor public executor;
+    ExecutorUpgradable public executor;
+    ExecutorUpgradable public executor2;
     TestCallable public callable;
+    InvalidMagic public executorInvalidMagic;
 
     uint16 OWNER_CHAIN_ID = 7;
     bytes32 OWNER_EMITTER = bytes32(uint256(1));
@@ -19,7 +22,13 @@ contract ExecutorTest is Test, WormholeTestUtils {
 
     function setUp() public {
         address _wormhole = setUpWormholeReceiver(NUM_SIGNERS);
-        executor = new Executor(
+        ExecutorUpgradable _executor = new ExecutorUpgradable();
+        ERC1967Proxy _proxy = new ERC1967Proxy(address(_executor), "");
+        executor = ExecutorUpgradable(payable(address(_proxy)));
+        executor2 = new ExecutorUpgradable();
+        executorInvalidMagic = new InvalidMagic();
+
+        executor.initialize(
             _wormhole,
             0,
             CHAIN_ID,
@@ -56,6 +65,51 @@ contract ExecutorTest is Test, WormholeTestUtils {
         executor.execute(vaa);
     }
 
+    function getTestUpgradeVaa(
+        address newImplementation
+    ) internal returns (bytes memory vaa) {
+        bytes memory payload = abi.encodePacked(
+            uint32(0x5054474d),
+            PythGovernanceInstructions.GovernanceModule.EvmExecutor,
+            Executor.ExecutorAction.Execute,
+            CHAIN_ID,
+            address(executor),
+            address(executor),
+            abi.encodeWithSelector(
+                ExecutorUpgradable.upgradeTo.selector,
+                newImplementation
+            )
+        );
+
+        vaa = generateVaa(
+            uint32(block.timestamp),
+            OWNER_CHAIN_ID,
+            OWNER_EMITTER,
+            1,
+            payload,
+            NUM_SIGNERS
+        );
+    }
+
+    function testUpgradeCallSucceedsForContractWithCorrectMagic() public {
+        bytes memory vaa = getTestUpgradeVaa(address(executor2));
+        executor.execute(vaa);
+    }
+
+    function testUpgradeCallFailsForNotUUPSContract() public {
+        bytes memory vaa = getTestUpgradeVaa(address(callable));
+
+        vm.expectRevert("ERC1967Upgrade: new implementation is not UUPS");
+        executor.execute(vaa);
+    }
+
+    function testUpgradeCallFailsForInvalidMagic() public {
+        bytes memory vaa = getTestUpgradeVaa(address(executorInvalidMagic));
+
+        vm.expectRevert(ExecutorErrors.InvalidMagicValue.selector);
+        executor.execute(vaa);
+    }
+
     function testCallSucceeds() public {
         callable.reset();
 

target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol renamed to target_chains/ethereum/contracts/forge-test/utils/InvalidMagic.t.sol

@@ -4,13 +4,8 @@ pragma solidity ^0.8.0;
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
-import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol";
 
-contract EntropyDifferentMagic is
-    Initializable,
-    OwnableUpgradeable,
-    UUPSUpgradeable
-{
+contract InvalidMagic is Initializable, OwnableUpgradeable, UUPSUpgradeable {
     function initialize() public initializer {
         __Ownable_init();
         __UUPSUpgradeable_init();
@@ -23,15 +18,7 @@ contract EntropyDifferentMagic is
     // // Only allow the owner to upgrade the proxy to a new implementation.
     function _authorizeUpgrade(address) internal override onlyOwner {}
 
-    function magicCheck() internal view {
-        // Calling a method using `this.<method>` will cause a contract call that will use
-        // the new contract. This call will fail if the method does not exists or the magic
-        // is different.
-        if (this.entropyUpgradableMagic() != 0x666972)
-            revert EntropyErrors.InvalidUpgradeMagic();
-    }
-
     function entropyUpgradableMagic() public pure returns (uint32) {
-        return 0x666972;
+        return 0x000000;
     }
 }