StdDaoToken vulnerability fix: startNewVoting() and finishVoting() ca… #209
Conversation
…n be called BY ANYONE! #202
contracts/tokens/StdDaoToken.sol
Outdated
| super.finishEvent(_votingID); | ||
| emit VotingFinished(msg.sender, _votingID); | ||
| } | ||
|
|
||
| function isContract(address addr) returns (bool) { |
There was a problem hiding this comment.
Это метод для проверки адреса, является ли он контрактом. Ты имеешь ввиду что я его расположил неудачно?
contracts/tokens/StdDaoToken.sol
Outdated
| @@ -59,17 +62,28 @@ contract StdDaoToken is DetailedERC20, PausableToken, CopyOnWriteToken, ITokenVo | |||
| // ITokenVotingSupport implementation | |||
| // TODO: VULNERABILITY! no onlyOwner! | |||
| function startNewVoting() public whenNotPaused returns(uint) { | |||
| require (isContract(msg.sender)); | |||
There was a problem hiding this comment.
На самом деле это не защищает ни от чего. Ну создаст атакующий просто контракт, который нас дернет)
There was a problem hiding this comment.
Ну это защищает от вызова простого аккаунта. Я пока не вижу других решений
contracts/tokens/StdDaoToken.sol
Outdated
| emit VotingStarted(msg.sender, idOut); | ||
| return idOut; | ||
| } | ||
|
|
||
| // TODO: VULNERABILITY! no onlyOwner! | ||
| function finishVoting(uint _votingID) whenNotPaused public { | ||
| require (votingOwners[_votingID] == msg.sender); |
There was a problem hiding this comment.
Лучше сделать модификатор отдельный для этого имхо
|
@RostyslavBortman Тесты не проходят. Нужно поправить |
| @@ -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); | |||
There was a problem hiding this comment.
Почему тут 0? А что если голосование не первое запущенное. Это не будет работать, правильно?
There was a problem hiding this comment.
Да, правильно. Это заглушка чтобы не лезть дальше
| @@ -59,17 +76,43 @@ contract StdDaoToken is DetailedERC20, PausableToken, CopyOnWriteToken, ITokenVo | |||
| // ITokenVotingSupport implementation | |||
| // TODO: VULNERABILITY! no onlyOwner! | |||
| function startNewVoting() public whenNotPaused returns(uint) { | |||
| uint idOut = super.startNewEvent(); | |||
| require (isContract(msg.sender)); | |||
There was a problem hiding this comment.
isContract() нужно убрать. Это ни от чего не защищает
There was a problem hiding this comment.
Ну в этом весь смысл таска. Тогда таск получается бесполезен, или я чего то не увидел/не понял
There was a problem hiding this comment.
Точнее не таска, а того что мы обсуждали. Возможно стоит подумать еще, каким образом можно еще сделать проверку. Но это уже больше похоже на этот таск
#210
| emit VotingFinished(msg.sender, _votingID); | ||
| } | ||
|
|
||
| function startNewEvent() whenNotPaused public returns(uint){ |
There was a problem hiding this comment.
Эти методы не должны быть тут.
Клиент контракта StdDaoToken не знает понятия "Event" (это понятие только низлежащего контракта CopyOnWriteToken)
Зачем ты вынес отдельные startnNewEvent/finishEvent и EventStarted/EventFinished сюда тоже не понятно. Нужно это убрать.
There was a problem hiding this comment.
" учти, что закрыть нужно еще и startNewEvent и finishEvent (переопределить их, а не прямо в CopyOnWriteToken!) " => Я их переопределил же
| @@ -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; | |||
There was a problem hiding this comment.
Ты добавил ВО ВСЕ методы Voting'ов _votingID. Так быть не должно:
- путается понятие "голосование" (сам адрес контракта Voting) и понятие "индекс голосования в токене" (то что называется _votingID)
- Не понятно, как ОДИН voting может быть связан с несколькими "индексами голосований в токене"
- Чисто логически не понятно, почему Voting не может ВНУТРИ себя хранить всю нужную инфу (допустим даже votingID), а должен ее откуда-то получать
- Кто и где хранит тогда votingID?
Короче, это очень плохое решение
| @@ -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; | |||
There was a problem hiding this comment.
Как понять логику "в одном контракте Voting может быть МНОЖЕСТВО привязанных индексов голосований токена"? Зачем тут uint => mapping появился?
There was a problem hiding this comment.
Ну типо, у одного инстанса Voting может быть 20 votings с разными ID
…n be called BY ANYONE! #202