Simple mistake leads to 30M+ USD theft

A simple bug in the multisig-wallet contract deployed by the ethereum-wallet parity just netted some attacker over 30M USD (153k ETH).

How was this possible?

The contract that was used for the multisig-wallet was missing a very simple check: Do not allow anyone to call the function initWallet if it already has been called.

This is the relevant parts of the solidity-code:

// constructor - just pass on the owner array to the multiowned and // the limit to daylimit function initWallet(address[] _owners, uint _required, uint _daylimit) { initDaylimit(_daylimit); initMultiowned(_owners, _required); } // constructor - stores initial daily limit and records the present day's index. function initDaylimit(uint _limit) { m_dailyLimit = _limit; m_lastDay = today(); } // constructor is given number of sigs required to do protected "onlymanyowners" transactions // as well as the selection of addresses capable of confirming them. function initMultiowned(address[] _owners, uint _required) { m_numOwners = _owners.length + 1; m_owners[1] = uint(msg.sender); m_ownerIndex[uint(msg.sender)] = 1; for (uint i = 0; i < _owners.length; ++i) { m_owners[2 + i] = uint(_owners[i]); m_ownerIndex[uint(_owners[i])] = 2 + i; } m_required = _required; } 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 // constructor - just pass on the owner array to the multiowned and // the limit to daylimit function initWallet ( address [ ] _owners , uint _required , uint _daylimit ) { initDaylimit ( _daylimit ) ; initMultiowned ( _owners , _required ) ; } // constructor - stores initial daily limit and records the present day's index. function initDaylimit ( uint _limit ) { m_dailyLimit = _limit ; m_lastDay = today ( ) ; } // constructor is given number of sigs required to do protected "onlymanyowners" transactions // as well as the selection of addresses capable of confirming them. function initMultiowned ( address [ ] _owners , uint _required ) { m_numOwners = _owners . length + 1 ; m_owners [ 1 ] = uint ( msg . sender ) ; m_ownerIndex [ uint ( msg . sender ) ] = 1 ; for ( uint i = 0 ; i < _owners . length ; ++ i ) { m_owners [ 2 + i ] = uint ( _owners [ i ] ) ; m_ownerIndex [ uint ( _owners [ i ] ) ] = 2 + i ; } m_required = _required ; }

It is important to remember that functions in Solidity are by default public and can be called from any address with arguments set by the sender.

As you can see there are no checks to prevent an attacker from calling initWallet and re-assigning the owner-addresses of the contract… And that is exactly what happened.

After changing the owner of the contract the attacker controlled the wallet and could send out over 153k ETH from three large wallets, with a current value over 30M USD to another address.

The fix to the contract was really simple; Setting initMultiowned and initDaylimit to internal (this prevents them from being called from external sources) and protecting initWallet with a new modifier “only_uninitialized”.

Relevant code after security-fix:

// throw unless the contract is not yet initialized. modifier only_uninitialized { if (m_numOwners > 0) throw; _; } // constructor - just pass on the owner array to the multiowned and // the limit to daylimit function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized { initDaylimit(_daylimit); initMultiowned(_owners, _required); } // constructor - stores initial daily limit and records the present day's index. function initDaylimit(uint _limit) internal { m_dailyLimit = _limit; m_lastDay = today(); } // constructor is given number of sigs required to do protected "onlymanyowners" transactions // as well as the selection of addresses capable of confirming them. function initMultiowned(address[] _owners, uint _required) internal { m_numOwners = _owners.length + 1; m_owners[1] = uint(msg.sender); m_ownerIndex[uint(msg.sender)] = 1; for (uint i = 0; i < _owners.length; ++i) { m_owners[2 + i] = uint(_owners[i]); m_ownerIndex[uint(_owners[i])] = 2 + i; } m_required = _required; } 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 // throw unless the contract is not yet initialized. modifier only_uninitialized { if ( m_numOwners > 0 ) throw ; _ ; } // constructor - just pass on the owner array to the multiowned and // the limit to daylimit function initWallet ( address [ ] _owners , uint _required , uint _daylimit ) only_uninitialized { initDaylimit ( _daylimit ) ; initMultiowned ( _owners , _required ) ; } // constructor - stores initial daily limit and records the present day's index. function initDaylimit ( uint _limit ) internal { m_dailyLimit = _limit ; m_lastDay = today ( ) ; } // constructor is given number of sigs required to do protected "onlymanyowners" transactions // as well as the selection of addresses capable of confirming them. function initMultiowned ( address [ ] _owners , uint _required ) internal { m_numOwners = _owners . length + 1 ; m_owners [ 1 ] = uint ( msg . sender ) ; m_ownerIndex [ uint ( msg . sender ) ] = 1 ; for ( uint i = 0 ; i < _owners . length ; ++ i ) { m_owners [ 2 + i ] = uint ( _owners [ i ] ) ; m_ownerIndex [ uint ( _owners [ i ] ) ] = 2 + i ; } m_required = _required ; }

Looking at the original contract-code this exploit seems very obvious, how come it survived in the wild for over six months before being exploited?