ethereum: be more explicit about invalid guardian + test

ethereum/contracts/Messages.sol

@@ -31,9 +31,9 @@ contract Messages is Getters {
 
        /**
         * @dev Checks whether the guardianSet has zero keys
-        * WARNING: This keys check is critical to ensure the guardianSet has keys present AND to ensure 
+        * 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 
+        * 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){
@@ -48,7 +48,7 @@ contract Messages is Getters {
        /**
         * @dev We're using a fixed point number transformation with 1 decimal to deal with rounding.
         *   WARNING: This quorum check is critical to assessing whether we have enough Guardian signatures to validate a VM
-        *   if making any changes to this, obtain additional peer review. If guardianSet key length is 0 and 
+        *   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)){
@@ -73,6 +73,7 @@ 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 guardianCount = guardianSet.keys.length;
         for (uint i = 0; i < signatures.length; i++) {
             Structs.Signature memory sig = signatures[i];
 
@@ -80,6 +81,14 @@ contract Messages is Getters {
             require(i == 0 || sig.guardianIndex > lastIndex, "signature indices must be ascending");
             lastIndex = sig.guardianIndex;
 
+            /// @dev Ensure that the provided signature index is within the
+            /// bounds of the guardianSet. This is implicitly checked by the array
+            /// index operation below, so this check is technically redundant.
+            /// However, reverting explicitly here ensures that a bug is not
+            /// introduced accidentally later due to the nontrivial storage
+            /// semantics of solidity.
+            require(sig.guardianIndex < guardianCount, "guardian index out of bounds");
+
             /// Check to see if the signer of the signature does not match a specific Guardian key at the provided index
             if(ecrecover(hash, sig.v, sig.r, sig.s) != guardianSet.keys[sig.guardianIndex]){
                 return (false, "VM signature invalid");

ethereum/forge-test/Messages.t.sol

@@ -4,9 +4,12 @@
 pragma solidity ^0.8.0;
 
 import "../contracts/Messages.sol";
+import "../contracts/Structs.sol";
 import "forge-std/Test.sol";
 
 contract TestMessages is Messages, Test {
+  address constant testGuardianPub = 0xbeFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe;
+
   function testQuorum() public {
     assertEq(quorum(0), 1);
     assertEq(quorum(1), 1);
@@ -24,4 +27,34 @@ contract TestMessages is Messages, Test {
     assertEq(quorum(19), 13);
     assertEq(quorum(20), 14);
   }
+
+  // This test ensures that submitting invalid signatures for non-existent
+  // guardians fails.
+  //
+  // The main purpose of this test is to ensure that there's no surprising
+  // behaviour arising from solidity's handling of invalid signatures and out of
+  // bounds memory access. In particular, pubkey recovery of an invalid
+  // signature returns 0, and in some cases out of bounds memory access also
+  // just returns 0.
+  function outOfBoundsSignature() public {
+    // Initialise a guardian set with a single guardian.
+    address[] memory keys = new address[](1);
+    keys[0] = testGuardianPub;
+    Structs.GuardianSet memory guardianSet = Structs.GuardianSet(keys, 0);
+    assertEq(quorum(guardianSet.keys.length), 1);
+
+    // Two invalid signatures, for guardian index 2 and 3 respectively.
+    // These guardian indices are out of bounds for the guardian set.
+    bytes32 message = "hello";
+    Structs.Signature memory bad1 = Structs.Signature(message, 0, 0, 2);
+    Structs.Signature memory bad2 = Structs.Signature(message, 0, 0, 3);
+    // ecrecover on an invalid signature returns 0 instead of reverting
+    assertEq(ecrecover(message, bad1.v, bad1.r, bad1.s), address(0));
+
+    Structs.Signature[] memory badSigs = new Structs.Signature[](2);
+    badSigs[0] = bad1;
+    badSigs[1] = bad2;
+    vm.expectRevert(bytes("guardian index out of bounds"));
+    verifySignatures(0, badSigs, guardianSet);
+  }
 }