ConsenSys MultiSigWallet contract audit report

ETC_ETH_ConsenSys_MultiSigWallet_audit_report.md

ConsenSys MultiSigWallet contract audit report

Summary

This is the report from a security audit performed on MultiSigWallet by gorbunovperm.

Multisignature wallet — Allows multiple parties to agree on transactions before execution.

In scope

1. MultiSigWalletWithDailyLimit.sol

Findings

In total, 5 issues were reported including:

• 0 high severity issue.

• 0 medium severity issues.

• 2 low severity issues.

• 3 minor observations.

Security issues

Issues for MultiSigWalletWithDailyLimit.sol

1. The address 0x0 can be accidentally set as owner.

Severity: low

Code snippet

function replaceOwner(address owner, address newOwner)
    public
    onlyWallet
    ownerExists(owner)
    ownerDoesNotExist(newOwner)
{
    for (uint i=0; i<owners.length; i++)
        if (owners[i] == owner) {
            owners[i] = newOwner;
            break;
        }
    isOwner[owner] = false;
    isOwner[newOwner] = true;
    OwnerRemoval(owner);
    OwnerAddition(newOwner);
}

Description

There is no zero-address check in replaceOwner method. This can lead to unexpected behavior.

Recommendation

Use notNull modifier.

2. It is possible to confuse a function parameter with a global variable.

Severity: minor observation

Code snippet

modifier validRequirement(uint ownerCount, uint _required) {
    /// ...
}

Description

The name of the function parameters is recommended to start with the "_" symbol. But in this code some parameters have prefix, but some do not. This can lead to confusion or to a mistake in the future.

Recommendation

Add "_" prefix to all parameters of functions.

3. The constant mark for functions is deprecated

Severity: minor observation

Code snippet

In 7 places in the code. For example:

function isConfirmed(uint transactionId)
    public
    constant
    returns (bool)
{
    // ...
}

Description

constant on functions is an alias to view, but this is deprecated and will be dropped in version 0.5.0.

Recommendation

Use view mark.

4. Functions that return variable sized arrays can produce unexpected results.

Severity: low

Code snippet

Here, here and here.

Example:

function getOwners()
    public
    constant
    returns (address[])
{
    return owners;
}

Description

The functions getOwners, getConfirmations and getTransactionIds have public visibility, which implies that they can be called internally. Even though such an internal call does not exist in the contract, a future modification may add one, producing unexpected results. More information here.

Recommendation

Functions that return variable sized arrays should be external.

5. Several compilation warnings in the Remix compiler.

Severity: minor observation

Description

1. throw is deprecated in favour of revert(), require() and assert().
2. Defining constructors as functions with the same name as the contract is deprecated.
3. Transaction tx = transactions[transactionId]. Variable is declared as a storage pointer.
4. Invoking events without emit prefix is deprecated.
5. No visibility specified. Defaulting to public.

Recommendation

1. Use revert(), require() and assert().
2. Use constructor(...) { ... } instead.
3. Use an explicit storage keyword.
4. Use emit prefix for events.
5. Use public mark.

Conclusion

There is no serious security vulnerabilities. But it is recommend to fix all issues so that the code is clearer and safer.