We reviewed and audited the Gnosis multisig wallet contract for our own internal use, and choose to publish our findings for informational purposes. We also shared this report privately with the Gnosis team.
We found the code under scrutiny to be elegant, robust, and secure. The wallet’s features are implemented with a minimal amount of code, resulting in a reduced attack surface. Despite such minimalism, its well-thought-out design allows for a surprisingly large feature set.
The audited code is located in the github.com/maraoz/MultiSigWallet repository, which is a fork of the original code found at github.com/gnosis/MultiSigWallet. The version used for this report is commit
585863178330d3c64855d596caba2b7f3271a423. Only the code in contracts/MultiSigWallet.sol was audited.
Here is our assessment and observations on possible improvements, in order of importance.
No issues of critical severity.
Incomplete test coverage
Of the features implemented in
MultiSigWallet, only the following are tested:
- Funding a wallet
- Submitting a transaction
- Confirming a transaction
- Changing the required number of confirmations
- Executing a transaction
A considerable amount of features are not tested, such as:
- Adding an owner
- Changing an owner
- Removing an owner
- Trying to add duplicate owners
- Trying to remove all owners
- Trying to set an invalid number of required approvals
- An Owner trying to execute a transaction that is not yet approved by other owners
- Non-owners trying to submit/approve/execute transactions
Consider making use of a coverage library such as solidity-coverage and increasing the test coverage to match all the features, behaviors and edge cases implemented in the
Update: The Gnosis team informed us that there are some tests which were not ported to the truffle testing framework.
replaceOwner can produce an invalid owner set
Since moving funds out of the wallet always requires a transaction, and at least one owner to authorize it, a wallet with no owners or invalid owner addresses (such as the address
0x0) would be unable to withdraw its funds.
To guard against this problem, the constructor function uses the
validRequirement modifier to guarantee that at least one valid owner is set. Similarly, the function
removeOwner will not allow its last owner to be removed because, in such situation, it would call
changeRequirement with zero as a parameter, which is not a
validRequirement and would end up triggering a revert on the
However, the function
replaceOwner does not check for the validity of the incoming owner address, which means that an address such as
0x0 could be set for a new owner. This will produce the unwanted situation described above, where funds cannot be withdrawn from the wallet.
Consider using the
notNull modifier in
replaceOwner to avoid such a situation.
Duplicate code for confirmation count
getConfirmationCount iterates over the
owners array and verifies that each owner has confirmed a given transaction id, counting the total verifications. The function
isConfirmed does the same thing, but finally checks if the total count is larger or equal to the wallet’s required approval count for confirming a transaction.
getConfirmationCount from within
isConfirmed to avoid code duplication.
Outdated Solidity version
The latest solidity version at the time of this writing is 0.4.20. The contract uses 0.4.18 and truffle is setup for version 3.4.9, which uses the 0.4.15 solidity compiler. As a result, all tests fail because the version specified in solidity is larger than the available compiler version.
Also, despite the solidity version of the contract being 0.4.18, deprecated usage of
constant in functions still exists.
Consider updating the solidity version in the contracts to 0.4.20, updating to truffle version 4.0.0, and replacing all usage of
constant in functions to
pure if applicable).
Functions that return variable sized arrays should be external
As addressed in the solidity documentation, section “Can you return an array or a string from a solidity function call?“, functions can return variable sized data only in external calls. The functions
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.
The bodies of the mentioned functions do construct and return fixed-size arrays, in what might be an attempt from the developer to mitigate this problem.
Consider simplifying the code by not using fixed sized arrays at all, and changing the visibility of the functions to
Notes & Additional Information
- The contract never specifies
uintresolution — It is a good practice to specify weather a
uint64, etc, in all cases. Doing so has multiple benefits such as reducing storage space, avoiding overflows, etc.
- Inconsistent variable naming conventions — The contract adopts the convention of using underscore in function parameters in order to distinguish them from contract members. However, it does so partially. For example, the function
changeRequirementuses underscore, while
addOwnerdoesn’t. Consider using underscore in all method parameters.
- The code would benefit from minor renaming to make it more explicit and easier to read. For example, the
confirmedmodifier, which checks if a given transaction id has been confirmed by a given owner, could be renamed to
txConfirmedByOwner. Similarly the modifier
notConfirmedcould be renamed to
txNotConfirmedByOwner. The central variable
requireddetermines the minimum number of owner confirmations that a transaction needs for approval, and a name like
requiredConfirmationswould be clearer. Finally,
changeRequirementcould be changed to
getTransactionIdsdoes not check if
tois larger than
- The fallback function should explicitly declare
No critical severity and one high severity issue was found and explained, along with recommendations on how to fix it. Some changes were proposed to follow best practices and reduce potential attack surface.
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to the Gnosis multisig wallet contracts. For general information about smart contract security, check out our thoughts _here._