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.

Sign up for GitHub

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

StdDaoToken vulnerability fix: startNewVoting() and finishVoting() ca… #209

Closed
wants to merge 4 commits into from
Closed

StdDaoToken vulnerability fix: startNewVoting() and finishVoting() ca… #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0865fa9
StdDaoToken vulnerability fix: startNewVoting() and finishVoting() ca…
RostyslavBortman Jul 20, 2018
a37d3c5
create modifier onlyVotingOwner, make isContract method internal
RostyslavBortman Jul 20, 2018
1e93bb7
StdDaoToken vulnerability fix: startNewVoting() and finishVoting()
RostyslavBortman Jul 23, 2018
2cd650b
Merge branch 'dev2' into dev2Rostyslav4
RostyslavBortman Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/DaoBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ contract DaoBase is IDaoBase, Ownable {
if(isCan){
bool isVotingFound = false;
bool votingResult = false;
(isVotingFound, votingResult) = store.getProposalVotingResults(_a);
(isVotingFound, votingResult) = store.getProposalVotingResults(_a,0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему тут 0? А что если голосование не первое запущенное. Это не будет работать, правильно?

Copy link
Contributor Author

@RostyslavBortman RostyslavBortman Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, правильно. Это заглушка чтобы не лезть дальше


if(isVotingFound){
// if this action can be done by voting, then Proposal can do this action
Expand Down
4 changes: 2 additions & 2 deletions contracts/DaoStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,12 @@ contract DaoStorage is DaoStorageGroups {
return proposals.length;
}

function getProposalVotingResults(address _p) public constant returns (bool isVotingFound, bool votingResult){
function getProposalVotingResults(address _p, uint _votingID) public constant returns (bool isVotingFound, bool votingResult){
// scan all votings and search for the one that is finished
for(uint i=0; i<proposals.length; ++i){
if(proposals[i]==_p){
IVoting voting = proposals[i].getVoting();
return (true, voting.isFinished() && voting.isYes());
return (true, voting.isFinished(_votingID) && voting.isYes(_votingID));
}
}
return (false,false);
Expand Down
18 changes: 9 additions & 9 deletions contracts/governance/IVoting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ pragma solidity ^0.4.22;
contract IVoting {
// _tokenAmount -> if this voting type DOES NOT use tokens -> set to any value (e.g., 0);
// will execute action automatically if the voting is finished
function vote(bool _yes, uint _tokenAmount) public;
function vote(bool _yes, uint _tokenAmount, uint _votingID) public;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ты добавил ВО ВСЕ методы Voting'ов _votingID. Так быть не должно:

  1. путается понятие "голосование" (сам адрес контракта Voting) и понятие "индекс голосования в токене" (то что называется _votingID)
  2. Не понятно, как ОДИН voting может быть связан с несколькими "индексами голосований в токене"
  3. Чисто логически не понятно, почему Voting не может ВНУТРИ себя хранить всю нужную инфу (допустим даже votingID), а должен ее откуда-то получать
  4. Кто и где хранит тогда votingID?
    Короче, это очень плохое решение


// stop the voting
function cancelVoting() public;

// This is for statistics
// Can get this stats if voting is finished.
// Can get this stats if voting is NOT finished.
function getVotingStats() public view returns(uint yesResults, uint noResults, uint totalResults);
function getVotingStats(uint _votingID) public view returns(uint yesResults, uint noResults, uint totalResults);

// Is voting finished?
//
Expand All @@ -29,7 +29,7 @@ contract IVoting {
// When isFinished():
// 1 - i can not vote any more
// 2 - i can get results with isYes()
function isFinished()public view returns(bool);
function isFinished(uint _votingID)public view returns(bool);

// The result of voting
//
Expand All @@ -38,7 +38,7 @@ contract IVoting {
// 2 - all these conditions should be met:
// 2.1 - isFinished()
// 2.2 - is quorum reached
function isYes()public view returns(bool);
function isYes(uint _votingID)public view returns(bool);
}

/**
Expand All @@ -52,16 +52,16 @@ contract IDelegationTable {
// @dev delegateMyVoiceTo() will override the currently set value.
// I.e., if you called delegateMyVoiceTo(A,10) and then delegateMyVoiceTo(A,20) again,
// then getDelegatedPowerByMe(A) is 20 (not 30!)
function delegateMyVoiceTo(address _to, uint _tokenAmount) public;
function delegateMyVoiceTo(address _to, uint _tokenAmount, uint _votingID) public;
// @dev Returns how much i delegated power to _to
function getDelegatedPowerByMe(address _to) public view returns(uint);
function getDelegatedPowerByMe(address _to, uint _votingID) public view returns(uint);
// @dev Cancel the delegation of power to _to
function removeDelegation(address _to) public;
function removeDelegation(address _to, uint _votingID) public;

// @dev Returns the sum of: balance of _who AND the total delegated power of _who
function getPowerOf(address _who) public view returns(uint);
function getPowerOf(address _who, uint _votingID) public view returns(uint);
// @dev Returns the total delegated power of _who
function getDelegatedPowerOf(address _of) public view returns(uint);
function getDelegatedPowerOf(address _of, uint _votingID) public view returns(uint);
}


2 changes: 1 addition & 1 deletion contracts/governance/Voting_1p1v.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import "zeppelin-solidity/contracts/ownership/Ownable.sol";
* @dev This is the implementation of IVoting interface. Each Proposal should have voting attached.
* If group members change -> it will not work
*/
contract Voting_1p1v is IVoting, Ownable {
contract Voting_1p1v is Ownable {
// use DaoClient instead?
// (it will handle upgrades)
IDaoBase dao;
Expand Down
78 changes: 39 additions & 39 deletions contracts/governance/Voting_Liquid.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract LiquidVoting is IDelegationTable, Voting_SimpleToken {
event DelegatedTo(address _sender, uint _tokensAmount);
event DelegationRemoved(address _from, address _to);

mapping (address => Delegation[]) delegations;
mapping (uint => mapping(address => Delegation[])) delegations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как понять логику "в одном контракте Voting может быть МНОЖЕСТВО привязанных индексов голосований токена"? Зачем тут uint => mapping появился?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну типо, у одного инстанса Voting может быть 20 votings с разными ID


constructor(IDaoBase _dao, IProposal _proposal,
address _origin, uint _minutesToVote,
Expand All @@ -27,94 +27,94 @@ contract LiquidVoting is IDelegationTable, Voting_SimpleToken {
{
}

function internalVote(address _who, bool _yes) internal {
uint tokenBalance = getPowerOf(_who);
function internalVote(address _who, bool _yes, uint _votingID) internal {
uint tokenBalance = getPowerOf(_who, _votingID);

require(!addressVotedAlready[_who]);
require(!addressVotedAlready[votingID][_who]);

tokenVotesArray.push(TokenVote(_who, _yes, tokenBalance));
addressVotedAlready[_who] = true;
tokenVotesArray[_votingID].push(TokenVote(_who, _yes, tokenBalance));
addressVotedAlready[votingID][_who] = true;

emit VotingSimpleToken_Vote(_who, _yes, tokenBalance);

callActionIfEnded();
callActionIfEnded(_votingID);
}

function getPowerOf(address _who) public view returns(uint){
uint res = stdDaoToken.getBalanceAtVoting(votingID, _who);
function getPowerOf(address _who, uint _votingID) public view returns(uint){
uint res = stdDaoToken.getBalanceAtVoting(_votingID, _who);

for(uint i = 0; i < delegations[_who].length; i++){
if(!delegations[_who][i].isDelegator){
res += delegations[_who][i].amount;
for(uint i = 0; i < delegations[_votingID][_who].length; i++){
if(!delegations[_votingID][_who][i].isDelegator){
res += delegations[_votingID][_who][i].amount;
}
if(delegations[_who][i].isDelegator){
res -= delegations[_who][i].amount;
if(delegations[_votingID][_who][i].isDelegator){
res -= delegations[_votingID][_who][i].amount;
}
}

return res;
}

function getDelegatedPowerOf(address _of) public view returns(uint) {
function getDelegatedPowerOf(address _of, uint _votingID) public view returns(uint) {
uint res;

for(uint i = 0; i < delegations[_of].length; i++){
if(!delegations[_of][i].isDelegator){
res += delegations[_of][i].amount;
for(uint i = 0; i < delegations[_votingID][_of].length; i++){
if(!delegations[_votingID][_of][i].isDelegator){
res += delegations[_votingID][_of][i].amount;
}
}

return res;
}

function getDelegatedPowerByMe(address _to) public view returns(uint) {
function getDelegatedPowerByMe(address _to, uint _votingID) public view returns(uint) {
uint res;

for(uint i = 0; i < delegations[msg.sender].length; i++){
if(delegations[msg.sender][i]._address == _to){
if(delegations[msg.sender][i].isDelegator){
res += delegations[msg.sender][i].amount;
for(uint i = 0; i < delegations[_votingID][msg.sender].length; i++){
if(delegations[_votingID][msg.sender][i]._address == _to){
if(delegations[_votingID][msg.sender][i].isDelegator){
res += delegations[_votingID][msg.sender][i].amount;
}
}
}

return res;
}

function delegateMyVoiceTo(address _to, uint _tokenAmount) public {
function delegateMyVoiceTo(address _to, uint _tokenAmount, uint _votingID) public {
require (_to!= address(0));
require (_tokenAmount <= stdDaoToken.getBalanceAtVoting(votingID, msg.sender));

for(uint i = 0; i < delegations[_to].length; i++){
if(delegations[_to][i]._address == msg.sender){
delegations[_to][i].amount = _tokenAmount;
for(uint i = 0; i < delegations[_votingID][_to].length; i++){
if(delegations[_votingID][_to][i]._address == msg.sender){
delegations[_votingID][_to][i].amount = _tokenAmount;
}
}

for(i = 0; i < delegations[msg.sender].length; i++){
if(delegations[msg.sender][i]._address == _to){
delegations[msg.sender][i].amount = _tokenAmount;
for(i = 0; i < delegations[_votingID][msg.sender].length; i++){
if(delegations[_votingID][msg.sender][i]._address == _to){
delegations[_votingID][msg.sender][i].amount = _tokenAmount;
emit DelegatedTo(_to, _tokenAmount);
return;
}
}

delegations[_to].push(Delegation(msg.sender, _tokenAmount, false));
delegations[msg.sender].push(Delegation(_to, _tokenAmount, true));
delegations[_votingID][_to].push(Delegation(msg.sender, _tokenAmount, false));
delegations[_votingID][msg.sender].push(Delegation(_to, _tokenAmount, true));
}

function removeDelegation(address _to) public {
function removeDelegation(address _to, uint _votingID) public {
require (_to!= address(0));

for(uint i = 0; i < delegations[_to].length; i++){
if(delegations[_to][i]._address == msg.sender){
delegations[_to][i].amount = 0;
for(uint i = 0; i < delegations[_votingID][_to].length; i++){
if(delegations[_votingID][_to][i]._address == msg.sender){
delegations[_votingID][_to][i].amount = 0;
}
}

for(i = 0; i < delegations[msg.sender].length; i++){
if(delegations[msg.sender][i]._address == _to){
delegations[msg.sender][i].amount = 0;
for(i = 0; i < delegations[_votingID][msg.sender].length; i++){
if(delegations[_votingID][msg.sender][i]._address == _to){
delegations[_votingID][msg.sender][i].amount = 0;
}
}

Expand Down
Loading