Merge pull request #2793 from keep-network/reading-members-hash-from-dkg

nkuba · web-flow · commit 4363db2dea76 · 2022-01-31T13:46:40.000+01:00
Fetching members hash from dkg result

We do not need to read members hash from the groups lib, because the dkg result under
challenge has to be the same one that was submitted. Otherwise, the challenge call will be reverted. We can read the members hash from the dkg result instead, which simplifies code.
diff --git a/solidity/random-beacon/contracts/DKGValidator.sol b/solidity/random-beacon/contracts/DKGValidator.sol
@@ -63,15 +63,12 @@ contract DKGValidator {
     ///         and signatures of operators supporting the result.
     /// @param seed seed used to start the DKG and select group members
     /// @param startBlock DKG start block
-    /// @param groupMembersHash Challenged group members hash. Hash must be created
-    ///        by filtering out misbehaved members.
     /// @return isValid true if the result is valid, false otherwise
     /// @return errorMsg validation error message; empty for a valid result
     function validate(
         DKG.Result calldata result,
         uint256 seed,
-        uint256 startBlock,
-        bytes32 groupMembersHash
+        uint256 startBlock
     ) external view returns (bool isValid, string memory errorMsg) {
         (bool hasValidFields, string memory error) = validateFields(result);
         if (!hasValidFields) {
@@ -87,7 +84,7 @@ contract DKGValidator {
         }
 
         // At this point all group members and mishbehaved members were verified
-        if (!validateMembersHash(result, groupMembersHash)) {
+        if (!validateMembersHash(result)) {
             return (false, "Invalid members hash");
         }
 
@@ -252,14 +249,13 @@ contract DKGValidator {
     /// @notice Performs validation of hashed group members that actively took
     ///         part in DKG.
     /// @param result DKG result
-    /// @param actualMembersHash Hashed group members that actively took part in
-    ///        dkg
     /// @return true if result's group members hash matches with the one that is
     ///         challenged.
-    function validateMembersHash(
-        DKG.Result calldata result,
-        bytes32 actualMembersHash
-    ) public view returns (bool) {
+    function validateMembersHash(DKG.Result calldata result)
+        public
+        view
+        returns (bool)
+    {
         if (result.misbehavedMembersIndices.length > 0) {
             // members that generated a group signing key
             uint32[] memory groupMembers = new uint32[](
@@ -277,9 +273,9 @@ contract DKGValidator {
                 }
             }
 
-            return keccak256(abi.encode(groupMembers)) == actualMembersHash;
+            return keccak256(abi.encode(groupMembers)) == result.membersHash;
         }
 
-        return keccak256(abi.encode(result.members)) == actualMembersHash;
+        return keccak256(abi.encode(result.members)) == result.membersHash;
     }
 }
diff --git a/solidity/random-beacon/contracts/RandomBeacon.sol b/solidity/random-beacon/contracts/RandomBeacon.sol
@@ -622,12 +622,8 @@ contract RandomBeacon is Ownable {
     /// @param dkgResult Result to challenge. Must match the submitted result
     ///        stored during `submitDkgResult`.
     function challengeDkgResult(DKG.Result calldata dkgResult) external {
-        bytes32 membersHash = groups
-            .getGroup(dkgResult.groupPubKey)
-            .membersHash;
-
         (bytes32 maliciousResultHash, uint32 maliciousSubmitter) = dkg
-            .challengeResult(dkgResult, membersHash);
+            .challengeResult(dkgResult);
 
         uint256 slashingAmount = maliciousDkgResultSlashingAmount;
         address maliciousSubmitterAddresses = sortitionPool.getIDOperator(
diff --git a/solidity/random-beacon/contracts/libraries/DKG.sol b/solidity/random-beacon/contracts/libraries/DKG.sol
@@ -394,14 +394,9 @@ library DKG {
     /// @dev Can be called during a challenge period for the submitted result.
     /// @param result Result to challenge. Must match the submitted result
     ///        stored during `submitResult`.
-    /// @param groupMembersHash Challenged group members hash.
     /// @return maliciousResultHash Hash of the malicious result.
     /// @return maliciousSubmitter Identifier of the malicious submitter.
-    function challengeResult(
-        Data storage self,
-        Result calldata result,
-        bytes32 groupMembersHash
-    )
+    function challengeResult(Data storage self, Result calldata result)
         external
         returns (bytes32 maliciousResultHash, uint32 maliciousSubmitter)
     {
@@ -425,12 +420,7 @@ library DKG {
         // https://github.com/crytic/slither/issues/982
         // slither-disable-next-line unused-return
         try
-            self.dkgValidator.validate(
-                result,
-                self.seed,
-                self.startBlock,
-                groupMembersHash
-            )
+            self.dkgValidator.validate(result, self.seed, self.startBlock)
         returns (
             // slither-disable-next-line uninitialized-local,variable-scope
             bool isValid,
diff --git a/solidity/random-beacon/test/DKGValidator.test.ts b/solidity/random-beacon/test/DKGValidator.test.ts
@@ -115,7 +115,7 @@ describe("DKGValidator", () => {
       _signers,
       _groupPublicKey,
       _misbehaved,
-      _membersHash
+      _membersHash?: string
     ) => {
       const dkgResult = await prepareDkgResult(
         _groupMembers,
@@ -125,12 +125,11 @@ describe("DKGValidator", () => {
         dkgStartBlock
       )
 
-      const result = await validator.validate(
-        dkgResult,
-        dkgSeed,
-        dkgStartBlock,
-        _membersHash
-      )
+      if (_membersHash) {
+        dkgResult.membersHash = _membersHash
+      }
+
+      const result = await validator.validate(dkgResult, dkgSeed, dkgStartBlock)
 
       return {
         isValid: result[0],
@@ -161,8 +160,7 @@ describe("DKGValidator", () => {
               selectedOperators,
               selectedOperators,
               groupPublicKey,
-              misbehavedMemberIds,
-              hashUint32Array(expectedMembersIds)
+              misbehavedMemberIds
             )
 
             expect(result.isValid).to.be.true
@@ -182,8 +180,7 @@ describe("DKGValidator", () => {
               selectedOperators,
               selectedOperators,
               groupPublicKey,
-              misbehavedMemberIds,
-              hashUint32Array(expectedMembersIds)
+              misbehavedMemberIds
             )
 
             expect(result.isValid).to.be.true
@@ -201,8 +198,7 @@ describe("DKGValidator", () => {
               selectedOperators,
               selectedOperators,
               groupPublicKey,
-              misbehavedMemberIds,
-              hashUint32Array(expectedMembersIds)
+              misbehavedMemberIds
             )
 
             expect(result.isValid).to.be.true
@@ -237,8 +233,7 @@ describe("DKGValidator", () => {
           selectedOperators,
           selectedOperators,
           groupPublicKey,
-          noMisbehaved,
-          hashUint32Array(selectedOperators.map((m) => m.id))
+          noMisbehaved
         )
 
         expect(result.isValid).to.be.true
@@ -257,8 +252,7 @@ describe("DKGValidator", () => {
           shuffledOperators,
           shuffledOperators,
           groupPublicKey,
-          noMisbehaved,
-          hashUint32Array(selectedOperators.map((m) => m.id))
+          noMisbehaved
         )
 
         expect(result.isValid).to.be.false
@@ -277,8 +271,7 @@ describe("DKGValidator", () => {
           selectedOperators,
           shuffledOperators,
           groupPublicKey,
-          noMisbehaved,
-          hashUint32Array(selectedOperators.map((m) => m.id))
+          noMisbehaved
         )
 
         expect(result.isValid).to.be.false
diff --git a/solidity/random-beacon/test/RandomBeacon.GroupCreation.test.ts b/solidity/random-beacon/test/RandomBeacon.GroupCreation.test.ts
@@ -2454,6 +2454,25 @@ describe("RandomBeacon - Group Creation", () => {
               ).to.be.revertedWith("Challenge period has already passed")
             })
           })
+
+          context(
+            "with challenged result not matching the submitted one",
+            async () => {
+              it("should revert with 'Result under challenge is different than the submitted one'", async () => {
+                const modifiedDkgResult: DkgResult = { ...dkgResult }
+                const modifiedMembersHash = hashUint32Array(
+                  dkgResult.members.splice(42, 1)
+                )
+                modifiedDkgResult.membersHash = modifiedMembersHash
+
+                await expect(
+                  randomBeacon.challengeDkgResult(modifiedDkgResult)
+                ).to.be.revertedWith(
+                  "Result under challenge is different than the submitted one"
+                )
+              })
+            }
+          )
         })
 
         context(
