how to build a better democracy in under a 100 lines of code [closed]

Question

Hello friends after three days of research and a lot of help of community I solved my problem with the code of democracy smart contract following these tutorial: https://blog.ethereum.org/2015/12/04/ethereum-in-practice-part-2-how-to-build-a-better-democracy-in-under-a-100-lines-of-code/

I have the crash named "Return value of low-level calls not used": Return value of low-level calls not used

So i solve the problem with Rob Hitchens advice and i make my code like that

pragma solidity 0.4.8;
contract token { mapping (address => uint256) public balanceOf; }

/* The democracy contract itself */
contract Democracy {

    /* Contract Variables and events */
    uint public minimumQuorum;
    uint public debatingPeriodInMinutes;
    Proposal[] public proposals;
    uint public numProposals;
    token public sharesTokenAddress;

    event ProposalAdded(uint proposalID, address recipient, uint amount, string description);
    event Voted(uint proposalID, bool position, address voter);
    event ProposalTallied(uint proposalID, int result, uint quorum, bool active);

    struct Proposal {
        address recipient;
        uint amount;
        string description;
        uint votingDeadline;
        bool openToVote;
        bool proposalPassed;
        uint numberOfVotes;
        bytes32 proposalHash;
        Vote[] votes;
        mapping (address => bool) voted;
    }

    struct Vote {
        bool inSupport;
        address voter;
    }

    /* modifier that allows only shareholders to vote and create new proposals */
    modifier onlyShareholders {
        if (sharesTokenAddress.balanceOf(msg.sender) == 0) throw;
        _;
    }

    /* First time setup */
    function Democracy(token sharesAddress, uint minimumSharesForVoting, uint minutesForDebate) {
        sharesTokenAddress = token(sharesAddress);
        if (minimumSharesForVoting == 0 ) minimumSharesForVoting = 1;
        minimumQuorum = minimumSharesForVoting;
        debatingPeriodInMinutes = minutesForDebate;
    }

    /* Function to create a new proposal */
    function newProposal(address beneficiary, uint etherAmount, string JobDescription, bytes transactionBytecode) onlyShareholders returns (uint proposalID) {
        proposalID = proposals.length++;
        Proposal p = proposals[proposalID];
        p.recipient = beneficiary;
        p.amount = etherAmount;
        p.description = JobDescription;
        p.proposalHash = sha3(beneficiary, etherAmount, transactionBytecode);
        p.votingDeadline = now + debatingPeriodInMinutes * 1 minutes;
        p.openToVote = true;
        p.proposalPassed = false;
        p.numberOfVotes = 0;
        ProposalAdded(proposalID, beneficiary, etherAmount, JobDescription);
        numProposals = proposalID+1;
    }

    /* function to check if a proposal code matches */
    function checkProposalCode(uint proposalNumber, address beneficiary, uint etherAmount, bytes transactionBytecode) constant returns (bool codeChecksOut) {
        Proposal p = proposals[proposalNumber];
        return p.proposalHash == sha3(beneficiary, etherAmount, transactionBytecode);
    }

    /* */
    function vote(uint proposalNumber, bool supportsProposal) onlyShareholders returns (uint voteID){
        Proposal p = proposals[proposalNumber];
        if (p.voted[msg.sender] == true) throw;

        voteID = p.votes.length++;
        p.votes[voteID] = Vote({inSupport: supportsProposal, voter: msg.sender});
        p.voted[msg.sender] = true;
        p.numberOfVotes = voteID +1;
        Voted(proposalNumber,  supportsProposal, msg.sender);
    }

    function executeProposal(uint proposalNumber, bytes transactionBytecode) returns (int result) {
        Proposal p = proposals[proposalNumber];
        /* Check if the proposal can be executed */
        if (now < p.votingDeadline  /* has the voting deadline arrived? */
            || !p.openToVote        /* has it been already executed? */
            ||  p.proposalHash != sha3(p.recipient, p.amount, transactionBytecode)) /* Does the transaction code match the proposal? */
            throw;

        /* tally the votes */
        uint quorum = 0;
        uint yea = 0;
        uint nay = 0;

        for (uint i = 0; i <  p.votes.length; ++i) {
            Vote v = p.votes[i];
            uint voteWeight = sharesTokenAddress.balanceOf(v.voter);
            quorum += voteWeight;
            if (v.inSupport) {
                yea += voteWeight;
            } else {
                nay += voteWeight;
            }
        }
        /* execute result */
        if (quorum > minimumQuorum && yea > nay ) {
            // has quorum and was approved
            if(p.recipient.call.value(p.amount*1000000000000000000)(transactionBytecode)) {
        // do something
         p.openToVote = false;
    } else {
        // do something else
         p.proposalPassed = true;
    }
        } else if (quorum > minimumQuorum && nay > yea) {
            p.openToVote = false;
            p.proposalPassed = false;
        }
        /* Fire Events */
        ProposalTallied(proposalNumber, result, quorum, p.openToVote);
    }
}

so the system accept it. I want to ask is the code is good for deploy the contract or i make a mistake here

Thanks in advance!

Answer 1

Caution re:

good for deploy the contract or i make a mistake here

I suspect it's unrealistic to ask users on this forum to certify the code is free of defects. I see what appear to be departures from best practices and areas of concern. So, Not yet.

I agree with Richard, in that pursuing the points raised below will greatly help your understanding.

I've elaborated my answer at Return value of low-level calls not used. Answered the original question (roughly, why is the compiler complaining?) and provided a little more guidance about // do something // do something else. In summary, use care and observe best practices described here: https://github.com/ConsenSys/smart-contract-best-practices

Under the heading External Calls you'll find a pattern labeled "Bad" and it appears to match your code.

So, re-entrance is a concern because I see state variable manipulation after the external call.

Another concern is the presence of a for loop.

for (uint i = 0; i <  p.votes.length; ++i) {

While this is possibly mitigated somewhere else in the code, if so, it isn't obvious, at least to me. I don't see anything that limits p.votes.length. That would mean no limit to the number of iterations over the loop. That would lead to a function that can't do it's job over a certain number of votes due to the block gasLimit. Are we to understand that if the contract is successful and there are many voters, the contract is supposed to break? Obviously, no, so I would need to understand why that won't happen before I could be comfortable with this.

Another concern is the (clever?) use of transactionByteCode. It seems to provide a great deal of flexibility; possibly too much flexibility. It appears to increase the attack surface area and test cases. With that move, things transition from a finite list of things the contract does, to a possibly limitless list of things the ByteCode might do.

The problem with this sort of thing is anticipating how it might be used and abused in the testing stage, and how vulnerabilities may not surface until there is considerable value entrusted to the contract and attackers are motivated to find a weakness. As a shorthand, there's a world of difference between asking here on SE and putting a million dollar bounty on the question. I would place the burden of proof on proponents of the strategy. Don't worry about proving there's a way to hack it. Someone needs to prove there is no way to hack it.

I see now that you're following a tutorial. A casual review suggests that the post has not been updated since 2015, well before the 2016 DAO hack. Also, it has not been updated to address the compiler issue that was first reported in your other post. These observations suggest caution using this example.

Considering the source, it's possible others have taken the time to really understand what the contract does each step of the way. It's possible each of my three concerns can be set aside if someone chimes in with a satisfactory explanation of how the contract is defensive.

I agree with Richard. You should always know what your contract does. You can't be too cautious about a piece of software that can't be modified. I found three concerns with only a casual glance at the code, so I am far from comfortable with expressing confidence that everything is ready.

It might be better to view the example as a demonstration of Ethereum's potential but not as a real-world battle-ready solution.

Hope it helps.