Ethereum: add optimized verification endpoint to the core bridge

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

ethereum/contracts/Getters.sol

@@ -53,4 +53,20 @@ 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];
+    }
+
+    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; }
+        }
+        encodedGuardianSet = abi.encodePacked(encodedGuardianSet, guardianSet.expirationTime);
+    }
 }

ethereum/contracts/Governance.sol

@@ -104,9 +104,15 @@ 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);
 
+        // Store the guardian set hash 
+        setGuardianSetHash(upgrade.newGuardianSetIndex);
+
         // Makes the new guardianSet effective
         updateGuardianSetIndex(upgrade.newGuardianSetIndex);
     }

ethereum/contracts/Messages.sol

@@ -6,17 +6,62 @@ 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;
+
+    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(
+            guardianSetHash == keccak256(guardianSet) && guardianSetHash != bytes32(0),
+            "invalid guardian set"
+        );
+
+        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, parseGuardianSet(guardianSet), false);
+    }
+
+    function parseGuardianSet(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) {
+        uint256 guardianSetDataLength = guardianSetData.length;
+        uint256 guardianCount = (guardianSetDataLength - EXPIRATION_TIME_SIZE) / ADDRESS_SIZE;
+
+        guardianSet = Structs.GuardianSet({
+            keys : new address[](guardianCount),
+            expirationTime : 0
+        });
+
+        uint256 offset = 0;
+        for(uint256 i = 0; i < guardianCount;) {
+            (guardianSet.keys[i], offset) = guardianSetData.asAddressUnchecked(offset);
+            unchecked {
+                ++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
     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 +73,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 +82,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,14 +107,16 @@ 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
         * that guardianSet key size doesn't fall to zero and negatively impact quorum assessment.  If guardianSet
         * 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 +131,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 +154,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 +179,8 @@ contract Messages is Getters {
             if(signatory != guardianSet.keys[sig.guardianIndex]){
                 return (false, "VM signature invalid");
             }
+
+            unchecked { ++i; }
         }
 
         /// If we are here, we've validated that the provided signatures are valid for the provided guardianSet
@@ -144,67 +191,58 @@ contract Messages is Getters {
      * @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;
-
-        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.guardianSetIndex = encodedVM.toUint32(index);
-        index += 4;
-
-        // Parse Signatures
-        uint256 signersLen = encodedVM.toUint8(index);
-        index += 1;
+    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.
+        (vm.version, offset) = encodedVM.asUint8Unchecked(offset);
+        require(vm.version == 1, "VM version incompatible");
+
+        // Guardian set index.
+        (vm.guardianSetIndex, offset) = encodedVM.asUint32Unchecked(offset);
+
+        // Parse sigs.
+        uint256 signersLen;
+        (signersLen, offset) = encodedVM.asUint8Unchecked(offset);
+
         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;
+            }
         }
 
         /*
         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 = 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");
     }
 
     /**

ethereum/contracts/Setters.sol

@@ -16,8 +16,9 @@ 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; }
         }
         _state.guardianSets[index] = set;
     }
@@ -54,4 +55,26 @@ contract Setters is State {
         require(evmChainId == block.chainid, "invalid evmChainId");
         _state.evmChainId = evmChainId;
     }
+
+    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;
+
+        // 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]);
+            unchecked { i += 1; }
+        }
+
+        // Store the hash. 
+        _state.guardianSetHashes[index] = keccak256(
+            abi.encodePacked(encodedGuardianSet, guardianSet.expirationTime)
+        );
+    }
 }

ethereum/contracts/State.sol

@@ -44,6 +44,9 @@ contract Storage {
 
         // EIP-155 Chain ID
         uint256 evmChainId;
+
+        // Guardian set hashes. 
+        mapping(uint32 => bytes32) guardianSetHashes;
     }
 }
 

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;
 }

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));
+    }
 }