[P1-H01] Require additional data in vote commit hash to prevent dupli…

…cation of votes (UMAprotocol#1217)

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

common/EncryptionHelper.js

@@ -1,17 +1,31 @@
 const web3 = require("web3");
 
+// Web3's soliditySha3 will attempt to auto-detect the type of given input parameters,
+// but this won't produce expected behavior for certain types such as `bytes32` or `address`.
+// Therefore, these helper methods will explicitly set types.
+
 function computeTopicHash(request, roundId) {
-  // Explicitly set the type. Otherwise `identifier` is encoded as a string.
   return web3.utils.soliditySha3(
     { t: "bytes32", v: request.identifier },
     { t: "uint", v: request.time },
     { t: "uint", v: roundId }
   );
 }
 
+function computeVoteHash(request) {
+  return web3.utils.soliditySha3(
+    { t: "int", v: request.price },
+    { t: "int", v: request.salt },
+    { t: "address", v: request.account },
+    { t: "uint", v: request.time },
+    { t: "uint", v: request.roundId },
+    { t: "bytes32", v: request.identifier }
+  );
+}
+
 function getKeyGenMessage(roundId) {
   // TODO: discuss dApp tradeoffs for changing this to a per-topic hash keypair.
   return `UMA Protocol one time key for round: ${roundId.toString()}`;
 }
 
-module.exports = { computeTopicHash, getKeyGenMessage };
+module.exports = { computeTopicHash, computeVoteHash, getKeyGenMessage };

common/VotingUtils.js

@@ -4,7 +4,7 @@ const {
   deriveKeyPairFromSignatureTruffle,
   deriveKeyPairFromSignatureMetamask
 } = require("./Crypto");
-const { getKeyGenMessage, computeTopicHash } = require("./EncryptionHelper");
+const { getKeyGenMessage, computeTopicHash, computeVoteHash } = require("./EncryptionHelper");
 const { BATCH_MAX_COMMITS, BATCH_MAX_RETRIEVALS, BATCH_MAX_REVEALS } = require("./Constants");
 
 const argv = require("minimist")(process.argv.slice());
@@ -21,7 +21,14 @@ const argv = require("minimist")(process.argv.slice());
 const constructCommitment = async (request, roundId, web3, price, account) => {
   const priceWei = web3.utils.toWei(price.toString());
   const salt = web3.utils.toBN(web3.utils.randomHex(32));
-  const hash = web3.utils.soliditySha3(priceWei, salt);
+  const hash = computeVoteHash({
+    price: priceWei,
+    salt,
+    account,
+    time: request.time,
+    roundId,
+    identifier: request.identifier
+  });
 
   const vote = { price: priceWei, salt };
   let publicKey;

core/contracts/oracle/implementation/Voting.sol

@@ -312,7 +312,7 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface, Encrypte
      * Commits can be changed.
      * @param identifier uniquely identifies the committed vote. EG BTC/USD price pair.
      * @param time unix timestamp of the price being voted on.
-     * @param hash keccak256 hash of the price you want to vote for and a `int salt`.
+     * @param hash keccak256 hash of the `price`, `salt`, voter `address`, `time`, current `roundId`, and `identifier`.
      */
     // TODO(#969) Remove once prettier-plugin-solidity can handle the "override" keyword
     // prettier-ignore
@@ -340,7 +340,7 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface, Encrypte
 
     /**
      * @notice Reveal a previously committed vote for `identifier` at `time`.
-     * @dev The revealed `price` and `salt` must match the latest `hash` that `commitVote()` was called with.
+     * @dev The revealed `price`, `salt`, `time`, `address`, `roundId`, and `identifier`, must hash to the latest `hash` that `commitVote()` was called with.
      * Only the committer can reveal their vote.
      * @param identifier voted on in the commit phase. EG BTC/USD price pair.
      * @param time specifies the unix timestamp of the price being voted on.
@@ -363,8 +363,14 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface, Encrypte
         // 0 hashes are disallowed in the commit phase, so they indicate a different error.
         // Cannot reveal an uncommitted or previously revealed hash
         require(voteSubmission.commit != bytes32(0), "Invalid hash reveal");
-        // Committed hash doesn't match revealed price and salt
-        require(keccak256(abi.encode(price, salt)) == voteSubmission.commit, "Invalid commit hash & salt");
+        require(keccak256(abi.encodePacked(
+            price,
+            salt,
+            msg.sender,
+            time,
+            roundId,
+            identifier
+        )) == voteSubmission.commit, "Revealed data != commit hash");
         delete voteSubmission.commit;
 
         // Lock in round variables including snapshotId and inflation rate

core/scripts/local/BatchGasEstimation.js

@@ -10,7 +10,7 @@
 const { RegistryRolesEnum } = require("../../../common/Enums.js");
 const { getRandomSignedInt, getRandomUnsignedInt } = require("../../../common/Random.js");
 const { encryptMessage, deriveKeyPairFromSignatureTruffle } = require("../../../common/Crypto.js");
-const { getKeyGenMessage } = require("../../../common/EncryptionHelper");
+const { getKeyGenMessage, computeVoteHash } = require("../../../common/EncryptionHelper");
 const { moveToNextRound, moveToNextPhase } = require("../../utils/Voting.js");
 
 const Registry = artifacts.require("Registry");
@@ -118,6 +118,7 @@ const cycleCommit = async (voting, identifier, time, requestNum, registeredContr
 
   // Advance to commit phase
   await moveToNextRound(voting);
+  const roundId = await voting.getCurrentRoundId();
 
   const salts = [];
   const price = getRandomSignedInt();
@@ -126,12 +127,18 @@ const cycleCommit = async (voting, identifier, time, requestNum, registeredContr
   // Create the batch of commitments.
   for (var i = 0; i < requestNum; i++) {
     const salt = getRandomUnsignedInt();
-    const hash = web3.utils.soliditySha3(price, salt);
+    const hash = computeVoteHash({
+      price,
+      salt,
+      account: voter,
+      time: time + i,
+      roundId,
+      identifier
+    });
     salts[i] = salt;
 
     // Generate encrypted vote to store on chain.
     const vote = { price, salt };
-    const roundId = await voting.getCurrentRoundId();
     const { publicKey } = await deriveKeyPairFromSignatureTruffle(web3, getKeyGenMessage(roundId), voter);
     const encryptedVote = await encryptMessage(publicKey, JSON.stringify(vote));
 
@@ -151,14 +158,22 @@ const cycleReveal = async (voting, identifier, time, requestNum, registeredContr
   await generatePriceRequests(requestNum, voting, identifier, time, registeredContract);
 
   await moveToNextRound(voting);
+  const roundId = await voting.getCurrentRoundId();
 
   const salts = [];
   const price = getRandomSignedInt();
 
   // Generate Commitments. We will use single commit so no upper bound from the previous test
   for (var i = 0; i < requestNum; i++) {
     const salt = getRandomUnsignedInt();
-    const hash = web3.utils.soliditySha3(price, salt);
+    const hash = computeVoteHash({
+      price,
+      salt,
+      account: voter,
+      time: time + i,
+      roundId,
+      identifier
+    });
     await voting.commitVote(identifier, time + i, hash, { from: voter });
     salts[i] = salt;
   }
@@ -183,13 +198,21 @@ const cycleClaim = async (voting, identifier, time, requestNum, registeredContra
   await generatePriceRequests(requestNum, voting, identifier, time, registeredContract);
 
   await moveToNextRound(voting);
+  let roundId = await voting.getCurrentRoundId();
 
   const salts = [];
   const price = getRandomSignedInt();
 
   for (var i = 0; i < requestNum; i++) {
     const salt = getRandomUnsignedInt();
-    const hash = web3.utils.soliditySha3(price, salt);
+    const hash = computeVoteHash({
+      price,
+      salt,
+      account: voter,
+      time: time + i,
+      roundId,
+      identifier
+    });
     await voting.commitVote(identifier, time + i, hash, { from: voter });
     salts[i] = salt;
   }
@@ -202,7 +225,7 @@ const cycleClaim = async (voting, identifier, time, requestNum, registeredContra
   }
 
   // Finally generate the batch rewards to retrieve
-  const roundId = await voting.getCurrentRoundId();
+  roundId = await voting.getCurrentRoundId();
 
   await moveToNextRound(voting);
 

core/scripts/local/VoteGasEstimation.js

@@ -1,6 +1,7 @@
 const { RegistryRolesEnum } = require("../../../common/Enums.js");
 const { getRandomSignedInt, getRandomUnsignedInt } = require("../../../common/Random.js");
 const { moveToNextRound, moveToNextPhase } = require("../../utils/Voting.js");
+const { computeVoteHash } = require("../../../common/EncryptionHelper");
 
 const Registry = artifacts.require("Registry");
 const Voting = artifacts.require("Voting");
@@ -93,6 +94,7 @@ const cycleRound = async (voting, votingToken, identifier, time, accounts) => {
 
   // Advance to commit phase
   await moveToNextRound(voting);
+  const roundId = await voting.getCurrentRoundId();
 
   const salts = {};
   const price = getRandomSignedInt();
@@ -106,9 +108,15 @@ const cycleRound = async (voting, votingToken, identifier, time, accounts) => {
 
     for (var j = 0; j < numVoters; j++) {
       const salt = getRandomUnsignedInt();
-      const hash = web3.utils.soliditySha3(price, salt);
-
       const voter = getVoter(accounts, j);
+      const hash = computeVoteHash({
+        price,
+        salt,
+        account: voter,
+        time: time + i,
+        roundId,
+        identifier
+      });
 
       const result = await voting.commitVote(identifier, time + i, hash, { from: voter });
       const gasUsed = result.receipt.gasUsed;

core/test/oracle/DesignatedVoting.js

@@ -9,7 +9,7 @@ const VotingToken = artifacts.require("VotingToken");
 const { RegistryRolesEnum } = require("../../../common/Enums.js");
 const { getRandomSignedInt, getRandomUnsignedInt } = require("../../../common/Random.js");
 const { moveToNextRound, moveToNextPhase } = require("../../utils/Voting.js");
-const { computeTopicHash, getKeyGenMessage } = require("../../../common/EncryptionHelper.js");
+const { computeTopicHash, computeVoteHash, getKeyGenMessage } = require("../../../common/EncryptionHelper.js");
 
 contract("DesignatedVoting", function(accounts) {
   const umaAdmin = accounts[0];
@@ -89,10 +89,20 @@ contract("DesignatedVoting", function(accounts) {
     await supportedIdentifiers.addSupportedIdentifier(identifier);
     await voting.requestPrice(identifier, time, { from: registeredContract });
     await moveToNextRound(voting);
+    let roundId = await voting.getCurrentRoundId();
 
     const price = getRandomSignedInt();
     const salt = getRandomUnsignedInt();
-    const hash = web3.utils.soliditySha3(price, salt);
+    // Note: the "voter" address for this vote must be the designated voting contract since its the one that will ultimately
+    // "reveal" the vote. Only the voter can call reveal through the designated voting contract.
+    const hash = computeVoteHash({
+      price,
+      salt,
+      account: designatedVoting.address,
+      time,
+      roundId,
+      identifier
+    });
 
     // Only the voter can commit a vote.
     await designatedVoting.commitVote(identifier, time, hash, { from: voter });
@@ -106,12 +116,12 @@ contract("DesignatedVoting", function(accounts) {
     await moveToNextPhase(voting);
 
     // Only the voter can reveal a vote.
-    await designatedVoting.revealVote(identifier, time, price, salt, { from: voter });
     assert(await didContractThrow(designatedVoting.revealVote(identifier, time, price, salt, { from: tokenOwner })));
     assert(await didContractThrow(designatedVoting.revealVote(identifier, time, price, salt, { from: umaAdmin })));
+    await designatedVoting.revealVote(identifier, time, price, salt, { from: voter });
 
     // Check the resolved price.
-    const roundId = await voting.getCurrentRoundId();
+    roundId = await voting.getCurrentRoundId();
     await moveToNextRound(voting);
     assert.equal((await voting.getPrice(identifier, time, { from: registeredContract })).toString(), price);
 
@@ -154,12 +164,26 @@ contract("DesignatedVoting", function(accounts) {
 
     const price1 = getRandomSignedInt();
     const salt1 = getRandomUnsignedInt();
-    const hash1 = web3.utils.soliditySha3(price1, salt1);
+    const hash1 = computeVoteHash({
+      price: price1,
+      salt: salt1,
+      account: designatedVoting.address,
+      time: time1,
+      roundId,
+      identifier
+    });
     const message1 = web3.utils.randomHex(4);
 
     const price2 = getRandomSignedInt();
     const salt2 = getRandomUnsignedInt();
-    const hash2 = web3.utils.soliditySha3(price2, salt2);
+    const hash2 = computeVoteHash({
+      price: price2,
+      salt: salt2,
+      account: designatedVoting.address,
+      time: time2,
+      roundId,
+      identifier
+    });
     const message2 = web3.utils.randomHex(4);
 
     // Batch commit.