diff --git a/target_chains/ethereum/contracts/contracts/executor/Executor.sol b/target_chains/ethereum/contracts/contracts/executor/Executor.sol index ae92b2a7..a0bce357 100644 --- a/target_chains/ethereum/contracts/contracts/executor/Executor.sol +++ b/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); diff --git a/target_chains/ethereum/contracts/contracts/executor/ExecutorErrors.sol b/target_chains/ethereum/contracts/contracts/executor/ExecutorErrors.sol index ae18743c..e9d972c0 100644 --- a/target_chains/ethereum/contracts/contracts/executor/ExecutorErrors.sol +++ b/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(); } diff --git a/target_chains/ethereum/contracts/contracts/executor/ExecutorUpgradable.sol b/target_chains/ethereum/contracts/contracts/executor/ExecutorUpgradable.sol new file mode 100644 index 00000000..fa9631cf --- /dev/null +++ b/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.` 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; + } +} diff --git a/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol b/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol index 3ab06b7b..d6b46be7 100644 --- a/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol +++ b/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol @@ -4,7 +4,7 @@ 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"; @@ -12,7 +12,7 @@ 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); } diff --git a/target_chains/ethereum/contracts/forge-test/Executor.t.sol b/target_chains/ethereum/contracts/forge-test/Executor.t.sol index da2f40b6..02a4983a 100644 --- a/target_chains/ethereum/contracts/forge-test/Executor.t.sol +++ b/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(); diff --git a/target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol b/target_chains/ethereum/contracts/forge-test/utils/InvalidMagic.t.sol similarity index 58% rename from target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol rename to target_chains/ethereum/contracts/forge-test/utils/InvalidMagic.t.sol index 66af7b27..cd703085 100644 --- a/target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol +++ b/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.` 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; } }