Repository

https://github.com/igormuba/EthereumSolidityClasses/tree/master/class22

What Will I Learn?

• Fix for 2 exploits on the multisig contract upgrader(changer)
• Halting users accounts
• Blockig halted users to send tokens/vote/submit changes

Requirements

• Internet connection
• Code editor
• Browser

Difficulty

• Advanced

Tutorial Contents

On the previous two tutorials, we have turned an ERC20 token contract into an ERC223 token contract. But what we have built was not a simple ERC223 contract, but a mutable ERC223 one, by mutable I mean that the code of the contract can be "changed" down the road. In case a new exploit or bug is discovered, it is a nice feature to be able to fix your contract, although, contracts are, by default immutable.

After implementing the mutability in our contract, I have explained about the "moral" discussion behind mutable contracts. Some people argue that it is not morally correct giving the power to the developer to change the contract in the future however he likes. While I abstain from this discussion on my tutorials, I have shown how can you give your contract such a feature while still pleasing, even if just a little, both sides of the discussion. The solution is implemented by requiring a consensus of the token holders before a change is approved.

So, the creator of the contract/app can deploy the original code, but we have designed it that he can only deploy it once, and any new change proposals require that the token holders vote on it, so any change would only be made if the holders agree with it.

But that generates a few issues and exploits. Today I am going to cover a little bit of an exploit that would allow the same tokens to vote multiple times. Again, as I have explained in other tutorials, but I need to say again to make it clear, we are working with lots of contracts that connect to each other in too many ways, and bugs are inherent to such a design.

While it would be impossible for me to cover every single "failure point" of the design, I can cover the biggest one. In a real contract using this design, you would need at least 1 week of bug hunting and exploit fixing for each day of coding, as each new contract and interaction adds another level of complexity, which makes the difficulty of pen testing exponentially harder.

Halting voter tokens

A simple solution I came out with, to avoid users from exploiting the system and voting multiple times with the same tokens by transferring it to a new account, is to halt the tokens of an account.

This is a similar feature from the ICO contract we have implemented in another class.

For this, I will introduce a mapping that stores the information of whether the users account is halted or not. We want to not allow him to vote and transfer tokens if his account is halted, so, on the ERC223Variables.sol contract we add the mapping:

mapping (address=>bool) private haltedAccount;//mapping of halted accounts

And the function to halt, "unhalt" and check the status of the account.

Halting an account:

    //halts an account
    function haltAccount(address _haltedAccount) public{
        require(msg.sender==changeManagerAddress);//requires caller change manager
        require(!haltedAccount[_haltedAccount]);//requires account is not yet halted
        haltedAccount[_haltedAccount]=true;//sets the account as halted
    }

In this case, we want the code to throw an error if, for whatever reason, the function tries to halt an already halted account, because obviously, if someone is trying to halt an already halted account probably he is trying to exploit something and do something he shouldn't be doing, that is why the "require(!haltedAccount[_haltedAccount])".

On the function to release an account from the halt we do something similar:

    //unhalts an account
    function unhaltAccount(address _haltedAccount) public{
        require(msg.sender==changeManagerAddress);//requires caller change manager
        require(haltedAccount[_haltedAccount]);//requires account is halted
        haltedAccount[_haltedAccount]=false;//frees the account from the halt
    }

But on the example above we confirm that the account is actually halted.

Now, we just need to implement the view function, that is gonna be used on a modifier, that will throw an error is a halted user tries to vote or transfer his coins.

    //checks halt status of an account
    function checkAccountHalt(address _haltedAccount) public view returns(bool){
        require(msg.sender==changeManagerAddress);//requires caller change manager
        return haltedAccount[_haltedAccount];//returns the status of the account
    }

Custom modifier to deny halted accounts

Now, on the change manager contract, we will add a custom modifier. This modifier will be used in functions on the change manager that requires an user to not have a halted account.
The modifier:

    //modifier to block halted accounts
    modifier notHalted {
        bool isHalted = variablesManager.checkAccountHalt(msg.sender); //loads the halt status
        require (!isHalted);//throws if account is halted
        _; //means this modifier is executed before the code of the function
    }

Now, still on the change manager, we will implement the logic that checks the halt status of the user before calling the functions to create proposals, and the logic that halts the accounts after a vote is casted. We won't, however, implement is this tutorial the logic to unhalt accounts to keep things simple. Unhalting accounts is not that simple because it is not possible to iterate through a mapping, so that would require a deeper refactor of the code to implement address tracking, and is, for now, out of the scope of the class.

On the function transferChangeProposal of the change manager, add the modifier we have just created notHalted, so it looks like:

function transferChangeProposal(address newTransferManager) public notHalted returns (uint thisProposalId){}

And on the else, in case the caller does not have enough balance to approve a change, add the logic to call the halt function from the variables manager:

        if(myVote>(variablesManager.totalSupply()/2)){
            proposals[_currentId].approved=true;
            coreManager.setTransferManager(newTransferManager, address(this));
            variablesManager.setTransferManager(newTransferManager);
        }else{
            proposals[_currentId].approved=false;
            variablesManager.haltAccount(msg.sender);//halts user account if not enough balance
        }

Same thing applies to the approveChange function.

Avoiding halted users to transfer tokens

The first exploit, above, was the one that allowed users vote more than once on the same account, and was more obvious. The second exploit I am covering on this section is more subtle. If a halted user transfers his tokens to another account, that would allow him to vote with the same tokens but from multiple accounts, maybe allowing him to approve the change of a malicious code.

To fix it, on the transfer manager contract we also need to implement some logic to prevent halted users from making transfer. It is not difficult, just need to implement the same custom modifier from the change manager and add it to the 4 transfer functions in that contract.

The only exception is the transferFrom function with data. This function already has a very complex logic and too many variables. Chances are that when you add that modifier there you will face an error saying "The stack is too deep".

The fix for the above issue is to simply implement the logic from the modifier to the first line of code without creating any new variables:

function transferFrom(address _caller, address _sender, address _receiver, uint _amount) public notHalted returns (bool){
        require(!variablesManager.checkAccountHalt(msg.sender));
//more code below hidden...
}

Testing

Let us now see if the user indeed can't vote more than once. I have deployed the contracts as always, so I won't go in too deep on the deployment process. I just want to say that you need to be aware of all connections between the contracts. The reason why we allow the creator to deploy and set the initial contracts and connect them without a consensus is that, without the proper initial configuration the code simply won't work.

Using a secondary account, not the owner, the user can submit a change proposal, as expected.

However, if the same person tries to approve anything else, or submit any other proposal, Ethereum will throw, as the users account is halted.

Curriculum

First tutorial

• (Part 1) Ethereum Solidity Development - Getting Started + Lower Level Explanation (PT 1)

Latest tutorial

• (Part 21) Ethereum Solidity - Allowing 2 Types Of New Contract And Introduction To Bug Hunting(PT 21)

Classes related to this one

• (Part 20) Ethereum Solidity - Decentralizing Contract Mutability, Multisig Vulnerabilities(PT 20)
• (Part 19) Ethereum Solidity - ERC223 Mutability, Mutable Contract Implementation And Hacking Mutable Contracts(PT 19)

Beneficiaries

This post has as beneficiaries
@utopian.pay with 5%
using the SteemPeak beneficiary tool