[P1-H01] Require additional data in vote commit hash to prevent duplication of votes by nicholaspai · Pull Request #1217 · UMAprotocol/protocol

nicholaspai · 2020-04-10T16:32:13Z

Resolves #1216
Resolves #1219

Caller now must include an address, price request timestamp, round ID, and pricefeed identifier with their vote hash. The hash is enforced in revealVote.

This includes fixes for the voting-cli and the voter-dapp as well.

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

… governor-payable-proposals

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

… governor-payable-proposals

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

nicholaspai · 2020-04-10T16:43:09Z

-        // Committed hash doesn't match revealed price and salt
-        require(keccak256(abi.encode(price, salt)) == voteSubmission.commit, "Invalid commit hash & salt");
+        // Committed hash doesn't match revealed voter address, price and salt
+        require(keccak256(abi.encodePacked(price, salt, msg.sender)) == voteSubmission.commit, "Invalid commit hash, address & salt");


abi.encode(...., address) does not return the same hash that web3.utils.soliditySha3(....,address) does, but abi.encodePacked does. I'm not 100% sure why this works, but I believe that if an address is input into abi.encode(), its format needs to be manipulated a bit, including getting rid of the leading 0x...

https://ethereum.stackexchange.com/questions/29139/how-does-solidity-tightly-packed-arguments-work-in-sha256/46161

My understanding is that encode and encodePacked differ for types that are less than 32 bytes or dynamically sized. Since an address is less than 32 bytes, I'm guessing it's creating the problem. See here for the solidity docs around this. In the docs for soliditySha3, they say that they "tightly pack" the arguments before hashing. So I think this is actually more correct than it was before.

Now that we're tossing more types in here, we may need to be more careful about how we compute soliditySha3 offchain. web3 attempts to autodetect types, but it's not perfect. Since solidity knows the types, if web3 autodetects incorrectly, it could lead to hash differences due to differences in encoding. To solve this problem, web3 offers a way to provide the types of the arguments passed to soliditySha3 (see examples here). We don't need to do this in this PR, but it's something we may consider doing in the future to make sure these hashes are always computed in exactly the same way.

Right. I'll raise an issue for this so we can do in a subsequent PR.

Just to add one more note on this.

I want to explain my concerns around js autodecting types. If you look at the docs, it autodetects bytes, but not addresses. My guess is that addresses just get encoded the same way as bytes, which probably works fine most of the time. However, ~1 out of every 128 addresses has a 0 for the last byte (or first byte), assuming they're evenly distributed. If the string representation of that address drops the trailing 0, then it might be interpreted as a bytes19 by js. However, solidity would still encode that 0 because it knows an address is 20 bytes. This would lead to a different hash in solidity and js. This could similarly cause problems when hashing the bytes32 representation of our identifiers since there will be padded 0s on the end, which may not show up in the string or may be thrown out by web3. I haven't tested any of this, but these sorts of errors are plausible to the point where I think it might make sense to specify the types on the client side to avoid corrupted hashes.

Just to add one more note on this.

I want to explain my concerns around js autodecting types. If you look at the docs, it autodetects bytes, but not addresses. My guess is that addresses just get encoded the same way as bytes, which probably works fine most of the time. However, ~1 out of every 128 addresses has a 0 for the last byte (or first byte), assuming they're evenly distributed. If the string representation of that address drops the trailing 0, then it might be interpreted as a bytes19 by js. However, solidity would still encode that 0 because it knows an address is 20 bytes. This would lead to a different hash in solidity and js. This could similarly cause problems when hashing the bytes32 representation of our identifiers since there will be padded 0s on the end, which may not show up in the string or may be thrown out by web3. I haven't tested any of this, but these sorts of errors are plausible to the point where I think it might make sense to specify the types on the client side to avoid corrupted hashes.

Agreed with this. Will add comments to issue.

what about type casing the input concatenation of price + salt + voter to a string and then it will always be the same, irrespective of type or how js interprets the leading (or trailing) 0 in the address?

@chrismaree that would work but it would require changing the contract interface. Its much easier IMO to just make the client code specify the types to hash:

old code:

const newHash = web3.utils.soliditySha3( newPrice, newSalt, account1, time, currentRoundId, identifier );

new code:

const newHash = web3.utils.soliditySha3( { t: "int", v: newPrice }, { t: "int", v: newSalt }, { t: "address", v: account1 }, { t: "uint", v: time }, { t: "uint", v: currentRoundId }, { t: "bytes32", v: identifier } );

@chrismaree while unlikely, one minor issue with the concatenation approach is that it technically can create hash collisions between different inputs:

price1 = 15 salt1 = 100 price1 + salt1 = 15100 price2 = 1 salt2 = 5100 price2 + salt2 = 15100

I think fixed length inputs are always safest for this reason. We should have the inputs to the hash always include all their bits, even if there are leading or trailing zeros.

great point @mrice32

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

mrice32 · 2020-04-10T17:36:01Z

Follow-up PR will add vote timestamp, price identifier, and round ID to the vote commit.

I know this might make this a huge PR, but can we add them all in one PR? Just so it's easier for OZ to review?

mrice32

Looks awesome! Thanks for tackling this. Like I mentioned in my above comment, I think we should probably go ahead and implement all the suggested additions to the hash in this PR just so it's easier to review the fix.

mrice32 · 2020-04-10T17:48:04Z

-        // Committed hash doesn't match revealed price and salt
-        require(keccak256(abi.encode(price, salt)) == voteSubmission.commit, "Invalid commit hash & salt");
+        // Committed hash doesn't match revealed voter address, price and salt
+        require(keccak256(abi.encodePacked(price, salt, msg.sender)) == voteSubmission.commit, "Invalid commit hash, address & salt");


My understanding is that encode and encodePacked differ for types that are less than 32 bytes or dynamically sized. Since an address is less than 32 bytes, I'm guessing it's creating the problem. See here for the solidity docs around this. In the docs for soliditySha3, they say that they "tightly pack" the arguments before hashing. So I think this is actually more correct than it was before.

Now that we're tossing more types in here, we may need to be more careful about how we compute soliditySha3 offchain. web3 attempts to autodetect types, but it's not perfect. Since solidity knows the types, if web3 autodetects incorrectly, it could lead to hash differences due to differences in encoding. To solve this problem, web3 offers a way to provide the types of the arguments passed to soliditySha3 (see examples here). We don't need to do this in this PR, but it's something we may consider doing in the future to make sure these hashes are always computed in exactly the same way.

nicholaspai · 2020-04-10T18:05:51Z

Follow-up PR will add vote timestamp, price identifier, and round ID to the vote commit.

I know this might make this a huge PR, but can we add them all in one PR? Just so it's easier for OZ to review?

That's exactly why I didn't put them in this PR haha. How about I'll add them after an initial round of reviews

mrice32 · 2020-04-10T18:08:03Z

Follow-up PR will add vote timestamp, price identifier, and round ID to the vote commit.

I know this might make this a huge PR, but can we add them all in one PR? Just so it's easier for OZ to review?

That's exactly why I didn't put them in this PR haha. How about I'll add them after an initial round of reviews

Good idea. Let's do that.

mrice32 · 2020-04-10T18:08:48Z

@ptare could you take a pass on this PR?

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

nicholaspai · 2020-04-10T18:55:16Z

    const newPrice = getRandomSignedInt();
    const newSalt = getRandomUnsignedInt();
-    const newHash = web3.utils.soliditySha3(newPrice, newSalt);
+    const newHash = web3.utils.soliditySha3(


@mrice32 Added an example of overriding soliditySha3 type autodetection, but will hold off on making sweeping changes across repo until another PR

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

ptare

Change looks good to me. But wouldn't this change have the same issue as PR #1231 in that it breaks the voter dapp? Is my understanding correct?

nicholaspai · 2020-04-13T15:05:11Z

Change looks good to me. But wouldn't this change have the same issue as PR #1231 in that it breaks the voter dapp? Is my understanding correct?

Yes this definitely breaks the voter-dapp, and I've made the neccessary changes in voter-dapp/src/ActiveRequests.

ptare · 2020-04-13T15:07:24Z

Right, but don't we need the old voter-dapp in order to vote on DVM upgrades?

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

nicholaspai · 2020-04-13T15:10:05Z

Right, but don't we need the old voter-dapp in order to vote on DVM upgrades?

Ah yes I see the problem now. Can we create two sites?

ptare · 2020-04-13T15:14:55Z

We'll have to, it looks like, since this is a high(ish) severity issue.

Here's what I recommend:

Remove ActiveRequests.js from this PR
Let's figure out how we want to do two sites or at least freeze the voter dapp
Then we can submit this PR and also [P1-N15] Remove EncryptedStore and emit an extra event instead #1231

nicholaspai · 2020-04-13T16:31:14Z

We'll have to, it looks like, since this is a high(ish) severity issue.

Here's what I recommend:

Remove ActiveRequests.js from this PR

Let's figure out how we want to do two sites or at least freeze the voter dapp

Then we can submit this PR and also [P1-N15] Remove EncryptedStore and emit an extra event instead #1231

Sounds like this and #1231 will be blocked by a PR to come that will freeze the voter-dapp site

mrice32

Looks great! Two minor comments/nits.

mrice32 · 2020-04-13T19:53:18Z

    assert(await didContractThrow(voting.revealVote(identifier, time, newPrice, newSalt)));
  });

-  it("Multiple voters", async function() {


nit: why was this test deleted?

I deleted this test because it doesn't really add anything to the previous test. The "Multiple voters" test checks that different voters can't reveal each other's votes, but this is implied from the previous test which checks that the hashed account is correct.

There's one other check in that test: that a voter who hasn't committed can't reveal. Are we testing that anywhere else?

Isn't that the same thing as the test above? If a voter has not committed, then their account component of the hash must fail (revealVote checks msg.sender).

I see, yeah, agreed.

mrice32 · 2020-04-13T19:57:07Z

    /**
     * @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 `address`, `price` and `salt` must match the latest `hash` that `commitVote()` was called with.


Should this comment also include roundId, identifier, time, etc? Or are those implied?

yup good catch

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

mrice32

LGTM!

chrismaree

This looks excellent to me, great work!

chrismaree · 2020-04-14T06:50:23Z

    assert(await didContractThrow(voting.revealVote(identifier, time, price, newSalt)));

+    // Can't reveal with the incorrect address.
+    assert(await didContractThrow(voting.revealVote(identifier, time, newPrice, newSalt, { from: account2 })));


This is great. Thanks for increaseing the code coverage here.

chrismaree · 2020-04-14T06:58:33Z

  for (var i = 0; i < requestNum; i++) {
    const salt = getRandomUnsignedInt();
-    const hash = web3.utils.soliditySha3(price, salt);
+    const hash = computeVoteHash({


+1 for being thorough here.

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

nicholaspai added 9 commits April 8, 2020 16:23

make propose() payable and check that sender sent enough ETH

c08b313

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

Merge branch 'master' of https://github.com/UMAprotocol/protocol into…

dfea6e1

… governor-payable-proposals

make executeProposal payable instead of propose

5eae521

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

Merge branch 'master' of https://github.com/UMAprotocol/protocol into…

3c77390

… governor-payable-proposals

require exact amount of ETH on executeProposal

6fe11f1

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

add test for transcations that don't need ETH

d330425

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

revealVote checks if caller's address is same as included in hash

5bd6ee8

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

Merge branch 'master' into vote-duplication

67bc431

lint

e72e945

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

nicholaspai added the audit-fix-phase1 Fixes for Phase 1 (DVM) of the OZ audit label Apr 10, 2020

typo

e088bb6

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

nicholaspai commented Apr 10, 2020

View reviewed changes

fix governor test

043ea6b

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

mrice32 reviewed Apr 10, 2020

View reviewed changes

nicholaspai marked this pull request as draft April 10, 2020 18:06

mrice32 requested a review from ptare April 10, 2020 18:08

mrice32 assigned ptare and mrice32 Apr 10, 2020

nicholaspai mentioned this pull request Apr 10, 2020

Force client scripts hashing data via web3.utils.soliditySha3(...) to specify the type of data #1219

Closed

nicholaspai added 2 commits April 10, 2020 14:43

Merge branch 'master' into vote-duplication

84fc522

merge master, add one example of overriding soliditySha3 autodetect type

97a325e

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

nicholaspai commented Apr 10, 2020

View reviewed changes

nicholaspai added 2 commits April 10, 2020 14:58

lint

256aa39

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

revealVote checks roundId, pricerequest-timestamp, and identifier

70764a5

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

nicholaspai changed the title ~~[P1-H01] Revealing votes requires caller's address to match hashed address~~ [P1-H01] Require additional data in vote commit hash to prevent duplication of votes Apr 12, 2020

use computeVoteHash throughout repo

ee517a1

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

ptare reviewed Apr 13, 2020

View reviewed changes

Comment thread core/contracts/oracle/implementation/Voting.sol

add comment

390dab2

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

nicholaspai marked this pull request as ready for review April 13, 2020 16:53

mrice32 reviewed Apr 13, 2020

View reviewed changes

nicholaspai added 2 commits April 13, 2020 16:22

Merge branch 'master' into vote-duplication

28ebc01

fix comment

c244084

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

mrice32 approved these changes Apr 14, 2020

View reviewed changes

chrismaree approved these changes Apr 14, 2020

View reviewed changes

nicholaspai and others added 3 commits April 14, 2020 08:23

Update core/contracts/oracle/implementation/Voting.sol

613c074

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

Update core/contracts/oracle/implementation/Voting.sol

f7c0524

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

Update core/contracts/oracle/implementation/Voting.sol

0b1baf0

Co-Authored-By: Chris Maree <christopher.maree@gmail.com>

nicholaspai merged commit 50a88a3 into master Apr 14, 2020

nicholaspai deleted the vote-duplication branch April 14, 2020 13:13

nicholaspai mentioned this pull request Apr 27, 2020

Clarify copy in Voting.revealVote #1301

Merged

Conversation

nicholaspai commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 commented Apr 10, 2020

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Apr 10, 2020

Uh oh!

mrice32 commented Apr 10, 2020

Uh oh!

mrice32 commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicholaspai commented Apr 13, 2020

Uh oh!

ptare commented Apr 13, 2020

Uh oh!

nicholaspai commented Apr 13, 2020

Uh oh!

ptare commented Apr 13, 2020

Uh oh!

nicholaspai commented Apr 13, 2020

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

chrismaree left a comment

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Apr 10, 2020 •

edited

Loading

mrice32 Apr 10, 2020 •

edited

Loading

mrice32 Apr 10, 2020 •

edited

Loading