Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix proposalInfo mapping #597

Merged
merged 3 commits into from
Feb 13, 2019
Merged

Fix proposalInfo mapping #597

merged 3 commits into from
Feb 13, 2019

Conversation

leviadam
Copy link
Contributor

@leviadam leviadam commented Feb 10, 2019

This PR is fixing #596.

The problem was that the mapping was from proposalId => proposalInfo. The reason that it cannot be part of the mapping which also contains the avatar, is that the result from the call from the voting machine does not contain the avatar.
This can be utilized by a malicious voting machine to overwrite another proposal. Note that it is not enough just to check that the proposalInfo is empty as it will be easy to predict the next IDs (the hash is deterministic) and freeze the DAO.

The solution is to have a mapping per voting machine, thus, if a DAO is working with a legit voting machine, its proposals are safe.

@orenyodfat
Copy link
Contributor

How about adding additional require before setting avatar->proposalId->proposal ?
To make sure it cannot be overwrite by any voting which sneaked in (not likely to happened )

@leviadam
Copy link
Contributor Author

I'm not sure about that @orenyodfat.
I think an organization must make sure the voting machine is producing random hashes. If this is not the case, then if you either throw or not does it not matter. If you throw, a DAO can be freezed. If you do not, a DAO's proposal can be over-written. I think freezing is worst.

@leviadam leviadam merged commit 5fd969d into master Feb 13, 2019
@leviadam leviadam deleted the fixProposalInfo branch February 13, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants