From 8e3c03ce3d2c5023c9b29603c6db6f5c7788892f Mon Sep 17 00:00:00 2001 From: gator-boi Date: Thu, 7 Sep 2023 23:52:38 -0400 Subject: [PATCH 1/9] evm: add optimized parseAndVerify endpoint --- ethereum/contracts/Getters.sol | 4 +++ ethereum/contracts/Messages.sol | 50 ++++++++++++++++++++++++------ ethereum/contracts/Setters.sol | 9 ++++-- ethereum/contracts/State.sol | 3 ++ ethereum/forge-test/Messages.t.sol | 41 ++++++++++++++++++++++-- 5 files changed, 93 insertions(+), 14 deletions(-) diff --git a/ethereum/contracts/Getters.sol b/ethereum/contracts/Getters.sol index 627b641509..46ce63a0d5 100644 --- a/ethereum/contracts/Getters.sol +++ b/ethereum/contracts/Getters.sol @@ -53,4 +53,8 @@ contract Getters is State { function nextSequence(address emitter) public view returns (uint64) { return _state.sequences[emitter]; } + + function getGuardianSetHash(uint32 index) public view returns (bytes32) { + return _state.guardianSetHashes[index]; + } } \ No newline at end of file diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index c70efbb6a1..066468c078 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -12,11 +12,41 @@ import "./libraries/external/BytesLib.sol"; contract Messages is Getters { using BytesLib for bytes; + function parseAndVerifyVMOptimized(bytes calldata encodedVM, bytes calldata guardianSet) public view returns (Structs.VM memory vm, bool valid, string memory reason) { + // Verify that the specified guardian set is the current guardian set. + require( + getGuardianSetHash(getCurrentGuardianSetIndex()) == keccak256(guardianSet), + "invalid guardian set" + ); + + // TODO: Optimize parsing function. + vm = parseVM(encodedVM); + (valid, reason) = verifyVMInternal(vm, parseGuardianSetOptimized(guardianSet), false); + } + + function parseGuardianSetOptimized(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) { + // Fetch the guardian set length. + uint256 endGuardianKeyIndex = guardianSetData.length - 4; + uint256 guardianCount = endGuardianKeyIndex / 20; + + guardianSet = Structs.GuardianSet({ + keys : new address[](guardianCount), + expirationTime : guardianSetData.toUint32(endGuardianKeyIndex) + }); + + for(uint256 i = 0; i < guardianCount;) { + unchecked { + guardianSet.keys[i] = guardianSetData.toAddress(i * 20); + i += 1; + } + } + } + /// @dev parseAndVerifyVM serves to parse an encodedVM and wholy validate it for consumption function parseAndVerifyVM(bytes calldata encodedVM) public view returns (Structs.VM memory vm, bool valid, string memory reason) { vm = parseVM(encodedVM); /// setting checkHash to false as we can trust the hash field in this case given that parseVM computes and then sets the hash field above - (valid, reason) = verifyVMInternal(vm, false); + (valid, reason) = verifyVMInternal(vm, getGuardianSet(vm.guardianSetIndex), false); } /** @@ -28,7 +58,7 @@ contract Messages is Getters { * - it aims to verify the hash field provided against the contents of the vm */ function verifyVM(Structs.VM memory vm) public view returns (bool valid, string memory reason) { - (valid, reason) = verifyVMInternal(vm, true); + (valid, reason) = verifyVMInternal(vm, getGuardianSet(vm.guardianSetIndex), true); } /** @@ -37,10 +67,7 @@ contract Messages is Getters { * in the case that the vm is securely parsed and the hash field can be trusted, checkHash can be set to false * as the check would be redundant */ - function verifyVMInternal(Structs.VM memory vm, bool checkHash) internal view returns (bool valid, string memory reason) { - /// @dev Obtain the current guardianSet for the guardianSetIndex provided - Structs.GuardianSet memory guardianSet = getGuardianSet(vm.guardianSetIndex); - + function verifyVMInternal(Structs.VM memory vm, Structs.GuardianSet memory guardianSet, bool checkHash) internal view returns (bool valid, string memory reason) { /** * Verify that the hash field in the vm matches with the hash of the contents of the vm if checkHash is set * WARNING: This hash check is critical to ensure that the vm.hash provided matches with the hash of the body. @@ -65,6 +92,8 @@ contract Messages is Getters { } } + uint256 guardianCount = guardianSet.keys.length; + /** * @dev Checks whether the guardianSet has zero keys * WARNING: This keys check is critical to ensure the guardianSet has keys present AND to ensure @@ -72,7 +101,7 @@ contract Messages is Getters { * key length is 0 and vm.signatures length is 0, this could compromise the integrity of both vm and * signature verification. */ - if(guardianSet.keys.length == 0){ + if(guardianCount == 0){ return (false, "invalid guardian set"); } @@ -87,7 +116,7 @@ contract Messages is Getters { * if making any changes to this, obtain additional peer review. If guardianSet key length is 0 and * vm.signatures length is 0, this could compromise the integrity of both vm and signature verification. */ - if (vm.signatures.length < quorum(guardianSet.keys.length)){ + if (vm.signatures.length < quorum(guardianCount)){ return (false, "no quorum"); } @@ -110,8 +139,9 @@ contract Messages is Getters { */ function verifySignatures(bytes32 hash, Structs.Signature[] memory signatures, Structs.GuardianSet memory guardianSet) public pure returns (bool valid, string memory reason) { uint8 lastIndex = 0; + uint256 sigCount = signatures.length; uint256 guardianCount = guardianSet.keys.length; - for (uint i = 0; i < signatures.length; i++) { + for (uint i = 0; i < sigCount;) { Structs.Signature memory sig = signatures[i]; address signatory = ecrecover(hash, sig.v, sig.r, sig.s); // ecrecover returns 0 for invalid signatures. We explicitly require valid signatures to avoid unexpected @@ -134,6 +164,8 @@ contract Messages is Getters { if(signatory != guardianSet.keys[sig.guardianIndex]){ return (false, "VM signature invalid"); } + + unchecked { i += 1; } } /// If we are here, we've validated that the provided signatures are valid for the provided guardianSet diff --git a/ethereum/contracts/Setters.sol b/ethereum/contracts/Setters.sol index 17f8828278..d5bb65445b 100644 --- a/ethereum/contracts/Setters.sol +++ b/ethereum/contracts/Setters.sol @@ -16,10 +16,11 @@ contract Setters is State { function storeGuardianSet(Structs.GuardianSet memory set, uint32 index) internal { uint setLength = set.keys.length; - for (uint i = 0; i < setLength; i++) { + for (uint i = 0; i < setLength;) { require(set.keys[i] != address(0), "Invalid key"); + unchecked { i += 1; } } - _state.guardianSets[index] = set; + _state.guardianSets[index] = set; } function setInitialized(address implementatiom) internal { @@ -54,4 +55,8 @@ contract Setters is State { require(evmChainId == block.chainid, "invalid evmChainId"); _state.evmChainId = evmChainId; } + + function setGuardianSetHash(uint32 index, bytes32 hash) internal { + _state.guardianSetHashes[index] = hash; + } } \ No newline at end of file diff --git a/ethereum/contracts/State.sol b/ethereum/contracts/State.sol index 7fa5a5c7f0..51a83c51ba 100644 --- a/ethereum/contracts/State.sol +++ b/ethereum/contracts/State.sol @@ -44,6 +44,9 @@ contract Storage { // EIP-155 Chain ID uint256 evmChainId; + + // Guardian set hashes. + mapping(uint32 => bytes32) guardianSetHashes; } } diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index 8b8224dc08..ae13be8372 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -25,10 +25,9 @@ contract TestMessages is Test { ExportedMessages messages; Structs.GuardianSet guardianSet; + Structs.GuardianSet guardianSetOpt; - function setUp() public { - messages = new ExportedMessages(); - + function setupSingleGuardian() internal { // initialize guardian set with one guardian address[] memory keys = new address[](1); keys[0] = vm.addr(testGuardian); @@ -36,6 +35,42 @@ contract TestMessages is Test { require(messages.quorum(guardianSet.keys.length) == 1, "Quorum should be 1"); } + function setupMultiGuardian() internal { + // initialize guardian set with 19 guardians + address[] memory keys = new address[](19); + for (uint256 i = 0; i < 19; ++i) { + keys[i] = makeAddr(string(abi.encodePacked("guarian", i))); + } + guardianSetOpt = Structs.GuardianSet(keys, 0); + require(messages.quorum(guardianSetOpt.keys.length) == 13, "Quorum should be 13"); + } + + function setUp() public { + messages = new ExportedMessages(); + setupSingleGuardian(); + setupMultiGuardian(); + } + + function testParseGuardianSetOptimized(uint8 guardianCount) public view { + vm.assume(guardianCount > 0 && guardianCount <= 19); + + // Encode the guardian set. + bytes memory encodedGuardianSet; + for (uint256 i = 0; i < guardianCount; ++i) { + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.keys[i]); + } + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.expirationTime); + + // Parse the guardian set. + Structs.GuardianSet memory parsedSet = messages.parseGuardianSetOptimized(encodedGuardianSet); + + // Validate the results by comparing the parsed set to the original set. + for (uint256 i = 0; i < guardianCount; ++i) { + assert(parsedSet.keys[i] == guardianSetOpt.keys[i]); + } + assert(parsedSet.expirationTime == guardianSetOpt.expirationTime); + } + function testQuorum() public { assertEq(messages.quorum(0), 1); assertEq(messages.quorum(1), 1); From 7b90ea119b95dd0153ee897148310d998dcc81ea Mon Sep 17 00:00:00 2001 From: gator-boi Date: Fri, 8 Sep 2023 09:30:38 -0400 Subject: [PATCH 2/9] evm: store guardian set hash when performing governance --- ethereum/contracts/Governance.sol | 3 +++ ethereum/contracts/GovernanceStructs.sol | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ethereum/contracts/Governance.sol b/ethereum/contracts/Governance.sol index cc01fb473c..adc81f1e3c 100644 --- a/ethereum/contracts/Governance.sol +++ b/ethereum/contracts/Governance.sol @@ -107,6 +107,9 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg // Add the new guardianSet to guardianSets storeGuardianSet(upgrade.newGuardianSet, upgrade.newGuardianSetIndex); + // Store the guardian set hash + setGuardianSetHash(upgrade.newGuardianSetIndex, upgrade.newGuardianSetHash); + // Makes the new guardianSet effective updateGuardianSetIndex(upgrade.newGuardianSetIndex); } diff --git a/ethereum/contracts/GovernanceStructs.sol b/ethereum/contracts/GovernanceStructs.sol index 88d9be2f14..29df2d841a 100644 --- a/ethereum/contracts/GovernanceStructs.sol +++ b/ethereum/contracts/GovernanceStructs.sol @@ -31,7 +31,8 @@ contract GovernanceStructs { uint8 action; uint16 chain; - Structs.GuardianSet newGuardianSet; + bytes32 newGuardianSetHash; + Structs.GuardianSet newGuardianSet; uint32 newGuardianSetIndex; } @@ -102,6 +103,9 @@ contract GovernanceStructs { uint8 guardianLength = encodedUpgrade.toUint8(index); index += 1; + // Guardian set hash. + gsu.newGuardianSetHash = keccak256(encodedUpgrade.slice(index, guardianLength * 20 + 4)); + gsu.newGuardianSet = Structs.GuardianSet({ keys : new address[](guardianLength), expirationTime : 0 From ed86ea95f7462edd6a5eab0c1891b774e8d60b1b Mon Sep 17 00:00:00 2001 From: gator-boi Date: Fri, 8 Sep 2023 12:30:44 -0400 Subject: [PATCH 3/9] evm: make setGuardianSetHash public --- ethereum/contracts/Governance.sol | 2 +- ethereum/contracts/GovernanceStructs.sol | 4 ---- ethereum/contracts/Setters.sol | 17 +++++++++++++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/ethereum/contracts/Governance.sol b/ethereum/contracts/Governance.sol index adc81f1e3c..a1818398ff 100644 --- a/ethereum/contracts/Governance.sol +++ b/ethereum/contracts/Governance.sol @@ -108,7 +108,7 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg storeGuardianSet(upgrade.newGuardianSet, upgrade.newGuardianSetIndex); // Store the guardian set hash - setGuardianSetHash(upgrade.newGuardianSetIndex, upgrade.newGuardianSetHash); + setGuardianSetHash(upgrade.newGuardianSetIndex); // Makes the new guardianSet effective updateGuardianSetIndex(upgrade.newGuardianSetIndex); diff --git a/ethereum/contracts/GovernanceStructs.sol b/ethereum/contracts/GovernanceStructs.sol index 29df2d841a..612a4bfb44 100644 --- a/ethereum/contracts/GovernanceStructs.sol +++ b/ethereum/contracts/GovernanceStructs.sol @@ -31,7 +31,6 @@ contract GovernanceStructs { uint8 action; uint16 chain; - bytes32 newGuardianSetHash; Structs.GuardianSet newGuardianSet; uint32 newGuardianSetIndex; } @@ -103,9 +102,6 @@ contract GovernanceStructs { uint8 guardianLength = encodedUpgrade.toUint8(index); index += 1; - // Guardian set hash. - gsu.newGuardianSetHash = keccak256(encodedUpgrade.slice(index, guardianLength * 20 + 4)); - gsu.newGuardianSet = Structs.GuardianSet({ keys : new address[](guardianLength), expirationTime : 0 diff --git a/ethereum/contracts/Setters.sol b/ethereum/contracts/Setters.sol index d5bb65445b..5be40a31ac 100644 --- a/ethereum/contracts/Setters.sol +++ b/ethereum/contracts/Setters.sol @@ -56,7 +56,20 @@ contract Setters is State { _state.evmChainId = evmChainId; } - function setGuardianSetHash(uint32 index, bytes32 hash) internal { - _state.guardianSetHashes[index] = hash; + function setGuardianSetHash(uint32 index) public { + // Fetch the guardian set at the specified index. + Structs.GuardianSet memory guardianSet = _state.guardianSets[index]; + + uint256 guardianCount = guardianSet.keys.length; + bytes memory encodedGuardianSet; + for (uint256 i = 0; i < guardianCount;) { + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.keys[i]); + unchecked { i += 1; } + } + + // Store the hash. + _state.guardianSetHashes[index] = keccak256( + abi.encodePacked(encodedGuardianSet, guardianSet.expirationTime) + ); } } \ No newline at end of file From 64edd07bb3921e9159fa41bf075ad23b9f20443e Mon Sep 17 00:00:00 2001 From: gator-boi Date: Fri, 8 Sep 2023 14:24:56 -0400 Subject: [PATCH 4/9] evm: add guardianSetIndex argument to parseAndVerifyVMOptimized --- ethereum/contracts/Messages.sol | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 066468c078..5043055262 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -12,15 +12,23 @@ import "./libraries/external/BytesLib.sol"; contract Messages is Getters { using BytesLib for bytes; - function parseAndVerifyVMOptimized(bytes calldata encodedVM, bytes calldata guardianSet) public view returns (Structs.VM memory vm, bool valid, string memory reason) { + function parseAndVerifyVMOptimized( + bytes calldata encodedVM, + bytes calldata guardianSet, + uint32 guardianSetIndex + ) public view returns (Structs.VM memory vm, bool valid, string memory reason) { // Verify that the specified guardian set is the current guardian set. require( - getGuardianSetHash(getCurrentGuardianSetIndex()) == keccak256(guardianSet), + getGuardianSetHash(guardianSetIndex) == keccak256(guardianSet), "invalid guardian set" ); // TODO: Optimize parsing function. vm = parseVM(encodedVM); + + // Verify that the VM is signed with the same guardian set that was specified. + require(vm.guardianSetIndex == guardianSetIndex, "mismatched guardian set index"); + (valid, reason) = verifyVMInternal(vm, parseGuardianSetOptimized(guardianSet), false); } From 572969c04801a63420c94e7acbc85d950d37cff0 Mon Sep 17 00:00:00 2001 From: gator-boi Date: Fri, 8 Sep 2023 16:17:52 -0400 Subject: [PATCH 5/9] evm: add getter to fetch the current encoded guardian set --- ethereum/contracts/Getters.sol | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ethereum/contracts/Getters.sol b/ethereum/contracts/Getters.sol index 46ce63a0d5..ccf690aba0 100644 --- a/ethereum/contracts/Getters.sol +++ b/ethereum/contracts/Getters.sol @@ -57,4 +57,16 @@ contract Getters is State { function getGuardianSetHash(uint32 index) public view returns (bytes32) { return _state.guardianSetHashes[index]; } + + function getEncodedGuardianSet(uint32 index) public view returns (bytes memory encodedGuardianSet) { + Structs.GuardianSet memory guardianSet = getGuardianSet(index); + + // Encode the guardian set. + uint256 guardianCount = guardianSet.keys.length; + for (uint256 i = 0; i < guardianCount;) { + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.keys[i]); + unchecked { i += 1; } + } + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.expirationTime); + } } \ No newline at end of file From c630fc4d0f621f395ff78dee9f013f4addee3e56 Mon Sep 17 00:00:00 2001 From: gator-boi Date: Fri, 8 Sep 2023 16:18:59 -0400 Subject: [PATCH 6/9] evm: add parse and verify test for the optimized end point --- ethereum/Makefile | 2 +- ethereum/contracts/Messages.sol | 2 +- ethereum/forge-test/Messages.t.sol | 169 +++++++++++++++++++++++++---- 3 files changed, 149 insertions(+), 24 deletions(-) diff --git a/ethereum/Makefile b/ethereum/Makefile index a6fc693a79..b199ad3b6a 100644 --- a/ethereum/Makefile +++ b/ethereum/Makefile @@ -35,7 +35,7 @@ node_modules: package-lock.json forge_dependencies: lib/forge-std lib/openzeppelin-contracts lib/forge-std: - forge install foundry-rs/forge-std@v1.5.5 --no-git --no-commit + forge install foundry-rs/forge-std@v1.6.1 --no-git --no-commit lib/openzeppelin-contracts: forge install openzeppelin/openzeppelin-contracts@0457042d93d9dfd760dbaa06a4d2f1216fdbe297 --no-git --no-commit diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 5043055262..01d815659b 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -17,7 +17,7 @@ contract Messages is Getters { bytes calldata guardianSet, uint32 guardianSetIndex ) public view returns (Structs.VM memory vm, bool valid, string memory reason) { - // Verify that the specified guardian set is the current guardian set. + // Verify that the specified guardian set is a valid. require( getGuardianSetHash(guardianSetIndex) == keccak256(guardianSet), "invalid guardian set" diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index ae13be8372..e61a6bfaea 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -6,7 +6,50 @@ pragma solidity ^0.8.0; import "../contracts/Messages.sol"; import "../contracts/Setters.sol"; import "../contracts/Structs.sol"; + import "forge-std/Test.sol"; +import "forge-std/Vm.sol"; + +contract WormholeSigner is Test { + // Signer wallet. + struct Wallet { + address addr; + uint256 key; + } + + function encodeAndSignMessage( + Structs.VM memory vm_, + uint256[] memory guardianKeys, + uint32 guardianSetIndex + ) public pure returns (bytes memory signedMessage) { + // Compute the hash of the body + bytes memory body = abi.encodePacked( + vm_.timestamp, + vm_.nonce, + vm_.emitterChainId, + vm_.emitterAddress, + vm_.sequence, + vm_.consistencyLevel, + vm_.payload + ); + vm_.hash = keccak256(abi.encodePacked(keccak256(body))); + + // Sign the hash with the specified guardian private keys. + uint256 guardianCount = guardianKeys.length; + bytes memory signatures = abi.encodePacked(uint8(guardianCount)); + for (uint256 i = 0; i < guardianCount; ++i) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(guardianKeys[i], vm_.hash); + signatures = abi.encodePacked(signatures, uint8(i), r, s, v - 27); + } + + signedMessage = abi.encodePacked( + vm_.version, + guardianSetIndex, + signatures, + body + ); + } +} contract ExportedMessages is Messages, Setters { function storeGuardianSetPub(Structs.GuardianSet memory set, uint32 index) public { @@ -23,9 +66,13 @@ contract TestMessages is Test { uint256 constant testGuardian = 93941733246223705020089879371323733820373732307041878556247502674739205313440; ExportedMessages messages; + WormholeSigner wormholeSimulator; Structs.GuardianSet guardianSet; + + // Guardian set with 19 guardians and wallets with each signing key. Structs.GuardianSet guardianSetOpt; + uint256[] guardianKeys = new uint256[](19); function setupSingleGuardian() internal { // initialize guardian set with one guardian @@ -39,36 +86,49 @@ contract TestMessages is Test { // initialize guardian set with 19 guardians address[] memory keys = new address[](19); for (uint256 i = 0; i < 19; ++i) { - keys[i] = makeAddr(string(abi.encodePacked("guarian", i))); + // create a keypair for each guardian + VmSafe.Wallet memory wallet = vm.createWallet(string(abi.encodePacked("guardian", i))); + keys[i] = wallet.addr; + guardianKeys[i] = wallet.privateKey; } guardianSetOpt = Structs.GuardianSet(keys, 0); - require(messages.quorum(guardianSetOpt.keys.length) == 13, "Quorum should be 13"); + require(messages.quorum(guardianSetOpt.keys.length) == 13, "Quorum should be 13"); } function setUp() public { messages = new ExportedMessages(); + wormholeSimulator = new WormholeSigner(); setupSingleGuardian(); setupMultiGuardian(); - } - - function testParseGuardianSetOptimized(uint8 guardianCount) public view { - vm.assume(guardianCount > 0 && guardianCount <= 19); - - // Encode the guardian set. - bytes memory encodedGuardianSet; - for (uint256 i = 0; i < guardianCount; ++i) { - encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.keys[i]); - } - encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.expirationTime); - - // Parse the guardian set. - Structs.GuardianSet memory parsedSet = messages.parseGuardianSetOptimized(encodedGuardianSet); - - // Validate the results by comparing the parsed set to the original set. - for (uint256 i = 0; i < guardianCount; ++i) { - assert(parsedSet.keys[i] == guardianSetOpt.keys[i]); - } - assert(parsedSet.expirationTime == guardianSetOpt.expirationTime); + } + + function getSignedVM( + bytes memory payload, + bytes32 emitterAddress, + uint16 emitterChainId, + uint256[] memory _guardianKeys, + uint32 guardianSetIndex + ) internal view returns (bytes memory signedTransfer) { + // construct `TransferWithPayload` Wormhole message + Structs.VM memory vm; + + // set the vm values inline + vm.version = uint8(1); + vm.timestamp = uint32(block.timestamp); + vm.emitterChainId = emitterChainId; + vm.emitterAddress = emitterAddress; + vm.sequence = messages.nextSequence( + address(uint160(uint256(emitterAddress))) + ); + vm.consistencyLevel = 15; + vm.payload = payload; + + // encode the bservation + signedTransfer = wormholeSimulator.encodeAndSignMessage( + vm, + _guardianKeys, + guardianSetIndex + ); } function testQuorum() public { @@ -196,4 +256,69 @@ contract TestMessages is Test { assertEq(valid, false); assertEq(reason, "vm.hash doesn't match body"); } + + function testParseGuardianSetOptimized(uint8 guardianCount) public view { + vm.assume(guardianCount > 0 && guardianCount <= 19); + + // Encode the guardian set. + bytes memory encodedGuardianSet; + for (uint256 i = 0; i < guardianCount; ++i) { + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.keys[i]); + } + encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.expirationTime); + + // Parse the guardian set. + Structs.GuardianSet memory parsedSet = messages.parseGuardianSetOptimized(encodedGuardianSet); + + // Validate the results by comparing the parsed set to the original set. + for (uint256 i = 0; i < guardianCount; ++i) { + assert(parsedSet.keys[i] == guardianSetOpt.keys[i]); + } + assert(parsedSet.expirationTime == guardianSetOpt.expirationTime); + } + + function testParseAndVerifyVMOptimized(bytes memory payload) public { + vm.assume(payload.length > 0 && payload.length < 1000); + + uint16 emitterChainId = 16; + bytes32 emitterAddress = bytes32(uint256(uint160(makeAddr("foreignEmitter")))); + uint32 currentSetIndex = messages.getCurrentGuardianSetIndex(); + + // Set the guardian set to the optimized guardian set. + messages.storeGuardianSetPub(guardianSetOpt, currentSetIndex); + messages.setGuardianSetHash(currentSetIndex); + + // Create a message with an arbitrary payload. + bytes memory signedMessage = getSignedVM( + payload, + emitterAddress, + emitterChainId, + guardianKeys, + currentSetIndex + ); + + // Parse and verify the VM. + (Structs.VM memory vm_, bool valid,) = messages.parseAndVerifyVM(signedMessage); + assertEq(valid, true); + + // Parse and verify the VM using the optimized endpoint. + (Structs.VM memory vmOptimized, bool valid_,) = messages.parseAndVerifyVMOptimized( + signedMessage, + messages.getEncodedGuardianSet(currentSetIndex), + currentSetIndex + ); + assertEq(valid_, true); + + // Validate the results by comparing the parsed VM to the optimized VM. + assertEq(vm_.version, vmOptimized.version); + assertEq(vm_.timestamp, vmOptimized.timestamp); + assertEq(vm_.nonce, vmOptimized.nonce); + assertEq(vm_.emitterChainId, vmOptimized.emitterChainId); + assertEq(vm_.emitterAddress, vmOptimized.emitterAddress); + assertEq(vm_.sequence, vmOptimized.sequence); + assertEq(vm_.consistencyLevel, vmOptimized.consistencyLevel); + assertEq(vm_.payload, vmOptimized.payload); + assertEq(vm_.guardianSetIndex, vmOptimized.guardianSetIndex); + assertEq(vm_.signatures.length, vmOptimized.signatures.length); + } } From 360410b510e1f99b285c58dd6f80f4c89fcc5a70 Mon Sep 17 00:00:00 2001 From: gator-boi Date: Wed, 4 Oct 2023 11:18:05 -0500 Subject: [PATCH 7/9] evm: optimize parseVM --- ethereum/contracts/Getters.sol | 2 +- ethereum/contracts/GovernanceStructs.sol | 2 +- ethereum/contracts/Messages.sol | 90 +++++++++++------------- ethereum/contracts/Setters.sol | 4 +- ethereum/forge-test/Messages.t.sol | 9 +++ ethereum/forge-test/MessagesRV.t.sol | 1 + 6 files changed, 55 insertions(+), 53 deletions(-) diff --git a/ethereum/contracts/Getters.sol b/ethereum/contracts/Getters.sol index ccf690aba0..3ea902b467 100644 --- a/ethereum/contracts/Getters.sol +++ b/ethereum/contracts/Getters.sol @@ -65,7 +65,7 @@ contract Getters is State { uint256 guardianCount = guardianSet.keys.length; for (uint256 i = 0; i < guardianCount;) { encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.keys[i]); - unchecked { i += 1; } + unchecked { ++i; } } encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.expirationTime); } diff --git a/ethereum/contracts/GovernanceStructs.sol b/ethereum/contracts/GovernanceStructs.sol index 612a4bfb44..88d9be2f14 100644 --- a/ethereum/contracts/GovernanceStructs.sol +++ b/ethereum/contracts/GovernanceStructs.sol @@ -31,7 +31,7 @@ contract GovernanceStructs { uint8 action; uint16 chain; - Structs.GuardianSet newGuardianSet; + Structs.GuardianSet newGuardianSet; uint32 newGuardianSetIndex; } diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 01d815659b..a043d982ff 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -6,11 +6,11 @@ pragma experimental ABIEncoderV2; import "./Getters.sol"; import "./Structs.sol"; -import "./libraries/external/BytesLib.sol"; +import "./libraries/relayer/BytesParsing.sol"; contract Messages is Getters { - using BytesLib for bytes; + using BytesParsing for bytes; function parseAndVerifyVMOptimized( bytes calldata encodedVM, @@ -23,7 +23,6 @@ contract Messages is Getters { "invalid guardian set" ); - // TODO: Optimize parsing function. vm = parseVM(encodedVM); // Verify that the VM is signed with the same guardian set that was specified. @@ -39,15 +38,17 @@ contract Messages is Getters { guardianSet = Structs.GuardianSet({ keys : new address[](guardianCount), - expirationTime : guardianSetData.toUint32(endGuardianKeyIndex) + expirationTime : 0 }); + (guardianSet.expirationTime, ) = guardianSetData.asUint32Unchecked(endGuardianKeyIndex); + uint256 offset = 0; for(uint256 i = 0; i < guardianCount;) { + (guardianSet.keys[i], offset) = guardianSetData.asAddressUnchecked(offset); unchecked { - guardianSet.keys[i] = guardianSetData.toAddress(i * 20); - i += 1; + ++i; } - } + } } /// @dev parseAndVerifyVM serves to parse an encodedVM and wholy validate it for consumption @@ -173,46 +174,46 @@ contract Messages is Getters { return (false, "VM signature invalid"); } - unchecked { i += 1; } + unchecked { ++i; } } /// If we are here, we've validated that the provided signatures are valid for the provided guardianSet return (true, ""); - } + } /** * @dev parseVM serves to parse an encodedVM into a vm struct * - it intentionally performs no validation functions, it simply parses raw into a struct */ - function parseVM(bytes memory encodedVM) public pure virtual returns (Structs.VM memory vm) { - uint index = 0; + function parseVM(bytes memory encodedVM) public view virtual returns (Structs.VM memory vm) { + uint256 offset = 0; - vm.version = encodedVM.toUint8(index); - index += 1; // SECURITY: Note that currently the VM.version is not part of the hash // and for reasons described below it cannot be made part of the hash. // This means that this field's integrity is not protected and cannot be trusted. // This is not a problem today since there is only one accepted version, but it // could be a problem if we wanted to allow other versions in the future. - require(vm.version == 1, "VM version incompatible"); + (vm.version, offset) = encodedVM.asUint8Unchecked(offset); + require(vm.version == 1, "invalid payload id"); - vm.guardianSetIndex = encodedVM.toUint32(index); - index += 4; + // Guardian set index. + (vm.guardianSetIndex, offset) = encodedVM.asUint32Unchecked(offset); + + // Parse sigs. + uint256 signersLen; + (signersLen, offset) = encodedVM.asUint8Unchecked(offset); - // Parse Signatures - uint256 signersLen = encodedVM.toUint8(index); - index += 1; vm.signatures = new Structs.Signature[](signersLen); - for (uint i = 0; i < signersLen; i++) { - vm.signatures[i].guardianIndex = encodedVM.toUint8(index); - index += 1; - - vm.signatures[i].r = encodedVM.toBytes32(index); - index += 32; - vm.signatures[i].s = encodedVM.toBytes32(index); - index += 32; - vm.signatures[i].v = encodedVM.toUint8(index) + 27; - index += 1; + for (uint i = 0; i < signersLen;) { + (vm.signatures[i].guardianIndex, offset) = encodedVM.asUint8Unchecked(offset); + (vm.signatures[i].r, offset) = encodedVM.asBytes32Unchecked(offset); + (vm.signatures[i].s, offset) = encodedVM.asBytes32Unchecked(offset); + (vm.signatures[i].v, offset) = encodedVM.asUint8Unchecked(offset); + + unchecked { + vm.signatures[i].v += 27; + ++i; + } } /* @@ -222,29 +223,20 @@ contract Messages is Getters { Changing it could result into two different hashes for the same observation. But xDapps rely on the hash of an observation for replay protection. */ - bytes memory body = encodedVM.slice(index, encodedVM.length - index); + bytes memory body; + (body, ) = encodedVM.sliceUnchecked(offset, encodedVM.length - offset); vm.hash = keccak256(abi.encodePacked(keccak256(body))); // Parse the body - vm.timestamp = encodedVM.toUint32(index); - index += 4; - - vm.nonce = encodedVM.toUint32(index); - index += 4; - - vm.emitterChainId = encodedVM.toUint16(index); - index += 2; - - vm.emitterAddress = encodedVM.toBytes32(index); - index += 32; - - vm.sequence = encodedVM.toUint64(index); - index += 8; - - vm.consistencyLevel = encodedVM.toUint8(index); - index += 1; - - vm.payload = encodedVM.slice(index, encodedVM.length - index); + (vm.timestamp, offset) = encodedVM.asUint32Unchecked(offset); + (vm.nonce, offset) = encodedVM.asUint32Unchecked(offset); + (vm.emitterChainId, offset) = encodedVM.asUint16Unchecked(offset); + (vm.emitterAddress, offset) = encodedVM.asBytes32Unchecked(offset); + (vm.sequence, offset) = encodedVM.asUint64Unchecked(offset); + (vm.consistencyLevel, offset) = encodedVM.asUint8Unchecked(offset); + (vm.payload, offset) = encodedVM.sliceUnchecked(offset, encodedVM.length - offset); + + require(encodedVM.length == offset, "invalid payload length"); } /** diff --git a/ethereum/contracts/Setters.sol b/ethereum/contracts/Setters.sol index 5be40a31ac..61a423fc5e 100644 --- a/ethereum/contracts/Setters.sol +++ b/ethereum/contracts/Setters.sol @@ -18,9 +18,9 @@ contract Setters is State { uint setLength = set.keys.length; for (uint i = 0; i < setLength;) { require(set.keys[i] != address(0), "Invalid key"); - unchecked { i += 1; } + unchecked { ++i; } } - _state.guardianSets[index] = set; + _state.guardianSets[index] = set; } function setInitialized(address implementatiom) internal { diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index e61a6bfaea..5f47824862 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -96,7 +96,9 @@ contract TestMessages is Test { } function setUp() public { + console.log("here?"); messages = new ExportedMessages(); + console.log("or?"); wormholeSimulator = new WormholeSigner(); setupSingleGuardian(); setupMultiGuardian(); @@ -320,5 +322,12 @@ contract TestMessages is Test { assertEq(vm_.payload, vmOptimized.payload); assertEq(vm_.guardianSetIndex, vmOptimized.guardianSetIndex); assertEq(vm_.signatures.length, vmOptimized.signatures.length); + + for (uint256 i = 0; i < vm_.signatures.length; ++i) { + assertEq(vm_.signatures[i].guardianIndex, vmOptimized.signatures[i].guardianIndex); + assertEq(vm_.signatures[i].r, vmOptimized.signatures[i].r); + assertEq(vm_.signatures[i].s, vmOptimized.signatures[i].s); + assertEq(vm_.signatures[i].v, vmOptimized.signatures[i].v); + } } } diff --git a/ethereum/forge-test/MessagesRV.t.sol b/ethereum/forge-test/MessagesRV.t.sol index 32bdb13479..8f234447f7 100644 --- a/ethereum/forge-test/MessagesRV.t.sol +++ b/ethereum/forge-test/MessagesRV.t.sol @@ -7,6 +7,7 @@ import "../contracts/Messages.sol"; import "../contracts/Setters.sol"; import "../contracts/Structs.sol"; import "forge-test/rv-helpers/TestUtils.sol"; +import "../contracts/libraries/external/BytesLib.sol"; contract TestMessagesRV is TestUtils { using BytesLib for bytes; From 5b94fb0308cb85968c2c92d59f35bc197175b90d Mon Sep 17 00:00:00 2001 From: gator-boi Date: Wed, 4 Oct 2023 16:46:46 -0500 Subject: [PATCH 8/9] evm: update parseGuardianSet function name --- ethereum/contracts/Messages.sol | 54 +++++++++++++++--------------- ethereum/forge-test/Messages.t.sol | 44 ++++++++++++------------ 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index a043d982ff..9b85807423 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -13,13 +13,13 @@ contract Messages is Getters { using BytesParsing for bytes; function parseAndVerifyVMOptimized( - bytes calldata encodedVM, - bytes calldata guardianSet, + bytes calldata encodedVM, + bytes calldata guardianSet, uint32 guardianSetIndex ) public view returns (Structs.VM memory vm, bool valid, string memory reason) { - // Verify that the specified guardian set is a valid. + // Verify that the specified guardian set is a valid. require( - getGuardianSetHash(guardianSetIndex) == keccak256(guardianSet), + getGuardianSetHash(guardianSetIndex) == keccak256(guardianSet), "invalid guardian set" ); @@ -28,13 +28,13 @@ contract Messages is Getters { // Verify that the VM is signed with the same guardian set that was specified. require(vm.guardianSetIndex == guardianSetIndex, "mismatched guardian set index"); - (valid, reason) = verifyVMInternal(vm, parseGuardianSetOptimized(guardianSet), false); + (valid, reason) = verifyVMInternal(vm, parseGuardianSet(guardianSet), false); } - function parseGuardianSetOptimized(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) { + function parseGuardianSet(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) { // Fetch the guardian set length. - uint256 endGuardianKeyIndex = guardianSetData.length - 4; - uint256 guardianCount = endGuardianKeyIndex / 20; + uint256 endGuardianKeyIndex = guardianSetData.length - 4; + uint256 guardianCount = endGuardianKeyIndex / 20; guardianSet = Structs.GuardianSet({ keys : new address[](guardianCount), @@ -45,11 +45,11 @@ contract Messages is Getters { uint256 offset = 0; for(uint256 i = 0; i < guardianCount;) { (guardianSet.keys[i], offset) = guardianSetData.asAddressUnchecked(offset); - unchecked { - ++i; - } - } - } + unchecked { + ++i; + } + } + } /// @dev parseAndVerifyVM serves to parse an encodedVM and wholy validate it for consumption function parseAndVerifyVM(bytes calldata encodedVM) public view returns (Structs.VM memory vm, bool valid, string memory reason) { @@ -67,7 +67,7 @@ contract Messages is Getters { * - it aims to verify the hash field provided against the contents of the vm */ function verifyVM(Structs.VM memory vm) public view returns (bool valid, string memory reason) { - (valid, reason) = verifyVMInternal(vm, getGuardianSet(vm.guardianSetIndex), true); + (valid, reason) = verifyVMInternal(vm, getGuardianSet(vm.guardianSetIndex), true); } /** @@ -179,7 +179,7 @@ contract Messages is Getters { /// If we are here, we've validated that the provided signatures are valid for the provided guardianSet return (true, ""); - } + } /** * @dev parseVM serves to parse an encodedVM into a vm struct @@ -188,18 +188,18 @@ contract Messages is Getters { function parseVM(bytes memory encodedVM) public view virtual returns (Structs.VM memory vm) { uint256 offset = 0; - // SECURITY: Note that currently the VM.version is not part of the hash - // and for reasons described below it cannot be made part of the hash. - // This means that this field's integrity is not protected and cannot be trusted. - // This is not a problem today since there is only one accepted version, but it - // could be a problem if we wanted to allow other versions in the future. + // SECURITY: Note that currently the VM.version is not part of the hash + // and for reasons described below it cannot be made part of the hash. + // This means that this field's integrity is not protected and cannot be trusted. + // This is not a problem today since there is only one accepted version, but it + // could be a problem if we wanted to allow other versions in the future. (vm.version, offset) = encodedVM.asUint8Unchecked(offset); require(vm.version == 1, "invalid payload id"); - // Guardian set index. + // Guardian set index. (vm.guardianSetIndex, offset) = encodedVM.asUint32Unchecked(offset); - // Parse sigs. + // Parse sigs. uint256 signersLen; (signersLen, offset) = encodedVM.asUint8Unchecked(offset); @@ -209,18 +209,18 @@ contract Messages is Getters { (vm.signatures[i].r, offset) = encodedVM.asBytes32Unchecked(offset); (vm.signatures[i].s, offset) = encodedVM.asBytes32Unchecked(offset); (vm.signatures[i].v, offset) = encodedVM.asUint8Unchecked(offset); - - unchecked { + + unchecked { vm.signatures[i].v += 27; - ++i; + ++i; } } /* Hash the body - SECURITY: Do not change the way the hash of a VM is computed! - Changing it could result into two different hashes for the same observation. + SECURITY: Do not change the way the hash of a VM is computed! + Changing it could result into two different hashes for the same observation. But xDapps rely on the hash of an observation for replay protection. */ bytes memory body; diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index 5f47824862..afe0e28bad 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -11,15 +11,15 @@ import "forge-std/Test.sol"; import "forge-std/Vm.sol"; contract WormholeSigner is Test { - // Signer wallet. + // Signer wallet. struct Wallet { address addr; uint256 key; } function encodeAndSignMessage( - Structs.VM memory vm_, - uint256[] memory guardianKeys, + Structs.VM memory vm_, + uint256[] memory guardianKeys, uint32 guardianSetIndex ) public pure returns (bytes memory signedMessage) { // Compute the hash of the body @@ -48,7 +48,7 @@ contract WormholeSigner is Test { signatures, body ); - } + } } contract ExportedMessages is Messages, Setters { @@ -66,11 +66,11 @@ contract TestMessages is Test { uint256 constant testGuardian = 93941733246223705020089879371323733820373732307041878556247502674739205313440; ExportedMessages messages; - WormholeSigner wormholeSimulator; + WormholeSigner wormholeSimulator; Structs.GuardianSet guardianSet; - // Guardian set with 19 guardians and wallets with each signing key. + // Guardian set with 19 guardians and wallets with each signing key. Structs.GuardianSet guardianSetOpt; uint256[] guardianKeys = new uint256[](19); @@ -83,16 +83,16 @@ contract TestMessages is Test { } function setupMultiGuardian() internal { - // initialize guardian set with 19 guardians + // initialize guardian set with 19 guardians address[] memory keys = new address[](19); for (uint256 i = 0; i < 19; ++i) { - // create a keypair for each guardian + // create a keypair for each guardian VmSafe.Wallet memory wallet = vm.createWallet(string(abi.encodePacked("guardian", i))); - keys[i] = wallet.addr; - guardianKeys[i] = wallet.privateKey; + keys[i] = wallet.addr; + guardianKeys[i] = wallet.privateKey; } - guardianSetOpt = Structs.GuardianSet(keys, 0); - require(messages.quorum(guardianSetOpt.keys.length) == 13, "Quorum should be 13"); + guardianSetOpt = Structs.GuardianSet(keys, 0); + require(messages.quorum(guardianSetOpt.keys.length) == 13, "Quorum should be 13"); } function setUp() public { @@ -102,7 +102,7 @@ contract TestMessages is Test { wormholeSimulator = new WormholeSigner(); setupSingleGuardian(); setupMultiGuardian(); - } + } function getSignedVM( bytes memory payload, @@ -269,13 +269,13 @@ contract TestMessages is Test { } encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSetOpt.expirationTime); - // Parse the guardian set. - Structs.GuardianSet memory parsedSet = messages.parseGuardianSetOptimized(encodedGuardianSet); + // Parse the guardian set. + Structs.GuardianSet memory parsedSet = messages.parseGuardianSet(encodedGuardianSet); // Validate the results by comparing the parsed set to the original set. for (uint256 i = 0; i < guardianCount; ++i) { assert(parsedSet.keys[i] == guardianSetOpt.keys[i]); - } + } assert(parsedSet.expirationTime == guardianSetOpt.expirationTime); } @@ -290,7 +290,7 @@ contract TestMessages is Test { messages.storeGuardianSetPub(guardianSetOpt, currentSetIndex); messages.setGuardianSetHash(currentSetIndex); - // Create a message with an arbitrary payload. + // Create a message with an arbitrary payload. bytes memory signedMessage = getSignedVM( payload, emitterAddress, @@ -299,14 +299,14 @@ contract TestMessages is Test { currentSetIndex ); - // Parse and verify the VM. + // Parse and verify the VM. (Structs.VM memory vm_, bool valid,) = messages.parseAndVerifyVM(signedMessage); assertEq(valid, true); - // Parse and verify the VM using the optimized endpoint. + // Parse and verify the VM using the optimized endpoint. (Structs.VM memory vmOptimized, bool valid_,) = messages.parseAndVerifyVMOptimized( - signedMessage, - messages.getEncodedGuardianSet(currentSetIndex), + signedMessage, + messages.getEncodedGuardianSet(currentSetIndex), currentSetIndex ); assertEq(valid_, true); @@ -328,6 +328,6 @@ contract TestMessages is Test { assertEq(vm_.signatures[i].r, vmOptimized.signatures[i].r); assertEq(vm_.signatures[i].s, vmOptimized.signatures[i].s); assertEq(vm_.signatures[i].v, vmOptimized.signatures[i].v); - } + } } } From 561b5ba0d6601d18508a075b14ae8beb02ad630c Mon Sep 17 00:00:00 2001 From: Dirk Brink Date: Fri, 5 Jul 2024 12:07:59 -0700 Subject: [PATCH 9/9] evm: Add some tests and update hash on guardian set update --- ethereum/contracts/Governance.sol | 3 ++ ethereum/contracts/Messages.sol | 18 +++++--- ethereum/contracts/Setters.sol | 7 ++- ethereum/contracts/interfaces/IWormhole.sol | 14 ++++++ ethereum/forge-test/Getters.t.sol | 13 ++++++ ethereum/forge-test/Governance.t.sol | 15 +++++++ ethereum/forge-test/Messages.t.sol | 24 +++++------ ethereum/forge-test/Setters.t.sol | 45 ++++++++++++++++++++ ethereum/forge-test/rv-helpers/MySetters.sol | 4 ++ ethereum/forge-test/rv-helpers/TestUtils.sol | 7 +++ 10 files changed, 131 insertions(+), 19 deletions(-) diff --git a/ethereum/contracts/Governance.sol b/ethereum/contracts/Governance.sol index a1818398ff..c569a71e75 100644 --- a/ethereum/contracts/Governance.sol +++ b/ethereum/contracts/Governance.sol @@ -104,6 +104,9 @@ abstract contract Governance is GovernanceStructs, Messages, Setters, ERC1967Upg // Trigger a time-based expiry of current guardianSet expireGuardianSet(getCurrentGuardianSetIndex()); + // Update the hash of the expiring guardian set + setGuardianSetHash(getCurrentGuardianSetIndex()); + // Add the new guardianSet to guardianSets storeGuardianSet(upgrade.newGuardianSet, upgrade.newGuardianSetIndex); diff --git a/ethereum/contracts/Messages.sol b/ethereum/contracts/Messages.sol index 9b85807423..ec6b43a514 100644 --- a/ethereum/contracts/Messages.sol +++ b/ethereum/contracts/Messages.sol @@ -12,14 +12,18 @@ import "./libraries/relayer/BytesParsing.sol"; contract Messages is Getters { using BytesParsing for bytes; + uint8 private constant ADDRESS_SIZE = 20; // in bytes + uint8 private constant EXPIRATION_TIME_SIZE = 4; // in bytes + function parseAndVerifyVMOptimized( bytes calldata encodedVM, bytes calldata guardianSet, uint32 guardianSetIndex ) public view returns (Structs.VM memory vm, bool valid, string memory reason) { // Verify that the specified guardian set is a valid. + bytes32 guardianSetHash = getGuardianSetHash(guardianSetIndex); require( - getGuardianSetHash(guardianSetIndex) == keccak256(guardianSet), + guardianSetHash == keccak256(guardianSet) && guardianSetHash != bytes32(0), "invalid guardian set" ); @@ -32,15 +36,13 @@ contract Messages is Getters { } function parseGuardianSet(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) { - // Fetch the guardian set length. - uint256 endGuardianKeyIndex = guardianSetData.length - 4; - uint256 guardianCount = endGuardianKeyIndex / 20; + uint256 guardianSetDataLength = guardianSetData.length; + uint256 guardianCount = (guardianSetDataLength - EXPIRATION_TIME_SIZE) / ADDRESS_SIZE; guardianSet = Structs.GuardianSet({ keys : new address[](guardianCount), expirationTime : 0 }); - (guardianSet.expirationTime, ) = guardianSetData.asUint32Unchecked(endGuardianKeyIndex); uint256 offset = 0; for(uint256 i = 0; i < guardianCount;) { @@ -49,6 +51,10 @@ contract Messages is Getters { ++i; } } + + (guardianSet.expirationTime, offset) = guardianSetData.asUint32Unchecked(offset); + + require(guardianSetDataLength == offset, "invalid guardian set data length"); } /// @dev parseAndVerifyVM serves to parse an encodedVM and wholy validate it for consumption @@ -194,7 +200,7 @@ contract Messages is Getters { // This is not a problem today since there is only one accepted version, but it // could be a problem if we wanted to allow other versions in the future. (vm.version, offset) = encodedVM.asUint8Unchecked(offset); - require(vm.version == 1, "invalid payload id"); + require(vm.version == 1, "VM version incompatible"); // Guardian set index. (vm.guardianSetIndex, offset) = encodedVM.asUint32Unchecked(offset); diff --git a/ethereum/contracts/Setters.sol b/ethereum/contracts/Setters.sol index 61a423fc5e..02e0a64b5a 100644 --- a/ethereum/contracts/Setters.sol +++ b/ethereum/contracts/Setters.sol @@ -20,7 +20,7 @@ contract Setters is State { require(set.keys[i] != address(0), "Invalid key"); unchecked { ++i; } } - _state.guardianSets[index] = set; + _state.guardianSets[index] = set; } function setInitialized(address implementatiom) internal { @@ -61,6 +61,11 @@ contract Setters is State { Structs.GuardianSet memory guardianSet = _state.guardianSets[index]; uint256 guardianCount = guardianSet.keys.length; + + // Only allow setting the hash for a valid guardian set index + // Governance guards against updating to an empty guardian set + require(guardianCount > 0, "non-existent guardian set"); + bytes memory encodedGuardianSet; for (uint256 i = 0; i < guardianCount;) { encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.keys[i]); diff --git a/ethereum/contracts/interfaces/IWormhole.sol b/ethereum/contracts/interfaces/IWormhole.sol index eba9f6f2ea..3d68bc8c68 100644 --- a/ethereum/contracts/interfaces/IWormhole.sol +++ b/ethereum/contracts/interfaces/IWormhole.sol @@ -88,6 +88,14 @@ interface IWormhole { function parseAndVerifyVM(bytes calldata encodedVM) external view returns (VM memory vm, bool valid, string memory reason); + function parseAndVerifyVMOptimized( + bytes calldata encodedVM, + bytes calldata guardianSet, + uint32 guardianSetIndex + ) external view returns (VM memory vm, bool valid, string memory reason); + + function parseGuardianSet(bytes calldata guardianSetData) external pure returns (GuardianSet memory guardianSet); + function verifyVM(VM memory vm) external view returns (bool valid, string memory reason); function verifySignatures(bytes32 hash, Signature[] memory signatures, GuardianSet memory guardianSet) external pure returns (bool valid, string memory reason); @@ -102,6 +110,10 @@ interface IWormhole { function getGuardianSetExpiry() external view returns (uint32); + function getGuardianSetHash(uint32 index) external view returns (bytes32); + + function getEncodedGuardianSet(uint32 index) external view returns (bytes memory encodedGuardianSet); + function governanceActionIsConsumed(bytes32 hash) external view returns (bool); function isInitialized(address impl) external view returns (bool); @@ -139,4 +151,6 @@ interface IWormhole { function submitTransferFees(bytes memory _vm) external; function submitRecoverChainId(bytes memory _vm) external; + + function setGuardianSetHash(uint32 index) external; } diff --git a/ethereum/forge-test/Getters.t.sol b/ethereum/forge-test/Getters.t.sol index 368150ec4e..2a5fb30433 100644 --- a/ethereum/forge-test/Getters.t.sol +++ b/ethereum/forge-test/Getters.t.sol @@ -217,4 +217,17 @@ contract TestGetters is TestUtils { { testEvmChainId(newEvmChainId, storageSlot); } + + function testGuardianSetHash(uint32 guardianSetIndex, bytes32 newGuardianSetHash, bytes32 storageSlot) + public + unchangedStorage(address(getters), storageSlot) + { + bytes32 storageLocation = hashedLocation(guardianSetIndex, GUARDIANSETHASHES_STORAGE_INDEX); + vm.assume(storageSlot != storageLocation); + + vm.store(address(getters), storageLocation, newGuardianSetHash); + + assertEq(getters.getGuardianSetHash(guardianSetIndex), newGuardianSetHash); + assertEq(newGuardianSetHash, vm.load(address(getters), storageLocation)); + } } diff --git a/ethereum/forge-test/Governance.t.sol b/ethereum/forge-test/Governance.t.sol index d6e315a5ce..f13e9c78f3 100644 --- a/ethereum/forge-test/Governance.t.sol +++ b/ethereum/forge-test/Governance.t.sol @@ -515,6 +515,12 @@ contract TestGovernance is TestUtils { vm.assume(storageSlot != hashedLocationOffset(1, GUARDIANSETS_SLOT, 0)); vm.assume(storageSlot != GUARDIANSETINDEX_SLOT); + + // We expect the hash of the existing and the new guardian sets to change + uint32 currentGuardianSetIndex = proxied.getCurrentGuardianSetIndex(); + vm.assume(storageSlot != hashedLocation(currentGuardianSetIndex, GUARDIANSETHASHES_STORAGE_INDEX)); + vm.assume(storageSlot != hashedLocation(currentGuardianSetIndex + 1, GUARDIANSETHASHES_STORAGE_INDEX)); + vm.assume(0 < newGuardianSet.length); vm.assume(newGuardianSet.length < 20); @@ -532,12 +538,17 @@ contract TestGovernance is TestUtils { vm.assume(storageSlot != hashedLocation(hash, CONSUMED_ACTIONS_SLOT)); + assertEq(proxied.getGuardianSetHash(currentGuardianSetIndex), bytes32(0)); + assertEq(proxied.getGuardianSetHash(currentGuardianSetIndex + 1), bytes32(0)); + proxied.submitNewGuardianSet(_vm); assertEq(true, proxied.governanceActionIsConsumed(hash)); assertEq(uint32(block.timestamp) + 86400, proxied.getGuardianSet(0).expirationTime); assertEq(newGuardianSet, proxied.getGuardianSet(1).keys); assertEq(1, proxied.getCurrentGuardianSetIndex()); + assertNotEq(proxied.getGuardianSetHash(currentGuardianSetIndex), bytes32(0)); + assertNotEq(proxied.getGuardianSetHash(currentGuardianSetIndex + 1), bytes32(0)); } function testSubmitNewGuardianSet_Revert_InvalidModule( @@ -766,6 +777,10 @@ contract TestGovernance is TestUtils { // New GuardianSet array length should be initialized from zero to non-zero vm.assume(storageSlot != hashedLocationOffset(1, GUARDIANSETS_SLOT, 0)); + // We expect the hash of the changing and the new guardian sets to change + vm.assume(storageSlot != hashedLocation(proxied.getCurrentGuardianSetIndex(), GUARDIANSETHASHES_STORAGE_INDEX)); + vm.assume(storageSlot != hashedLocation(proxied.getCurrentGuardianSetIndex() + 1, GUARDIANSETHASHES_STORAGE_INDEX)); + vm.assume(storageSlot != GUARDIANSETINDEX_SLOT); vm.assume(0 < newGuardianSet.length); vm.assume(newGuardianSet.length < 20); diff --git a/ethereum/forge-test/Messages.t.sol b/ethereum/forge-test/Messages.t.sol index afe0e28bad..4cb8e7a19b 100644 --- a/ethereum/forge-test/Messages.t.sol +++ b/ethereum/forge-test/Messages.t.sol @@ -112,22 +112,22 @@ contract TestMessages is Test { uint32 guardianSetIndex ) internal view returns (bytes memory signedTransfer) { // construct `TransferWithPayload` Wormhole message - Structs.VM memory vm; + Structs.VM memory vm_; // set the vm values inline - vm.version = uint8(1); - vm.timestamp = uint32(block.timestamp); - vm.emitterChainId = emitterChainId; - vm.emitterAddress = emitterAddress; - vm.sequence = messages.nextSequence( + vm_.version = uint8(1); + vm_.timestamp = uint32(block.timestamp); + vm_.emitterChainId = emitterChainId; + vm_.emitterAddress = emitterAddress; + vm_.sequence = messages.nextSequence( address(uint160(uint256(emitterAddress))) ); - vm.consistencyLevel = 15; - vm.payload = payload; + vm_.consistencyLevel = 15; + vm_.payload = payload; - // encode the bservation + // encode the observation signedTransfer = wormholeSimulator.encodeAndSignMessage( - vm, + vm_, _guardianKeys, guardianSetIndex ); @@ -259,8 +259,8 @@ contract TestMessages is Test { assertEq(reason, "vm.hash doesn't match body"); } - function testParseGuardianSetOptimized(uint8 guardianCount) public view { - vm.assume(guardianCount > 0 && guardianCount <= 19); + function testParseGuardianSetOptimized(uint256 guardianCount) public view { + guardianCount = bound(guardianCount, 1, 19); // Encode the guardian set. bytes memory encodedGuardianSet; diff --git a/ethereum/forge-test/Setters.t.sol b/ethereum/forge-test/Setters.t.sol index e1649ec2b0..694534a8d0 100644 --- a/ethereum/forge-test/Setters.t.sol +++ b/ethereum/forge-test/Setters.t.sol @@ -5,12 +5,25 @@ pragma solidity ^0.8.0; import "forge-test/rv-helpers/MySetters.sol"; import "forge-test/rv-helpers/TestUtils.sol"; +import "../contracts/Structs.sol"; contract TestSetters is TestUtils { + address constant testGuardianPub = 0xbeFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe; + MySetters setters; + Structs.GuardianSet guardianSet; function setUp() public { setters = new MySetters(); + + address[] memory initialGuardians = new address[](1); + initialGuardians[0] = testGuardianPub; + + // Create a guardian set + guardianSet = Structs.GuardianSet({ + keys: initialGuardians, + expirationTime: 0 + }); } function testUpdateGuardianSetIndex(uint32 index, bytes32 storageSlot) @@ -265,4 +278,36 @@ contract TestSetters is TestUtils { { testSetEvmChainId_Revert(newEvmChainId, storageSlot); } + + function testSetGuardianSetHash_Success(uint256 index, uint256 numGuardianSets, bytes32 storageSlot) + public + unchangedStorage(address(setters), storageSlot) + { + numGuardianSets = bound(numGuardianSets, 1, type(uint8).max); + index = bound(index, 0, numGuardianSets - 1); + + bytes32 storageLocation = hashedLocation(index, GUARDIANSETHASHES_STORAGE_INDEX); + vm.assume(storageSlot != storageLocation); + + for (uint256 i = 0; i < numGuardianSets; i++) { + setters.storeGuardianSet_external(guardianSet, uint32(i)); + } + + setters.setGuardianSetHash(uint32(index)); + } + + function testSetGuardianSetHash_Revert(uint256 index, uint256 numGuardianSets, bytes32 storageSlot) + public + unchangedStorage(address(setters), storageSlot) + { + numGuardianSets = bound(numGuardianSets, 1, type(uint8).max); + index = bound(index, numGuardianSets, type(uint8).max); + + for (uint256 i = 0; i < numGuardianSets; i++) { + setters.storeGuardianSet_external(guardianSet, uint32(i)); + } + + vm.expectRevert("non-existent guardian set"); + setters.setGuardianSetHash(uint32(index)); + } } diff --git a/ethereum/forge-test/rv-helpers/MySetters.sol b/ethereum/forge-test/rv-helpers/MySetters.sol index d2e4388920..30836e828a 100644 --- a/ethereum/forge-test/rv-helpers/MySetters.sol +++ b/ethereum/forge-test/rv-helpers/MySetters.sol @@ -44,4 +44,8 @@ contract MySetters is Setters { function setEvmChainId_external(uint256 evmChainId) external { setEvmChainId(evmChainId); } + + function storeGuardianSet_external(Structs.GuardianSet memory set, uint32 index) external { + storeGuardianSet(set, index); + } } diff --git a/ethereum/forge-test/rv-helpers/TestUtils.sol b/ethereum/forge-test/rv-helpers/TestUtils.sol index 188ec26f0d..b335757e77 100644 --- a/ethereum/forge-test/rv-helpers/TestUtils.sol +++ b/ethereum/forge-test/rv-helpers/TestUtils.sol @@ -19,6 +19,7 @@ bytes32 constant CONSUMEDGOVACTIONS_STORAGE_INDEX = bytes32(uint256(5)); bytes32 constant INITIALIZEDIMPLEMENTATIONS_STORAGE_INDEX = bytes32(uint256(6)); bytes32 constant MESSAGEFEE_STORAGE_INDEX = bytes32(uint256(7)); bytes32 constant EVMCHAINID_STORAGE_INDEX = bytes32(uint256(8)); +bytes32 constant GUARDIANSETHASHES_STORAGE_INDEX = bytes32(uint256(9)); uint256 constant SECP256K1_CURVE_ORDER = 115792089237316195423570985008687907852837564279074904382605163141518161494337; @@ -37,6 +38,12 @@ contract TestUtils is Test, KEVMCheats { return keccak256(abi.encode(_key, _index)); } + // Returns the index hash of the storage slot of a map at location `index` and the key `_key`. + function hashedLocation(uint256 _key, bytes32 _index) public pure returns(bytes32) { + // returns `keccak(#buf(32,_key) +Bytes #buf(32, index)) + return keccak256(abi.encode(_key, _index)); + } + // Returns the index hash of the storage slot of a map at location `index` and the key `_key`. function hashedLocationOffset(uint32 _key, bytes32 _index, uint256 offset) public pure returns(bytes32) { // returns `keccak(#buf(32,_key) +Bytes #buf(32, index))