MultiSigWallet Manual Review Findings

MSW-01M: Potentially Incorrect Filters

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L309, L345-L346

Description:

The pending filter would infer that the transaction is "pending" execution, however, a transaction whose confirmation time threshold has been surpassed should no longer be pending and should instead be "cancelled".

Impact:

The severity of this exhibit will be adjusted according to the course of action the Tozex team takes to rectify it.

Example:

Copy
307function getTransactionCount(bool pending, bool executed) public view returns (uint count) {
308  for (uint i = 0; i < transactionCount; i++)
309    if (pending && !transactions[i].executed || executed && transactions[i].executed)
310      count += 1;
311}

Recommendation:

We advise the Tozex team to evaluate this particular behaviour of the MultiSigWallet::getTransactionCount and MultiSigWallet::getTransactionIds functions and to adjust their code accordingly.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

A pending boolean signifying that pending transactions should be fetched now evaluates whether the transaction has timed out and will not yield any that have.

As this change was applied to both referenced functions, we consider this exhibit alleviated.

MSW-02M: Bypass of Signer / Owner Restriction

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L123, L135, L147

Description:

The MultiSigWallet contract is meant to prevent a signer from being the Ownable::owner of the contract, however, these restrictions present in the MultiSigWallet::constructor, MultiSigWallet::transferOwnership, and MultiSigWallet::requestSignerChange functions can be bypassed.

Specifically, if a signer change is requested via the MultiSigWallet::requestSignerChange function and the _newSigner is made an owner via MultiSigWallet::transferOwnership, the MultiSigWallet::confirmSignerChange function would cause the Ownable::owner to become a signer.

Impact:

The security guarantee that the Ownable::owner of the contract is not a signer is not currently upheld by the MultiSigWallet::confirmSignerChange function.

Example:

Copy
115/**
116 * Public functions
117 * @dev Contract constructor sets initial signers and required number of confirmations.
118 * @param _signers List of initial signers.
119 * @param _required Number of required confirmations.
120 */
121constructor (address[] memory _signers, uint _required) validRequirement(_signers.length, _required) {
122  for (uint i = 0; i < _signers.length; i++) {
123    require(!isSigner[_signers[i]] && _signers[i] != address(0) && _signers[i] != msg.sender);
124    isSigner[_signers[i]] = true;
125  }
126  signers = _signers;
127  required = _required;
128}
129
130/**
131  * @dev Transfers ownership of the contract to a new account (`newOwner`).
132  * Can only be called by the current owner.
133  */
134function transferOwnership(address newOwner) public override virtual onlyOwner {
135    require(!isSigner[newOwner], "Ownable: new owner cannot be signer.");
136    super.transferOwnership(newOwner);
137}
138
139/** 
140 * @notice Allows the owner to request a change of signer.
141 * @param _oldSigner The address of the current signer to be replaced.
142 * @param _newSigner The address of the new signer to be added.
143 */
144function requestSignerChange(address _oldSigner, address _newSigner) external onlyOwner {
145  require(isSigner[_oldSigner], "Old signer does not exist.");
146  require(!isSigner[_newSigner], "New signer is already a signer.");
147  require(_newSigner != owner(), "Onwer cannot be a signer.");
148
149  signerChangeRequests[_oldSigner] = _newSigner;
150  emit SignerChangeRequested(_oldSigner, _newSigner);
151}
152
153/**
154 * @notice Allows a signer to confirm a signer change requested by the owner.
155 * @param _oldSigner The address of the current signer to be replaced.
156 * @param _newSigner The address of the new signer to be added.
157 */
158function confirmSignerChange(address _oldSigner, address _newSigner) external signerExists(msg.sender) {
159  require(signerChangeRequests[_oldSigner] == _newSigner, "New signer address invalid.");
160  require(_newSigner != address(0), "No pending signer update request.");
161  require(isSigner[_newSigner] == false, "New signer is already a signer.");

Recommendation:

We advise the code to also evaluate whether the _newSigner is an Ownable::owner in the MultiSigWallet::confirmSignerChange function, ensuring that the Ownable::owner of the contract can never be a signer.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

The MultiSigWallet::confirmSignerChange function was updated to evaluate that the _newSigner is not the contract's Ownable::owner, alleviating this exhibit in full.

MSW-03M: Improper Pagination Mechanism

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L339

Description:

The MultiSigWallet::getTransactionIds function is meant to support pagination via the from and to arguments, however, all transactions of the contract are iterated regardless of the pagination specified.

Impact:

The MultiSigWallet::getTransactionIds function will become inoperable in a short timeframe as more transactions are executed in the lifetime of the MultiSigWallet due to the non-finite pagination mechanism it employs.

Example:

Copy
339function getTransactionIds(uint from, uint to, bool pending, bool executed) public view returns (uint[] memory _transactionIds)
340{
341  uint[] memory transactionIdsTemp = new uint[](transactionCount);
342  uint count = 0;
343  uint i;
344  for (i = 0; i < transactionCount; i++)
345    if (pending && !transactions[i].executed
346    || executed && transactions[i].executed)
347    {
348      transactionIdsTemp[count] = i;
349      count += 1;
350    }
351  _transactionIds = new uint[](to - from);
352  for (i = from; i < to; i++)
353    _transactionIds[i - from] = transactionIdsTemp[i];
354}

Recommendation:

We advise the Tozex team to correct the pagination mechanism, performing an initial loop from the from argument to the to argument and thus ensuring the function can be executed regardless of how many transactions have occurred in the lifetime of the MultiSigWallet contract.

Alleviation (4f28adbafc):

While the code was updated to iterate solely on the from-to range, it will inefficiently declare a full-length memory array that is then copied over to the actual _transactionIds return variable.

This does not alleviate the issue as the mere instantiation of the memory may cause the contract to run out of gas. We advise the code to instantiate the final array (_transactionIds) immediately to the to - from range. Additionally, the code still suffers from potentially undefined indexes. To amend this, the from variable should be assigned to Math.min(from, transactionCount) and the to variable should be assigned to Math.max(to, transactionCount). Finally, a require check should be introduced that ensures from < to to guarantee that the function behaves as expected.

Alleviation (8f83a1307b):

All requested changes have been applied to the codebase, optimizing the transactionIdsTemp array's instantiation and ensuring that the from and to variables represent a valid range of values to iterate through.

As such, we consider this exhibit fully alleviated.

MSW-04M: Incorrect Requirement Validation

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L103

Description:

The MultiSigWallet::validRequirement modifier will evaluate certain conditions using an ELSE clause (||) rather than an AND clause (&&), causing only one of these to validate the requirements.

Impact:

It is presently possible to deploy a MultiSigWallet that is significantly weak, has more signers than permitted by the system, or requires more signatures than the owner count.

Example:

Copy
102modifier validRequirement(uint signerCount, uint _required) {
103  require((signerCount <= MAX_OWNER_COUNT) || (required <= signerCount) || (_required != 0) || (signerCount != 0));
104  _;
105}

Recommendation:

We advise the code to properly validate its input arguments by ensuring that the conditional evaluates to true only when all its predicates are met.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

The referenced conditionals within MultiSigWallet::validRequirement have been updated to be chained with an AND (&&) clause correctly, ensuring instantiation of the contract cannot occur in an insecure way.

MSW-05M: Inexistent Removal of Old Signers

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L167

Description:

The MultiSigWallet::confirmSignerChange function will not remove the _oldSigner from the signers array, effectively carrying over all their confirmations in the MultiSigWallet::isConfirmed and MultiSigWallet::getConfirmationCount functions significantly undermining the security of the wallet.

Impact:

It is presently possible to effectively reduce the security of the wallet from n-out-of-m to n-out-of-(m - 1) by performing a "double-vote".

This vulnerability is especially prevalent on proposals with no expiry (i.e. ones with a confirmTimestamp of 0).

Example:

Copy
158function confirmSignerChange(address _oldSigner, address _newSigner) external signerExists(msg.sender) {
159  require(signerChangeRequests[_oldSigner] == _newSigner, "New signer address invalid.");
160  require(_newSigner != address(0), "No pending signer update request.");
161  require(isSigner[_newSigner] == false, "New signer is already a signer.");
162
163  // Confirm the update by the current signer
164  signerChangeRequests[_oldSigner] = address(0);
165  isSigner[_oldSigner] = false;
166  isSigner[_newSigner] = true;
167  signers.push(_newSigner);
168  emit SignerUpdated(_oldSigner, _newSigner);
169}

Recommendation:

We advise the _oldSigner to be properly removed from the signers array, preventing the security of the contract from being undermined.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

A MultiSigWallet::removeSigner function was introduced to the codebase that is invoked within each MultiSigWallet::confirmSignerChange function execution and will remove the signer being removed from the signers array.

We would like to note that the removal procedure can be significantly optimized in its worst-case scenario by performing the replacement operation solely when i != signerCount - 1.

In any case, the original vulnerability is no longer applicable and has been rectified properly rendering this exhibit alleviated.

MSW-06M: Weak Validation of Caller

Type	Severity	Location
Logical Fault		MultiSigWallet.sol:L158-L161

Description:

The MultiSigWallet::confirmSignerChange function will ensure that its caller is an existing signer, however, it will not evaluate that the _oldSigner is the one confirming the change.

Impact:

The severity of this exhibit will be adjusted based on the actions taken by the Tozex team.

Example:

Copy
158function confirmSignerChange(address _oldSigner, address _newSigner) external signerExists(msg.sender) {
159  require(signerChangeRequests[_oldSigner] == _newSigner, "New signer address invalid.");
160  require(_newSigner != address(0), "No pending signer update request.");
161  require(isSigner[_newSigner] == false, "New signer is already a signer.");
162
163  // Confirm the update by the current signer
164  signerChangeRequests[_oldSigner] = address(0);
165  isSigner[_oldSigner] = false;
166  isSigner[_newSigner] = true;
167  signers.push(_newSigner);
168  emit SignerUpdated(_oldSigner, _newSigner);
169}

Recommendation:

We advise the Tozex team to re-evaluate this mechanism and potentially ensure that the msg.sender is the _oldSigner as otherwise a single Ownable::owner can collude with a single MultiSigWallet signer to compromise the wallet entirely.

Alleviation (4f28adbafc):

The Tozex team evaluated this exhibit and has stated that it is a business requirement for a single signer and owner to be able to adjust the signer list of the multisignature wallet.

We would like to denote that this essentially renders the multisignature wallet a 2-out-of-n scheme regardless of its actual configuration. In any case, we consider it acknowledged as the Tozex team wishes to proceed as such.

Alleviation (8f83a1307b):

The Tozex team re-evaluated this exhibit and opted to alleviate the vulnerability described after an internal assessment.

To this end, the signer change workflow was refactored to instead ensure the signer change has acquired sufficient approval from members of the multisignature wallet before being actuated.

The current implementation is incorrect as the signerChangeConfirmations is not cleared once a change is confirmed, meaning that the particular member that was confirmed will forever remain confirmed and thus can replace all _oldSigner members of the wallet if the owner submits the necessary requests.

We strongly advise the signerChangeConfirmations entry to be cleared once a signer change is confirmed. The entry should additionally be cleared if the MultiSigWallet::requestSignerChange function is invoked whilst a change is "in-progress".

Alleviation (b2b3cc0949):

A new MultiSigWallet::clearSignerChangeConfirmations function has been introduced that will clear any previously-present confirmations for the _newSigner entry based on the current signers of the wallet.

Given that the function relies on the current signers of the wallet, its invocation order is incorrect in MultiSigWallet::confirmSignerChange as the _oldSigner (which may have confirmed the _newSigner) is removed from the signers array causing the confirmations of the _newSigner to remain with one more vote than it actually has.

As such, we consider this exhibit partially alleviated and we urge the Tozex team to re-order the MultiSigWallet::clearSignerChangeConfirmations function invocation right after the MultiSigWallet::isSignerChangeConfirmed evaluation.

Alleviation (3408220c9f):

The MultiSigWallet::clearSignerChangeConfirmations function call was re-ordered per our latest recommendation, ultimately alleviating this exhibit in full.