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

Security Considerations #64

Open
D4nte opened this issue Sep 14, 2021 · 5 comments
Open

Security Considerations #64

D4nte opened this issue Sep 14, 2021 · 5 comments

Comments

@D4nte
Copy link

D4nte commented Sep 14, 2021

Few issues I noticed when looking at the security of the SDK.

I started to write a document: https://github.com/status-im/dappconnect-voting-sdk#security-considerations

1. Election start

if an account calls initializeVotingRoom with voteAmount: 0 then it can starts an election without holding any token.
Only token holder MUST be able to start an election.

2. Cross-contract votes

As far as I can see, a vote can be re-used for 2 different voting contract (same or different ERC-20 tokens) as the vote only the voter account, room id (indexed per voting contract), voting value and token amount.

The voting contract address is NOT part of the vote. I believe it must be added to stop users re-using votes for election that have same index across different contracts (e.g. user participate to 2 different DAOs for 2 different ERC20 token that use this SDK).

@Szymx95
Copy link
Contributor

Szymx95 commented Sep 14, 2021

2. Cross-contract votes

As far as I can see, a vote can be re-used for 2 different voting contract (same or different ERC-20 tokens) as the vote only the voter account, room id (indexed per voting contract), voting value and token amount.

The voting contract address is NOT part of the vote. I believe it must be added to stop users re-using votes for election that have same index across different contracts (e.g. user participate to 2 different DAOs for 2 different ERC20 token that use this SDK).

Contract address is used in signature EIP712 domain separator therefore vote can't be reused for different voting contract because the signature won't match
EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)

@Szymx95
Copy link
Contributor

Szymx95 commented Sep 14, 2021

1. Election start

Good catch
Fixed in PR #65

2. Cross-contract votes

As I said in comment before, contract address is used in EIP712Domain so vote can't be reused in different contract, i added a test to verify that in PR #66

@D4nte
Copy link
Author

D4nte commented Sep 15, 2021

Please see more properties at https://github.com/status-im/dappconnect-voting-sdk

@D4nte
Copy link
Author

D4nte commented Sep 15, 2021

Contract address is used in signature EIP712 domain separator therefore vote can't be reused for different voting contract because the signature won't match
EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)

what is verifyingContract? the voting contract? Maybe best to rename to votingContract then?
What are name and version? Also, shouldn't the election index in the votingContract be included?

@D4nte
Copy link
Author

D4nte commented Sep 15, 2021

#66 looks good, I think it's a question of terminology.

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

No branches or pull requests

2 participants