[P1-N10] Warn users against re-using salts #1273

nicholaspai · 2020-04-22T17:07:35Z

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

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

mrice32

Looks awesome! Thanks for adding this. Just a few minor comments.

mrice32 · 2020-04-22T17:21:22Z

core/contracts/oracle/implementation/Voting.sol

@@ -358,6 +358,8 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface {
     * @notice Reveal a previously committed vote for `identifier` at `time`.
     * @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.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * it assumes that voters will never reuse the same pair of salt and price. Otherwise their commits could be easily disclosed in advance.


I think we should rephrase this slightly to discourage voters from reusing the same salt twice, not just salt-price pair. Since an attacker may have some ability to predict the price for a particular vote, so a known salt and an unknown price wouldn't offer that much protection.

mrice32 · 2020-04-22T17:22:07Z

core/contracts/oracle/interfaces/VotingInterface.sol

+     * @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.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * it assumes that voters will never reuse the same pair of salt and price. Otherwise their commits could be easily disclosed in advance.


Same comment as above.

mrice32 · 2020-04-22T17:22:45Z

documentation/oracle/dvm_interfaces.md

@@ -153,7 +153,8 @@ Reveals a vote that the voter committed to during the commit period.
 This method takes the `identifier` and `time` to identify the price request.

 So the reveal can be verified on-chain, the voter must also provide the `price` and `salt` that they used to compute
-the `hash` that they passed to `commitVote`.
+the `hash` that they passed to `commitVote`. Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+it assumes that voters will never reuse the same pair of salt and price. Voters should never reuse pairs of salt and price or their commits can be disclosed in advance, and to be safe voters should avoid reusing salts in general.


Same comment as above.

mrice32 · 2020-04-22T17:23:06Z

documentation/oracle/dvm_interfaces.md

@@ -153,7 +153,8 @@ Reveals a vote that the voter committed to during the commit period.
 This method takes the `identifier` and `time` to identify the price request.

 So the reveal can be verified on-chain, the voter must also provide the `price` and `salt` that they used to compute
-the `hash` that they passed to `commitVote`.
+the `hash` that they passed to `commitVote`. Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,


nit: maybe we should also put this warning in the commitVote section.

good idea done.

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

nicholaspai · 2020-04-22T17:34:37Z

documentation/oracle/dvm_interfaces.md

-`keccak256(price, salt)`, where price is the `int256` price value that they wish to submit and `salt` is a random
-`int256` value. The voter must remember the `price` and `salt` that they submitted so they can reveal their commit
-later - otherwise, the commit cannot be revealed and the vote won't be counted.
+`keccak256(price, salt, time, address, roundId, identifier)`, where price is the `int256` price value that they wish to submit, `time` is the unix timestamp of the request being voted on, `address` is the voter's address (technically, the same one that will reveal the vote), `roundId` is the current voting round, `identifier` is the pricefeed identifier relevant to the price request, and `salt` is a random `int256` value. The voter must remember the inputs to the vote `hash` that they submitted so they can reveal their commit later - otherwise, the commit cannot be revealed and the vote won't be counted.


@rcai1 I changed this description to commitVote to reflect that the vote hash now requires additional information.

Makes sense!

rcai1

LGTM

mrice32

Just a few small language suggestions.

documentation/oracle/dvm_interfaces.md

mrice32 · 2020-04-22T18:01:55Z

core/contracts/oracle/interfaces/VotingInterface.sol

+     * that `commitVote()` was called with. Only the committer can reveal their vote.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then
+     * they can easily reveal the vote.


Suggested change

* they can easily reveal the vote.

* they can easily determine the vote pre-reveal.

mrice32 · 2020-04-22T18:02:40Z

core/contracts/oracle/implementation/Voting.sol

@@ -358,6 +358,9 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface {
     * @notice Reveal a previously committed vote for `identifier` at `time`.
     * @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.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then
+     * they can easily reveal the vote.


Suggested change

* they can easily reveal the vote.

* they can easily determine the vote pre-reveal.

mrice32 · 2020-04-22T18:03:09Z

core/contracts/oracle/implementation/Voting.sol

@@ -358,6 +358,9 @@ contract Voting is Testable, Ownable, OracleInterface, VotingInterface {
     * @notice Reveal a previously committed vote for `identifier` at `time`.
     * @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.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then


Suggested change

* voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then

* voters should never reuse salts. If someone else is able to guess the voted price and knows that a salt will be reused, then

mrice32 · 2020-04-22T18:03:33Z

core/contracts/oracle/interfaces/VotingInterface.sol

+     * @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.
+     * @dev Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior,
+     * voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then


Suggested change

* voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then

* voters should never reuse salts. If someone else is able to guess the voted price and knows that a salt will be reused, then

mrice32 · 2020-04-22T18:04:24Z

documentation/oracle/dvm_interfaces.md

+`keccak256(price, salt, time, address, roundId, identifier)`, where price is the `int256` price value that they wish to submit, `time` is the unix timestamp of the request being voted on, `address` is the voter's address (technically, the same one that will reveal the vote), `roundId` is the current voting round, `identifier` is the pricefeed identifier relevant to the price request, and `salt` is a random `int256` value. The voter must remember the inputs to the vote `hash` that they submitted so they can reveal their commit later - otherwise, the commit cannot be revealed and the vote won't be counted.
+
+Since transaction data is public, the salt will be revealed along with the vote. While this is the system’s expected behavior,
+voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then


Suggested change

voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then

voters should never reuse salts. If someone else is able to guess the voted price and knows that a salt will be reused, then

add comments about salt-reuse

66dd280

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 22, 2020

nicholaspai requested review from hlambur and rcai1 April 22, 2020 17:07

mrice32 reviewed Apr 22, 2020

View reviewed changes

edit comments

69a1ede

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

nicholaspai commented Apr 22, 2020

View reviewed changes

rcai1 approved these changes Apr 22, 2020

View reviewed changes

chrismaree approved these changes Apr 22, 2020

View reviewed changes

nicholaspai merged commit dd20eec into master Apr 22, 2020

nicholaspai deleted the comment-salt-reuse branch April 22, 2020 18:02

mrice32 reviewed Apr 22, 2020

View reviewed changes

nicholaspai mentioned this pull request Apr 22, 2020

Move salt reuse warnings to commitVote, refine language #1276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P1-N10] Warn users against re-using salts #1273

[P1-N10] Warn users against re-using salts #1273

nicholaspai commented Apr 22, 2020

mrice32 left a comment

mrice32 Apr 22, 2020

nicholaspai Apr 22, 2020

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

nicholaspai Apr 22, 2020

nicholaspai Apr 22, 2020

rcai1 Apr 22, 2020

rcai1 left a comment

mrice32 left a comment

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

mrice32 Apr 22, 2020

	* they can easily reveal the vote.
	* they can easily determine the vote pre-reveal.

	* voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then
	* voters should never reuse salts. If someone else is able to guess the voted price and knows that a salt will be reused, then

	voters should never reuse salts. If someone else is able to predict the voted price or knows that a price will be reused, then
	voters should never reuse salts. If someone else is able to guess the voted price and knows that a salt will be reused, then

[P1-N10] Warn users against re-using salts #1273

[P1-N10] Warn users against re-using salts #1273

Conversation

nicholaspai commented Apr 22, 2020

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcai1 left a comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment