From b1081d7d9370d4b2b946013eae7b2d1375e66bd7 Mon Sep 17 00:00:00 2001 From: Kevin Peters Date: Fri, 9 Sep 2022 04:26:15 +0000 Subject: [PATCH] ethereum: Simplified forked chain check No longer need to return uint16 max in chainId() for forked chains --- ethereum/contracts/Getters.sol | 3 -- ethereum/contracts/Governance.sol | 13 ++--- ethereum/contracts/bridge/BridgeGetters.sol | 3 -- .../contracts/bridge/BridgeGovernance.sol | 9 ++-- ethereum/contracts/nft/NFTBridgeGetters.sol | 3 -- .../contracts/nft/NFTBridgeGovernance.sol | 9 ++-- ethereum/forge-test/Bridge.t.sol | 4 +- ethereum/test/wormhole.js | 54 +------------------ 8 files changed, 20 insertions(+), 78 deletions(-) diff --git a/ethereum/contracts/Getters.sol b/ethereum/contracts/Getters.sol index 511374433..8540de0c8 100644 --- a/ethereum/contracts/Getters.sol +++ b/ethereum/contracts/Getters.sol @@ -27,9 +27,6 @@ contract Getters is State { } function chainId() public view returns (uint16) { - if (evmChainId() != block.chainid) { - return type(uint16).max; - } return _state.provider.chainId; } diff --git a/ethereum/contracts/Governance.sol b/ethereum/contracts/Governance.sol index 67366e951..cd21252ca 100644 --- a/ethereum/contracts/Governance.sol +++ b/ethereum/contracts/Governance.sol @@ -25,6 +25,8 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg * @dev Upgrades a contract via Governance VAA/VM */ function submitContractUpgrade(bytes memory _vm) public { + require(evmChainId() == block.chainid, "bad fork"); + Structs.VM memory vm = parseVM(_vm); // Verify the VAA is valid before processing it @@ -37,8 +39,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg require(upgrade.module == module, "Invalid Module"); // Verify the VAA is for this chain - uint16 chainId = chainId(); - require(upgrade.chain == chainId && chainId != type(uint16).max, "Invalid Chain"); + require(upgrade.chain == chainId(), "Invalid Chain"); // Record the governance action as consumed setGovernanceActionConsumed(vm.hash); @@ -63,7 +64,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg require(upgrade.module == module, "Invalid Module"); // Verify the VAA is for this chain - require(upgrade.chain == chainId(), "Invalid Chain"); + require(upgrade.chain == chainId() && evmChainId() == block.chainid, "Invalid Chain"); // Record the governance action as consumed to prevent reentry setGovernanceActionConsumed(vm.hash); @@ -88,7 +89,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg require(upgrade.module == module, "invalid Module"); // Verify the VAA is for this chain - require(upgrade.chain == chainId() || upgrade.chain == 0, "invalid Chain"); + require((upgrade.chain == chainId() && evmChainId() == block.chainid) || upgrade.chain == 0, "invalid Chain"); // Verify the Guardian Set keys are not empty, this guards // against the accidential upgrade to an empty GuardianSet @@ -127,7 +128,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg require(transfer.module == module, "invalid Module"); // Verify the VAA is for this chain - require(transfer.chain == chainId() || transfer.chain == 0, "invalid Chain"); + require((transfer.chain == chainId() && evmChainId() == block.chainid) || transfer.chain == 0, "invalid Chain"); // Record the governance action as consumed to prevent reentry setGovernanceActionConsumed(vm.hash); @@ -143,7 +144,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg * @dev Updates the `chainId` and `evmChainId` on a forked chain via Governance VAA/VM */ function submitRecoverChainId(bytes memory _vm) public { - require(chainId() == type(uint16).max, "invalid chain"); + require(evmChainId() != block.chainid, "not a fork"); Structs.VM memory vm = parseVM(_vm); diff --git a/ethereum/contracts/bridge/BridgeGetters.sol b/ethereum/contracts/bridge/BridgeGetters.sol index d6bf4e84f..c1bbf48bb 100644 --- a/ethereum/contracts/bridge/BridgeGetters.sol +++ b/ethereum/contracts/bridge/BridgeGetters.sol @@ -27,9 +27,6 @@ contract BridgeGetters is BridgeState { } function chainId() public view returns (uint16){ - if (evmChainId() != block.chainid) { - return type(uint16).max; - } return _state.provider.chainId; } diff --git a/ethereum/contracts/bridge/BridgeGovernance.sol b/ethereum/contracts/bridge/BridgeGovernance.sol index fd0dce803..600bb9c6c 100644 --- a/ethereum/contracts/bridge/BridgeGovernance.sol +++ b/ethereum/contracts/bridge/BridgeGovernance.sol @@ -33,7 +33,7 @@ contract BridgeGovernance is BridgeGetters, BridgeSetters, ERC1967Upgrade { BridgeStructs.RegisterChain memory chain = parseRegisterChain(vm.payload); - require(chain.chainId == chainId() || chain.chainId == 0, "invalid chain id"); + require((chain.chainId == chainId() && evmChainId() == block.chainid) || chain.chainId == 0, "invalid chain id"); require(bridgeContracts(chain.emitterChainID) == bytes32(0), "chain already registered"); setBridgeImplementation(chain.emitterChainID, chain.emitterAddress); @@ -41,6 +41,8 @@ contract BridgeGovernance is BridgeGetters, BridgeSetters, ERC1967Upgrade { // Execute a UpgradeContract governance message function upgrade(bytes memory encodedVM) public { + require(evmChainId() == block.chainid, "bad fork"); + (IWormhole.VM memory vm, bool valid, string memory reason) = verifyGovernanceVM(encodedVM); require(valid, reason); @@ -48,8 +50,7 @@ contract BridgeGovernance is BridgeGetters, BridgeSetters, ERC1967Upgrade { BridgeStructs.UpgradeContract memory implementation = parseUpgrade(vm.payload); - uint16 chainId = chainId(); - require(implementation.chainId == chainId && chainId != type(uint16).max, "wrong chain id"); + require(implementation.chainId == chainId(), "wrong chain id"); upgradeImplementation(address(uint160(uint256(implementation.newContract)))); } @@ -58,7 +59,7 @@ contract BridgeGovernance is BridgeGetters, BridgeSetters, ERC1967Upgrade { * @dev Updates the `chainId` and `evmChainId` on a forked chain via Governance VAA/VM */ function submitRecoverChainId(bytes memory encodedVM) public { - require(chainId() == type(uint16).max, "invalid chain"); + require(evmChainId() != block.chainid, "not a fork"); (IWormhole.VM memory vm, bool valid, string memory reason) = verifyGovernanceVM(encodedVM); require(valid, reason); diff --git a/ethereum/contracts/nft/NFTBridgeGetters.sol b/ethereum/contracts/nft/NFTBridgeGetters.sol index 12546b5e3..7bf5215cf 100644 --- a/ethereum/contracts/nft/NFTBridgeGetters.sol +++ b/ethereum/contracts/nft/NFTBridgeGetters.sol @@ -27,9 +27,6 @@ contract NFTBridgeGetters is NFTBridgeState { } function chainId() public view returns (uint16){ - if (evmChainId() != block.chainid) { - return type(uint16).max; - } return _state.provider.chainId; } diff --git a/ethereum/contracts/nft/NFTBridgeGovernance.sol b/ethereum/contracts/nft/NFTBridgeGovernance.sol index 060d9a900..b2af249ad 100644 --- a/ethereum/contracts/nft/NFTBridgeGovernance.sol +++ b/ethereum/contracts/nft/NFTBridgeGovernance.sol @@ -32,13 +32,15 @@ contract NFTBridgeGovernance is NFTBridgeGetters, NFTBridgeSetters, ERC1967Upgra NFTBridgeStructs.RegisterChain memory chain = parseRegisterChain(vm.payload); - require(chain.chainId == chainId() || chain.chainId == 0, "invalid chain id"); + require((chain.chainId == chainId() && evmChainId() == block.chainid) || chain.chainId == 0, "invalid chain id"); setBridgeImplementation(chain.emitterChainID, chain.emitterAddress); } // Execute a UpgradeContract governance message function upgrade(bytes memory encodedVM) public { + require(evmChainId() == block.chainid, "bad fork"); + (IWormhole.VM memory vm, bool valid, string memory reason) = verifyGovernanceVM(encodedVM); require(valid, reason); @@ -46,8 +48,7 @@ contract NFTBridgeGovernance is NFTBridgeGetters, NFTBridgeSetters, ERC1967Upgra NFTBridgeStructs.UpgradeContract memory implementation = parseUpgrade(vm.payload); - uint16 chainId = chainId(); - require(implementation.chainId == chainId && chainId != type(uint16).max, "wrong chain id"); + require(implementation.chainId == chainId(), "wrong chain id"); upgradeImplementation(address(uint160(uint256(implementation.newContract)))); } @@ -56,7 +57,7 @@ contract NFTBridgeGovernance is NFTBridgeGetters, NFTBridgeSetters, ERC1967Upgra * @dev Updates the `chainId` and `evmChainId` on a forked chain via Governance VAA/VM */ function submitRecoverChainId(bytes memory encodedVM) public { - require(chainId() == type(uint16).max, "invalid chain"); + require(evmChainId() != block.chainid, "not a fork"); (IWormhole.VM memory vm, bool valid, string memory reason) = verifyGovernanceVM(encodedVM); require(valid, reason); diff --git a/ethereum/forge-test/Bridge.t.sol b/ethereum/forge-test/Bridge.t.sol index 544b6439a..194793ef7 100644 --- a/ethereum/forge-test/Bridge.t.sol +++ b/ethereum/forge-test/Bridge.t.sol @@ -14,7 +14,7 @@ contract TestBridge is Bridge, Test { require(converted == b, "truncate does not roundrip"); } - function testChainId() public { + function testEvmChainId() public { vm.chainId(1); setChainId(1); setEvmChainId(1); @@ -23,8 +23,6 @@ contract TestBridge is Bridge, Test { // fork occurs, block.chainid changes vm.chainId(10001); - assertEq(chainId(), type(uint16).max); - assertEq(evmChainId(), 1); setEvmChainId(10001); assertEq(chainId(), 1); diff --git a/ethereum/test/wormhole.js b/ethereum/test/wormhole.js index 461b04b9b..3a4ad81f9 100644 --- a/ethereum/test/wormhole.js +++ b/ethereum/test/wormhole.js @@ -614,57 +614,7 @@ contract("Wormhole", function () { assert.ok(isUpgraded); }) - it("should revert smart contract upgrades with the bad fork chain ID (uint16 max)", async function () { - const initialized = new web3.eth.Contract(ImplementationFullABI, Wormhole.address); - const accounts = await web3.eth.getAccounts(); - - const mock = await MockImplementation.new(); - - const timestamp = 1000; - const nonce = 1001; - const emitterChainId = testGovernanceChainId; - const emitterAddress = testGovernanceContract - - data = [ - // Core - core, - // Action 1 (Contract Upgrade) - actionContractUpgrade, - // ChainID (max uint16 - bad fork) - web3.eth.abi.encodeParameter("uint16", 65535).substring(2 + (64 - 4)), - // New Contract Address - web3.eth.abi.encodeParameter("address", mock.address).substring(2), - ].join('') - - const vm = await signAndEncodeVM( - timestamp, - nonce, - emitterChainId, - emitterAddress, - 0, - data, - [ - testSigner1PK, - testSigner2PK, - testSigner3PK - ], - 1, - 2 - ); - - try { - await initialized.methods.submitContractUpgrade("0x" + vm).send({ - value: 0, - from: accounts[0], - gasLimit: 1000000 - }); - assert.fail("contract upgrade for bad fork accepted") - } catch (e) { - assert.equal(e.data[Object.keys(e.data)[0]].reason, "Invalid Chain") - } - }) - - it("should revert recover chain ID governance packets on supported (non-bad-fork) chains", async function () { + it("should revert recover chain ID governance packets on canonical chains (non-forked)", async function () { const initialized = new web3.eth.Contract(ImplementationFullABI, Wormhole.address); const accounts = await web3.eth.getAccounts(); @@ -708,7 +658,7 @@ contract("Wormhole", function () { }); assert.fail("recover chain ID governance packet on supported chain accepted") } catch (e) { - assert.equal(e.data[Object.keys(e.data)[0]].reason, "invalid chain") + assert.equal(e.data[Object.keys(e.data)[0]].reason, "not a fork") } })