From d7c5ab162e30540e8a51decd6bde4f630001cb56 Mon Sep 17 00:00:00 2001 From: Micah Zoltu Date: Fri, 20 May 2016 21:03:17 -0700 Subject: [PATCH] Changes `m_owners` and `m_ownerIndex` to use address instead of uint. It is unclear why this code was using `uint` previously instead of `address`. If `address` is what is intended, `address` should be used. Using `address` not only makes the intent more clear, it allows the static analyzer to validate the code. Also, tooling will now work better with this contract since the types are correct. If these variables were intentionally `uint`, I am curious to know the reason why and at the least there should be a comment in the code indicating why they are `uint`. --- wallet/wallet.sol | 78 +++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/wallet/wallet.sol b/wallet/wallet.sol index c522a9b..f92968a 100644 --- a/wallet/wallet.sol +++ b/wallet/wallet.sol @@ -53,19 +53,19 @@ contract multiowned { // as well as the selection of addresses capable of confirming them. function multiowned(address[] _owners, uint _required) { m_numOwners = _owners.length + 1; - m_owners[1] = uint(msg.sender); - m_ownerIndex[uint(msg.sender)] = 1; + m_owners[1] = msg.sender; + m_ownerIndex[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_owners[2 + i] = _owners[i]; + m_ownerIndex[_owners[i]] = 2 + i; } m_required = _required; } - + // Revokes a prior confirmation of the given operation function revoke(bytes32 _operation) external { - uint ownerIndex = m_ownerIndex[uint(msg.sender)]; + uint ownerIndex = m_ownerIndex[msg.sender]; // make sure they're an owner if (ownerIndex == 0) return; uint ownerIndexBit = 2**ownerIndex; @@ -76,20 +76,20 @@ contract multiowned { Revoke(msg.sender, _operation); } } - + // Replaces an owner `_from` with another `_to`. function changeOwner(address _from, address _to) onlymanyowners(sha3(msg.data)) external { if (isOwner(_to)) return; - uint ownerIndex = m_ownerIndex[uint(_from)]; + uint ownerIndex = m_ownerIndex[_from]; if (ownerIndex == 0) return; clearPending(); - m_owners[ownerIndex] = uint(_to); - m_ownerIndex[uint(_from)] = 0; - m_ownerIndex[uint(_to)] = ownerIndex; + m_owners[ownerIndex] = _to; + m_ownerIndex[_from] = 0; + m_ownerIndex[_to] = ownerIndex; OwnerChanged(_from, _to); } - + function addOwner(address _owner) onlymanyowners(sha3(msg.data)) external { if (isOwner(_owner)) return; @@ -99,37 +99,37 @@ contract multiowned { if (m_numOwners >= c_maxOwners) return; m_numOwners++; - m_owners[m_numOwners] = uint(_owner); - m_ownerIndex[uint(_owner)] = m_numOwners; + m_owners[m_numOwners] = _owner; + m_ownerIndex[_owner] = m_numOwners; OwnerAdded(_owner); } - + function removeOwner(address _owner) onlymanyowners(sha3(msg.data)) external { - uint ownerIndex = m_ownerIndex[uint(_owner)]; + uint ownerIndex = m_ownerIndex[_owner]; if (ownerIndex == 0) return; if (m_required > m_numOwners - 1) return; m_owners[ownerIndex] = 0; - m_ownerIndex[uint(_owner)] = 0; + m_ownerIndex[_owner] = 0; clearPending(); reorganizeOwners(); //make sure m_numOwner is equal to the number of owners and always points to the optimal free slot OwnerRemoved(_owner); } - + function changeRequirement(uint _newRequired) onlymanyowners(sha3(msg.data)) external { if (_newRequired > m_numOwners) return; m_required = _newRequired; clearPending(); RequirementChanged(_newRequired); } - + function isOwner(address _addr) returns (bool) { - return m_ownerIndex[uint(_addr)] > 0; + return m_ownerIndex[_addr] > 0; } - + function hasConfirmed(bytes32 _operation, address _owner) constant returns (bool) { var pending = m_pending[_operation]; - uint ownerIndex = m_ownerIndex[uint(_owner)]; + uint ownerIndex = m_ownerIndex[_owner]; // make sure they're an owner if (ownerIndex == 0) return false; @@ -138,12 +138,12 @@ contract multiowned { uint ownerIndexBit = 2**ownerIndex; return !(pending.ownersDone & ownerIndexBit == 0); } - + // INTERNAL METHODS function confirmAndCheck(bytes32 _operation) internal returns (bool) { // determine what index the present sender is: - uint ownerIndex = m_ownerIndex[uint(msg.sender)]; + uint ownerIndex = m_ownerIndex[msg.sender]; // make sure they're an owner if (ownerIndex == 0) return; @@ -192,7 +192,7 @@ contract multiowned { } } } - + function clearPending() internal { uint length = m_pendingIndex.length; for (uint i = 0; i < length; ++i) @@ -200,19 +200,19 @@ contract multiowned { delete m_pending[m_pendingIndex[i]]; delete m_pendingIndex; } - + // FIELDS // the number of owners that must confirm the same operation before it is run. uint public m_required; // pointer used to find a free slot in m_owners uint public m_numOwners; - + // list of owners - uint[256] m_owners; + address[256] m_owners; uint constant c_maxOwners = 250; // index on the list of owners to allow reverse lookup - mapping(uint => uint) m_ownerIndex; + mapping(address => uint) m_ownerIndex; // the ongoing operations. mapping(bytes32 => PendingState) m_pending; bytes32[] m_pendingIndex; @@ -246,9 +246,9 @@ contract daylimit is multiowned { function resetSpentToday() onlymanyowners(sha3(msg.data)) external { m_spentToday = 0; } - + // INTERNAL METHODS - + // checks to see if there is at least `_value` left from the daily limit today. if there is, subtracts it and // returns true. otherwise just returns false. function underLimit(uint _value) internal onlyowner returns (bool) { @@ -288,9 +288,9 @@ contract multisig { event MultiTransact(address owner, bytes32 operation, uint value, address to, bytes data); // Confirmation still needed for a transaction. event ConfirmationNeeded(bytes32 operation, address initiator, uint value, address to, bytes data); - + // FUNCTIONS - + // TODO: document function changeOwner(address _from, address _to) external; function execute(address _to, uint _value, bytes _data) external returns (bytes32); @@ -318,19 +318,19 @@ contract Wallet is multisig, multiowned, daylimit { function Wallet(address[] _owners, uint _required, uint _daylimit) multiowned(_owners, _required) daylimit(_daylimit) { } - + // kills the contract sending everything to `_to`. function kill(address _to) onlymanyowners(sha3(msg.data)) external { suicide(_to); } - + // gets called when no other function matches function() { // just being sent some cash? if (msg.value > 0) Deposit(msg.sender, msg.value); } - + // Outside-visible transact entry point. Executes transacion immediately if below daily spend limit. // If not, goes into multisig process. We provide a hash on return to allow the sender to provide // shortcuts for the other confirmations (allowing them to avoid replicating the _to, _value @@ -352,7 +352,7 @@ contract Wallet is multisig, multiowned, daylimit { ConfirmationNeeded(_r, msg.sender, _value, _to, _data); } } - + // confirm a transaction through just the hash. we use the previous transactions map, m_txs, in order // to determine the body of the transaction from the hash provided. function confirm(bytes32 _h) onlymanyowners(_h) returns (bool) { @@ -363,9 +363,9 @@ contract Wallet is multisig, multiowned, daylimit { return true; } } - + // INTERNAL METHODS - + function clearPending() internal { uint length = m_pendingIndex.length; for (uint i = 0; i < length; ++i)