blockchainsllc / dao Goto Github PK

Changelog

• 16 - 05 - 2016: Issue created
• 13 - 06 - 2016: Modified issue to turn it into a DIR

Problem Definition

As of DAO v1.0 getting out of the DAO and reclaiming your remaining ether requires the user to perform a split. This is a very long process which is cumbersome and very difficult to follow for non-technical people. Plus there is a possibility for other users to follow in a split and try to make your life difficult as defined in the Stalker Attack.

Proposed Solution

The solution is straightforward. A function allowing instant withdrawal from the DAO needs to be added for the user. This function should burn the user's tokens and return to him the remaining ether. An implementation of this can be seen here.

An alternative implementation which also allows for changing votes during the voting process can be seen here

Consider the scenario:

1. DAO A spends money on a proposal
2. DAO B splits from DAO A. This means that DAO B is added to DAO A's rewardToken mapping. When ETH is transferred to the DAOrewardaccount, DAO B can collect their share.
3. DAO C splits from DAO B. This change is not reflected in DAO A's rewardToken mapping. DAO B's rewardToken mapping is updated to assign rewards to DAO C in proportion to the amount of ETH they spent from DAO B. In the extreme case, if the token holders of DAO C never spent any money in DAO B, they are not represented in DAO B's rewardToken mapping. DAO C will correctly receive their share of rewards at the time of the split, but any future rewards that they are entitled to from DAO A are now lost forever (or more accurately - their rewards are (incorrectly) distributed among the DAO B token holders)

The solution is for the DAO to record the number of tokens it has distributed to child DAOs. In this way, the relevant proportion of tokens belonging to DAO B and DAO C are publicly queryable from any contract. DAO A can then include a mechanism (that anyone can call) to add DAO C to its rewardToken mapping, while reducing the rewards owed to DAO B appropriately. This solution has the added advantage that descendant DAOs can claim their rewards directly from the DAO that earned them, without having to go through the intermediate DAOs.

As of now, I don't see the possibility of merging existing DAOs, although if I'm wrong, please correct me. The only possibility I see, is if people split from their existing DAOs, get a refund, and then reinvest in a whole new DAO, they are able to pseudo-merge.

However, what would be the viability of implementing a merge function, is it possible to keep most of the current code intact or would this require a rewrite? The merge should only be possible between 2 identical DAO contracts.

If you have tests add any CI service to test the PRs.

1. I create a proposal with a huge deposit say 1000 ETH
2. I split the DAO Which this sets p.splitData[0].splitBalance = this.balance effectively. this.balance includes my 1000 ETH

fundsToBeMoved = (balances[msg.sender] * p.splitData[0].splitBalance) / p.splitData[0].totalSupply;

1. I get 1000 times my original ETH.
2. My proposal meets the quorum and i get my ETH deposit back

Is this where proposal deposits are stored?

All the variables and functions that individual users can change should be defined in a separate contract for transfer* newProposal vote executeProposal splitDAO retrieveDAOReward getMyReward withdrawRewardFor halveMinQuorum

DAO.sol should define the functions and variables that a Majority of the Users can changes. Changing proposal deposit changeProposalDeposit, newContract

'Curators.sol' should define the functions and variables that the Curator can change. changeAllowedRecipients halveMinQuorum

This is a good idea because

1. It will make reviewing a little simpler as you can easily confirm that all the functions in Curator.sol are only accessible to the curators. Also the files will be smaller and considering the modules separately can make things less complex.
2. There are many use cases where only changes to UserState.sol would be required.
3. Although this reduces the readability a little, it make the understanding the framework and its implications easier. If forces the reader to consider that it is basically a state machine, some variables can be changed by individuals, some by the majority (if you don't like a change you can split to a new DAO which is initialized with a certain state based upon 'splitDAO') this is a very powerful paradigm. These changes will allow developers to more easily leverage the DAO framework.

If someone votes on a proposal that has a malformed hash. Then

if ... ... ... || p.proposalHash != sha3(p.recipient, p.amount, _transactionData)) { throw; }

will cause their tokens to be blocked forever. Preventing them from splitting or transferring.

Is there a need to add curator to allowedRecipients

The curator can call changeAllowedRecipients(address_of_dao, false). After that the DAO cannot receive rewards because it cannot call retrieveDAOReward(). The Token holders will need to split before receiving rewards.

As it stands, after a call to newContract(), the old DAO tokens must be traded in for new contract tokens. This involves exchanges having to change the address of the token contract, a potential time lag for people moving to the new system (perhaps missing out on reward tokens), and general headache.

This will be unavoidable for the first upgrade, however one solution could be moving the tokens to a separate sub-contract. This contract could have special owner only-functions for burning or blocking tokens. Ownership of this sub-contract could then be passed from contract to contract as part of a call to newContract.

I have written the following JS:

var fs = require("fs");
var process = require("process");
var solc = require("solc");

var files = { };
for (var i = 2; i < process.argv.length; i++) {
    var file = process.argv[i];
    files[file] = fs.readFileSync(file).toString("utf-8");
}

var compiled = solc.compile({ sources: files });

console.log(compiled);

and placed it at the root of the repo as verify.js. I have done npm install solc to get the dependency. When I run node verify.js *.sol libs/*.sol, I get the following errors:

{ errors: 
   [ 'PFOffer.sol:245:16: Error: Operator > not compatible with types bool and uint256\n            && votingDeadline > now\n               ^------------------^\n',
     'PFOffer.sol:255:9: Error: Wildcard both at beginning and end of variable declaration list is only allowed if the number of components is equal.\n        var (,,,votingDeadline,,,,,,yea,nay,) = client.proposals(proposalID);\n        ^------------------------------------------------------------------^\n' ] }

python test.py --scenario deploy
Compiling the DAO contracts...
Compiling /home//DAO/DAOcopy.sol...
Compiling /home/DAO/SampleOffer.sol...
Creating deploy.js
Traceback (most recent call last):
File "test.py", line 265, in
ctx.run_test(args)
File "test.py", line 256, in run_test
self.run_scenario(self.args.scenario)
File "test.py", line 246, in run_scenario
scenario.run(self)
File "/home/__/DAO/tests/scenarios/deploy/run.py", line 30, in run
cb_before_creation=calculate_closing_time
File "test.py", line 212, in create_js_file
write_js("{}.js".format(name), s, len(self.accounts))
AttributeError: TestContext instance has no attribute 'accounts'

Currently on proposal creation, a proposalHash is created via sha3(_recipient, _amount, _transactionData) to enforce that the right code is called during a call to executeProposal(). While this might save space, it also makes it difficult when browsing proposals to see what _transactionData consisted of without examining the original transaction that created the proposal.

_transactionData should be added as a field to the Proposal struct, or at the very least, the data should be propagated by the ProposalAdded() event, so that various automated tools in use by the community can make it available (dao.report et al)

I don't think you need to increment proposals.lenght here as its already done here

If i am correct and this function is used to create the new split DAO then there should be no + 42 days for crowd sourcing.

function createNewDAO(address _newServiceProvider) internal returns (DAO _newDAO) { NewServiceProvider(_newServiceProvider); return daoCreator.createDAO(_newServiceProvider, 0, now + 42 days); }
Creating a new DAO with 42 days of crowd sourcing time would allow anyone to buy into it and then get a share in the original splitters profits. This should be now + 0 days. It will also result in the new DAO not being able to make any proposals for this time.

There may be a problem on https://github.com/slockit/DAO/blob/master/DAO.sol#L545If two groups are splitting into two separate DAO's the one that calls splitDAO function first will end up with less ETH in their DAO at the end. Due to p.splitData[0].totalSupply = totalSupply; being set at the time it is first called and not updated as tokens are burnt.
uint fundsToBeMoved = (balances[msg.sender] * p.splitData[0].splitBalance) / p.splitData[0].totalSupply; if (p.splitData[0].newDAO.buyTokenProxy.value(fundsToBeMoved)(msg.sender) == false) throw;

https://github.com/slockit/DAO/blob/master/DAO.sol#L552

Say the DAO has 100 DAO and 100 eth. And it splits into 2 DAO's
GROUP 1:
Calls splitDAO at the start so its totalSupply=100. Then all its the person withdraws their ETH in one go. rewardTokenToBeMoved = 50 x 50/100 = 25
GROUP 2:
DAO2 calls split DAO at after all this has happened totalSupply=50. Then the person withdraws their ETH in one go. rewardTokenToBeMoved = 50 x 50/50 = 50.
Something is not quite right in the number here. total should be 100. But it might help to illustrate what I am getting at. DAO splits in parallel may be broken.

As it currently stands, a proposal to send money to the extra balance would work. This should not be allowed.

I suspect that it maybe, understandably, too late for this suggestion for The DAO but maybe something for another future DAO.

I'm new to Solidity and the DAO but as I have reviewed all of the DAO code over the weekend and while there is not that much code it's pretty easy to do something incorrect and the more complex, the more chances of missing something critical that may put much at risk.

Again, it's probably too late for this for THE DAO and if it were suggested to the wider token holder audience it would probably produce outrage (Disclosure: I am a DAO token HODLER) but I'm going to ask anyway: Can we not allow splits at all? This would simplify SO much. Current corporations don't allow the equivalent of a DAO split. A corporation obtains cash from various source of funding and returns from their projects. As a shareholder of a corporation you cannot simply ask/propose for your portion of cash and for your portion of the upside for past funded projects. If you want cash, you sell your shares and you are done - simple. One other way shareholders get cash is if there is a dividend distribution but it is distributed out to all shareholders equally.

This would simplify the DAO tremendously, making it more maintainable and safer too (no stalker attack, etc). It would allow for the removal of all the split logic and the baggage that goes with it like maintaining state like reward tokens, splitData, etc. If token holders want/need ETH, they can sell their tokens on an exchange or propose a dividends proposal where a % of DAO funds would be distributed to all token holders equally - this would require small amount of code but would be much simpler, something line this:

my dividend = (dividend % * (DAO.balance + extraBalance - sumOfProposalDeposits)) * (balance[me] / totalSupply)

Just a thought...

US legal hackers/researchers new to this code base looking to add more test scenarios (here: https://github.com/slockit/DAO/tree/master/tests). Clues/cautions/ideas/help are welcome.

Context: This DAO code seem to fit a law.mit.edu autonomous Credit Union prototype. The prototype would test a few things, including how well Solidity can meet a few common regulatory compliance requirements. We are looking at some data provided by Solidity (and maybe better with this DAO design) that regulators/auditors can use to objectively, verifiably test legal compliance with requirements not covered by the current test scenarios. Seems the best next step is to create them and test it out, right?

I invite anybody familiar with this DAO project to come hack the law (in a good way!) with us on a prototypejam hangout or skype sesh. HT to @ peterchau (in DOAhub slack) who suggested I inquire with this issue.

Since the quorum takes both yea and nay votes into account, there may be cases where the quorum is just barely reached, and would not have been reached without the nay-voters.

Because of this people who want to vote against a proposal are disincentivized to do so. This can be easily fixed by only counting the yea votes of a proposal towards its quorum.

I'm new to Solidity and just took first pass of all the DAO code, so I'm not 100% sure but I think this may be a bug.

Let's say that we have halved the minimum quorum by doubling the minQuorumDivisor (=10) because it took a long time for a quorum to be met. So looking at the code below which is part of the DAO.executeProposal() function , the first 'if' statement checks that we've met the lower halved quorum requirement. We enter that first 'if' statement and it modifies the minQuorumDivisor back to higher 20% requirement (minQuorumDivisor=5). Then immediately after we attempt to execute the proposal where there's a second 'if' statement checking to see ii the quorum is met again but now it has the higher quorum requirement since DAO.minQuorum() is a function of the minQuorumDivisor that was just previously reset to 20%. So, isn't it possible that the in some situations that proposal never gets executed?

if (quorum >= minQuorum(p.amount)) {
   ...
  if (quorum > totalSupply / 5)
    minQuorumDivisor = 5;
 }

// Execute result
if (quorum >= minQuorum(p.amount) && p.yea > p.nay && proposalCheck) {
  ...
}

Me and @Hiro_Mant had a chat in #art_of_the_DAO about implementing liquid democracy in "user land" with a separate contract that acts as intermediary.

Basically a DTH send there tokens to this contract and defines a second delegate DTH. The contract then "watches" the DAO and votes the same way the delegate votes. If the delegate choses to split the contract splits but withdraws the original users ETH to their account. This provides the same level of security as the original contract. White reducing the need for active voters.

This would require making a contract delegate.sol with this functionality. A user would then deploy this and set it up to delegate their vote.

I think this is an elegant way to implement complex features like liquid democracy with minimum changes to the base DAO.sol contract.

It is currently 20. This is a problem because in a few years ETH may be worth 1000 USD and in the case where someone wants to withdraw their ETH they will split the DAO and the proposal to withdraw money will require 20*1000 = 20,000 USD. Most people do not have this ETH available and will thus not be able to withdraw it.

You can withdraw negative amount of money from SampleOffer.getDailyPayment() because the calculations ignores historical changes to dailyWithdrawLimit.

Use case:

user has ether balance in address A
user has dao tokens in address B (transfer) C (transfer/voting) D (voting)

it would be super if the contract be passed a message signed by B, C, D to transfer dao tokens from the address, and use A's ether balance to pay for the mining fee.

We can expect better decisions if more people vote. Mandatory voting is one way to ensure that voting happens. Australia and some S. American countries do it. Either vote or forfeit 1 DAO token or some small percentage of your DAO tokens.

One way to address vacations and voter fatigue is to allow DAO holders to nominate some other holder to vote for them ("liquid democracy"). There is already an issue and pull request for something similar.

python test.py --solc /usr/bin/solc --geth /usr/bin/geth --clean-chain --deploy-sale-seconds 10 --scenario deploy
Cleaning blockchain data directory ...
Creating accounts and genesis block ...
Done!
Compiling the DAO contracts...
    Compiling /home/__/slock.it/DAO/DAOcopy.sol...
    Compiling /home/__/slock.it/DAO/SampleOffer.sol...
Creating deploy.js
Traceback (most recent call last):
  File "test.py", line 266, in <module>
    ctx.run_test(args)
  File "test.py", line 257, in run_test
    self.run_scenario(self.args.scenario)
  File "test.py", line 247, in run_scenario
    scenario.run(self)
  File "/home/__/DAO/tests/scenarios/deploy/run.py", line 30, in run
    cb_before_creation=calculate_closing_time
  File "test.py", line 211, in create_js_file
    substitutions = cb_before_creation(self, name, substitutions)
  File "/home/__/DAO/tests/scenarios/deploy/run.py", line 13, in calculate_closing_time
    obj.closing_time = seconds_in_future(obj.args.deploy_sale_seconds)
  File "/home/__/DAO/tests/utils.py", line 64, in seconds_in_future
    return ts_now() + secs
TypeError: unsupported operand type(s) for +: 'int' and 'str'

I have a fix will submit pull request.

1) Minor change for clarity. The comment below is misleading because calling newContract() does not necessarily require 53%. It's actually from 33% to a maximum of 53%. That's because minQuorumDivisor can tend toward 0 if a quorum is not met over a long period of time. Current code has:

====> // require 53% for calling newContract()
if (_transactionData.length >= 4 && _transactionData[0] == 0x68
    && _transactionData[1] == 0x37 && _transactionData[2] == 0xff
    && _transactionData[3] == 0x1e
    && quorum < minQuorum(actualBalance() + rewardToken[address(this)])) {

        proposalCheck = false;
}

2) There seems to be something strange in that a newContract() is not not called but you can get into a situation where the DAO sends proposalDeposits to itself. In addition, when the the minQuorum (between 33% and 53%) is not met for calling newContract() but we can still have a situation where we reset the divisor and last time quorum was met:

// require 53% for calling newContract()
if (_transactionData.length >= 4 && _transactionData[0] == 0x68
    && _transactionData[1] == 0x37 && _transactionData[2] == 0xff
    && _transactionData[3] == 0x1e
    && quorum < minQuorum(actualBalance() + rewardToken[address(this)])) {

        proposalCheck = false;

       ===> The newContract will not get executed if we dont reach 53%
       ===> (given a minQuorumDivisor of 5)
}

if (quorum >= minQuorum(p.amount)) {

    ===> Although we haven't reached 53% 
    ===> we may have reached 21% (for example), so we are here 
    ===> where we send ourselves proposalDeposit and spend gas. 

   if (!p.creator.send(p.proposalDeposit))
        throw;

    lastTimeMinQuorumMet = now;
    // set the minQuorum to 20% again, in the case it has been reached
    if (quorum > totalSupply / 5)

        ===> We are in this if statement since we reached 21%.
        ===> It seems strange that we have reset our divisor and 
        ===> lastTimeMinQuorum was met although we will not be executing a
        ===> proposal in this thread of execution. ***

        minQuorumDivisor = 5;
}

Use case:
I have voted with my "The DAO" account (A1) I buy more tokens, transfer them to A1. Now they are locked, and they shouldn't be. The locking mechanism seems too primitive:

mapping (address => uint) public blocked;

I can see in the source code that the blocking map discriminates on the account level. It would be ideal to map the addresses to a count (blockedNumberOfTokens)

The same approach should be used for proposals. If I vote from A1 for P5 with 1000 tokens, and then acquire more tokens (let's say 500) into A1, I should be able to vote again for P5, and the DAO contract should increase the votes on P5 and the lockedNumberOfTokens to 1500 for A1.

I haven't had the time to look too much at the code and I'm quite new to the EVM, so I have to make a disclaimer that I don't know if this feature is technically viable.

After updating the Ethereum Wallet to Mist Mode. I am having issues when attempting to test out the voting. Everything works fine all the way up to the time to vote. It brings up the options. I select a vote, but after that when it is supposed to prompt me for my password. It does nothing.

Any suggestions on what I have done wrong?

Defeat the quorum rules by calling newContract() with a users address sending a little gas to this contract and withdrawing all the ETH.

totalSupply / minQuorumDivisor+(_value * totalSupply) / (3 * (actualBalance() + totalRewardToken));1

Here where _value should equal totalSupply as it is all ETH that is being moved. It will only equal the gas of the transaction. Thus what used to be a 51% attack becomes a 20% attack.

Okay lets say you and me are in the DAO together. I own 51 tokens and you own 49. I stop you from doing anything so you vote to change service provider. I vote with you and we are still in the same position. I follow you to the new DAO.

So lets say you split your tokens and take 1 token of them and create a new service provider. Then I take 1.01 of my tokens and agree with you to switch service providers with you, following you. Then you are in the same position unable to withdraw your ETH. That leaves me owning 49.99 and you with 48 in the original DAO and me with 1.01 and you with 1 in the new DAO.

So we continue this you trying to run away and me following you until we have split the DAO to its ultimate granularity which is 1 WEI each (if i am not mistaken). But at this stage fees will have eaten all our ETH. But some people will make a deal with me to get their ETH in exchange for theirs.

At any time I can split the DAO and withdraw my ETH. Letting people have theirs back. But only after made some profit.

Token.approve(...,uint256 _amount) and Token.approveAndCall(..., uint256 _amount, ...) should both check that _amount > 0 to keep the allowed[][] mapping clean and to avoid the possibility of introducing any overflow issues in the future.

Can we call newProposal so many times that _proposalID wraps around. Then we can call newProposal and change the creator of proposals with high deposits. Then we can call executeProposal and get the proposal deposit sent to us.

I believe that the requirement that msg.value > 0 prevents a proposal deposit for new newCurator proposals.

In combination with condition ' || (msg.value < proposalDeposit && !_newCurator)) {'

it will prevent anyone from changing curator once the proposal deposit has been set to non-zero

The vote function does not return (_voteID) even though it says it supposed to.

I'm new to Solidity so maybe there is a default return from every function even if it's not explicitly made in the function but I couldn't find it anywhere in the docs.

function vote(
    uint _proposalID,
    bool _supportsProposal
) onlyTokenholders noEther returns (uint _voteID)
{
   [... _voteID not assigned nor do we return anything from here ...]
}

Someone can execute a proposal being used to split the DAO

That will close the proposal here then if an attacker calls isBlocked using the spitting user address their isBlocked Id will be set to 0 because the proposal is closed. Then blocked[msg.sender] != _proposalID will prevent this user from splitting.

Termination clauses for contractors are frequently symmetrical and may require x days of warning. Sometimes they are terminate at will.

Seems like contractors should have a function similar to returnRemainingMoney() that indicates that they are terminating the relationship with the DAO. This might be necessary in cases like a crash in the value of Ether leading to an inability of the contractor to adequately compensate workers for an extended period of time:
https://github.com/slockit/DAO/blob/master/SampleOffer.sol#L92

Splitting was conceived and implemented as a last resort to leave the DAO in the case of a malicious curator, but it seems that people also start seeing it as a way to disagree with a proposal. Some people may want to leave in the case a proposal goes through.

With DAO v1.0 there would be a race condition, since right after the proposal debate period ends, the proposal can be immediately executed. So there is not enough time to split before people know the outcome of the vote.

Although it is likely that the result will be obvious before the last second of the debate period, technically it is also possible for all votes to happen in the last block before the period ends, introducing an uncertainty to potential splitters.

This uncertainty can be easily fixed by setting a Grace Period between the end of the proposal debate period and the execution of the proposal so that people who really want to split because of the result of a vote can wait until the end of this vote to do so.

If the proposal Deposit is included in the DAO then a malicious attacker could set the limit prohibitively high to prevent splits.

if (address(p.splitData[0].newDAO) == 0)
  throw;
// p.proposalDeposit should be zero here
if (this.balance < p.proposalDeposit)
  throw;
p.splitData[0].splitBalance = this.balance - p.proposalDeposit;
p.splitData[0].rewardToken = rewardToken[address(this)];
p.splitData[0].totalSupply = totalSupply;
p.proposalPassed = true;

if (now < closingTime + 40 days) here
should be if (now < closingTime) There are no errors introduced by this because it is double checked in buyTokenProxy

I believe that in SampleOffer.sol, in the sign() method (line 81) the signature should not be accepted if total cost is not transfered OR is already signed.

In the code there is an AND ( && ) instead of an OR ( || ). If I understand the contract, this is a bug and this should be an OR ( || ).

msg.sender != address(client) already prevented by callingRestriction modifier so should not be called a second time in the if statement.

function setRewardDivisor(uint _rewardDivisor) callingRestriction {
        if (_rewardDivisor < 50 && msg.sender != address(client))
            throw; // 2% is the default max reward
        rewardDivisor = _rewardDivisor;
    }

Can you write comments in the code please? For those of us who don't code, please write what each variable means.

Because executeProposal takes _transactionData as a function input and not from p._transactionData here and here

There is no problem here. However because sha3 concatenates without padding an attacker can inject a malicious _transactionData into the executeProposal function along as it is concentrated with p.recipient and p.amount the check will catch it. But I would be a little more comfortable if we used p._transactionData here.

Is there a reason we take _transactionData as input?

I am a little worried instead of making 1 proposal to spend all the ETH of the DAO. Someone can make 20. In the case where the this.balance = 100 ETH the quorum will be 20% on 20 proposals to spend 5 ETH and can be passed by a minority to spend all the ETH.

Defeating the quorum rules that more users need to agree in order to pass a proposal that spends more ETH.

It is just an idea.

How about allowing the Client and the Contractor changing the address of the Contractor. That can be useful in something happens to the old account, the Contractor wants to upgrade, the DAO wants to change the contractor.

Changelog

• 23 - 05 - 2016 Issue created
• 13 - 06 - 2016 Turned the issue into a DAO Improvement Request

Problem Description

For the original DAO deployment the notion of an extraBalance account was used in order to keep the extra funds sent to the DAO by potential DAO Token Holders after the lowest price DAO creation phase was over.

This was due to initial requests by the community to provide for variable price on the DAO tokens during the creation period. The existence of the extraBalance and the fact that the DAO needs to spend as much as is in the extraBalance to be able to access it creates a lot of complications both during the creation phase of the DAO and after.

Proposed Solution.

Completely Remove the extraBalance from the code. Here is the PR that provides the implementation for this: #244

I'm trying to get the historical votes of the proposals in the same way:
https://etherscan.io/token/TheDAO#votes

But there isn't a way. I tried handling events in Geth console:

> var theDAOVotedEvent = theDAO.Voted({fromBlock: 1600769, toBlock:1618798});

> theDAOVotedEvent.watch(function(error, result){
  console.log(JSON.stringify(result));
});

But it only reports the new votes but I can't get the old ones. I read the code and it makes sense since the only place Voted event is called is in vote function.
I also tried to replace .watch with .get just in case, but I don't get any vote with this change.

Is there any way to access Proposal.votedYes and proposal.votedNo or any other way to get the same
information?

Thanks on advanced,
Pablo Yabo

Calling transferFromWithoutReward allows an attacker to call the withdrawRewardFor which is an internal function and send the rewards of any user to that users account.

This can be scripted away to make a large % of every users rewards be charged as fees for these micro withdrawals.