ethereum: adding hash check in verifyVM (#1851)

* adding a check to verify the vm.hash against the hash of the body and tests for the same

ethereum/contracts/Messages.sol

@@ -15,7 +15,8 @@ contract Messages is Getters {
     /// @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);
-        (valid, reason) = verifyVM(vm);
+        /// 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);
     }
 
    /**
@@ -24,11 +25,46 @@ contract Messages is Getters {
     *  - it aims to ensure the guardianSet is not expired
     *  - it aims to ensure the VM has reached quorum
     *  - it aims to verify the signatures provided against the guardianSet
+    *  - 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);    
+    }
+
+    /**
+    * @dev `verifyVMInternal` serves to validate an arbitrary vm against a valid Guardian set
+    * if checkHash is set then the hash field of the vm is verified against the hash of its contents
+    * 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);
 
+        /**
+         * 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.
+         * Without this check, it would not be safe to call verifyVM on it's own as vm.hash can be a valid signed hash
+         * but the body of the vm could be completely different from what was actually signed by the guardians
+         */
+        if(checkHash){
+            bytes memory body = abi.encodePacked(
+                vm.timestamp,
+                vm.nonce,
+                vm.emitterChainId,
+                vm.emitterAddress,
+                vm.sequence,
+                vm.consistencyLevel,
+                vm.payload
+            );
+
+            bytes32 vmHash = keccak256(abi.encodePacked(keccak256(body)));
+
+            if(vmHash != vm.hash){
+                return (false, "vm.hash doesn't match body");
+            }
+        }
+
        /**
         * @dev Checks whether the guardianSet has zero keys
         * WARNING: This keys check is critical to ensure the guardianSet has keys present AND to ensure
@@ -65,6 +101,7 @@ contract Messages is Getters {
         return (true, "");
     }
 
+
     /**
      * @dev verifySignatures serves to validate arbitrary sigatures against an arbitrary guardianSet
      *  - it intentionally does not solve for expectations within guardianSet (you should use verifyVM if you need these protections)

ethereum/forge-test/Messages.t.sol

@@ -4,10 +4,16 @@
 pragma solidity ^0.8.0;
 
 import "../contracts/Messages.sol";
+import "../contracts/Setters.sol";
 import "../contracts/Structs.sol";
 import "forge-std/Test.sol";
 
-contract TestMessages is Test {
+contract TestMessages is Messages, Test, Setters {
+  address constant testGuardianPub = 0xbeFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe;
+
+  // A valid VM with one signature from the testGuardianPublic key
+  bytes validVM = hex"01000000000100867b55fec41778414f0683e80a430b766b78801b7070f9198ded5e62f48ac7a44b379a6cf9920e42dbd06c5ebf5ec07a934a00a572aefc201e9f91c33ba766d900000003e800000001000b0000000000000000000000000000000000000000000000000000000000000eee00000000000005390faaaa";
+
   uint256 constant testGuardian = 93941733246223705020089879371323733820373732307041878556247502674739205313440;
 
   Messages messages;
@@ -116,4 +122,37 @@ contract TestMessages is Test {
     assertEq(valid, true);
     assertEq(bytes(reason).length, 0);
   }
+
+  // This test checks the possibility of getting a unsigned message verified through verifyVM
+  function testHashMismatchedVMIsNotVerified() public {
+    // Set the initial guardian set
+    address[] memory initialGuardians = new address[](1);
+    initialGuardians[0] = testGuardianPub;
+
+    // Create a guardian set
+    Structs.GuardianSet memory initialGuardianSet = Structs.GuardianSet({
+      keys: initialGuardians,
+      expirationTime: 0
+    });
+
+    storeGuardianSet(initialGuardianSet, uint32(0));
+
+    // Confirm that the test VM is valid
+    (Structs.VM memory parsedValidVm, bool valid, string memory reason) = this.parseAndVerifyVM(validVM);
+    require(valid, reason);
+    assertEq(valid, true);
+    assertEq(reason, "");
+
+    // Manipulate the payload of the vm
+    Structs.VM memory invalidVm = parsedValidVm;
+    invalidVm.payload = abi.encodePacked(
+        parsedValidVm.payload,
+        "malicious bytes in payload"
+    );
+
+    // Confirm that the verifyVM fails on invalid VM
+    (valid, reason) = this.verifyVM(invalidVm);
+    assertEq(valid, false);
+    assertEq(reason, "vm.hash doesn't match body");
+  }
 }