This document is a security audit report performed by RideSolo, where ConsenSys MultiSigWallet has been reviewed.
Multi signature wallet is a contract that require more than one key to authorize a transaction, two diffrent contracts have been reviewed MultiSigWallet and MultiSigWalletWithDailyLimit that inherit MultiSigWallet.
MultiSigWalletWithDailyLimit contract allow owners to transfert ethers without need to multi signature but within a threshold limit.
- MultiSigWalletWithDailyLimit.sol github commit hash f1024676436b0e3c4aceb5cab4277ee05050d5d1.
5 issues were reported including:
-
2 medium severity issues.
-
1 low severity issues.
-
2 minors remarks.
Even if MultiSigWallet implement an ownership management mechanism, but a weakness of this logic is if the required number of owners to confirm a transaction is equal to the total number of owners of the MultiSigWallet. In some special circumstances (loss of private key for example), the contract wallet can be locked in a definitive way since the requirement cannot be met to execute a transaction.
Another related reason for this issue is that addOwner, removeOwner, replaceOwner and changeRequirement functions have to be executed only by the MultiSigWallet address itself. An owner has to call submitTransaction with the MultiSigWallet address and the function data to be able to excute the mentioned functions.
One solution can be to implement a mechanism where the contract owners can vote to change the requirement to be able to meet the required number of confirmation to execute a transaction.
getTransactionIds function member of MultiSigWallet contract is implemented incorrectly and has to be recoded.
This function return an array with the ids of the pending and the executed transactions between a defined range of the whole submitted transactions. However this range is not taking into account while checking the executed and pending transations when assigning transactionIdsTemp array. please check the recommended code to solve this issue.
function getTransactionIds(uint from, uint to, bool pending, bool executed)
public
constant
returns (uint[] _transactionIds)
{
require(to > from && transactionCount >= to); // check the value of to and from
uint[] memory transactionIdsTemp = new uint[](transactionCount);
uint count = 0;
uint i;
for (i=from; i<to; i++) // scan the array following the input range
if ( pending && !transactions[i].executed
|| executed && transactions[i].executed)
{
transactionIdsTemp[count] = i;
count += 1;
}
_transactionIds = new uint[](count); // _transactionIds lenght is equal to count
for (i=0; i<count; i++)
_transactionIds[i] = transactionIdsTemp[i];
}
The audited contract was deployed using Solidity compiler version 0.4.10, probably to reject being compiled with future compiler versions that might introduce incompatible changes. Developing such important contracts has to be done while thinking about the code portability and the repository has to be constantly maintained to mirror the solidity changes to the maintained code. there was no fatal incompatibility changes, however here are the compiler deprecatted code warnings:
- Invoking event without "emit" prefix is deprecated.
- Defining constructors as functions with the same name as the contract is deprecated.
- "throw" is deprecated in favour of "revert()".
tx variable in excuteTransaction is shadowing builtin symbol.
The fallback function member of MultiSigWallet is missing visibility specifier, Such important contracts for the blockchain industry have to follow the same coding standard.
No critical vulnerabilities were found, however due to the importance of the ownership managment, this contract need a mechanism to avoid the above mentioned issue (3.1).
None of the mentioned issue can cause exploit or a direct breach of operability, therefore this contract is safe to be deployed.