MultiSigWallet Code Style Findings

MSW-01C: Inefficient Loop Limit Evaluations

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L256, L297, L322

Description:

The linked for loops evaluate their limit inefficiently on each iteration.

Example:

Copy
256for (uint i = 0; i < signers.length; i++) {

Recommendation:

We advise the statements within the for loop limits to be relocated outside to a local variable declaration that is consequently utilized for the evaluations to significantly reduce the codebase's gas cost. We should note the same optimization is applicable for storage reads present in those limits as they are newly read on each iteration (i.e. length members of arrays in storage).

Alleviation (4f28adbafc):

A new contract-level signersCount variable was introduced that is meant to optimize these evaluations, however, it is incorrect as an optimization would cause the variable to be stored in a local variable rather than fetched from storage.

As such, we consider this exhibit partially alleviated.

Alleviation (8f83a1307b):

The contract-level signersCount variable has been omitted and its declaration has been relocated to each instance it is utilized albeit as a local variable per our original recommendation.

As such, we consider this exhibit fully addressed.

MSW-02C: Inefficient Variable Packing

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L49-L50, L55

Description:

The Transaction struct of the MultiSigWallet contract has a bool variable as well as address variables that are not grouped together.

Example:

Copy
48struct Transaction {
49  address payable destination;
50  address token;
51  TokenStandard ts;
52  uint tokenId;
53  uint value;
54  bytes data;
55  bool executed;
56  uint confirmTimestamp;
57  uint txTimestamp;
58}

Recommendation:

We advise the bool variable to be relocated either after or before an address variable, ensuring that the Transaction struct is tight-packed and occupies one less storage slot than it presently does.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

The executed boolean variable has been relocated before the destination address, optimizing the storage space of the Transaction struct.

MSW-03C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L209, L257, L298, L323

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

Copy
209require(_getNow() < transactions[transactionId].txTimestamp + transactions[transactionId].confirmTimestamp * 1 seconds || transactions[transactionId].confirmTimestamp == 0);

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

All referenced inefficient mapping lookups have been optimized according to our recommendation.

The MultiSigWallet::isTransactionTimedOut lookup can be further optimized by declaring the variable as storage, however, we consider the original mapping inefficiencies resolved fully.

MSW-04C: Inexistent Error Messages

Type	Severity	Location
Code Style		MultiSigWallet.sol:L63, L68, L73, L78, L83, L93, L98, L103, L123, L209

Description:

The linked require checks have no error messages explicitly defined.

Example:

Copy
63require(msg.sender == address(this));

Recommendation:

We advise each to be set so to increase the legibility of the codebase and aid in validating the require checks' conditions.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

An explicit error message has been introduced in all referenced require instances, addressing this exhibit in full.

MSW-05C: Loop Iterator Optimizations

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L122, L256, L297, L308, L322, L328, L344, L352

Description:

The linked for loops increment / decrement their iterator "safely" due to Solidity's built - in safe arithmetics (post-0.8.X).

Example:

Copy
122for (uint i = 0; i < _signers.length; i++) {

Recommendation:

We advise the increment / decrement operations to be performed in an unchecked code block as the last statement within each for loop to optimize their execution cost.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

All for loop iterators have been optimized according to our recommendation, addressing this exhibit in full.

MSW-06C: Redundant Multiplication of Time

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L209

Description:

The value of 1 seconds is 1 in Solidity and as such a multiplication by it is redundant.

Example:

Copy
209require(_getNow() < transactions[transactionId].txTimestamp + transactions[transactionId].confirmTimestamp * 1 seconds || transactions[transactionId].confirmTimestamp == 0);

Recommendation:

We advise the multiplication present in the referenced line to be safely omitted, optimizing the code's gas cost.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

The redundant multiplication has been safely omitted from the codebase.

MSW-07C: Redundant Parenthesis Statements

Type	Severity	Location
Code Style		MultiSigWallet.sol:L103

Description:

The referenced statements are redundantly wrapped in parenthesis' (()).

Example:

Copy
103require((signerCount <= MAX_OWNER_COUNT) || (required <= signerCount) || (_required != 0) || (signerCount != 0));

Recommendation:

We advise them to be safely omitted, increasing the legibility of the codebase.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

The redundant parenthesis statements have been properly omitted, addressing this exhibit.

MSW-08C: Redundant Security Checks

Type	Severity	Location
Gas Optimization		MultiSigWallet.sol:L179, L186, L235, L239, L243

Description:

The MultiSigWallet::depositERC721, MultiSigWallet::depositERC1155, and MultiSigWallet::executeTransaction functions will validate that the caller is in possession of the assets being deposited, however, this is redundant as such checks are already taken care of by the transfer functions invoked.

Example:

Copy
178function depositERC721(address token, uint tokenId) external {
179  require(IERC721(token).ownerOf(tokenId) == msg.sender, "You must own the ERC721 token.");
180  // Transfer the ERC721 token to the multisig contract
181  IERC721(token).safeTransferFrom(msg.sender, address(this), tokenId);
182  emit ERC721Deposited(msg.sender, token, tokenId);
183}
184
185function depositERC1155(address token, uint tokenId, uint amount) external {
186  require(IERC1155(token).balanceOf(msg.sender, tokenId) >= amount, "Insufficient ERC1155 balance.");
187  // Transfer the ERC1155 tokens to the multisig contract
188  IERC1155(token).safeTransferFrom(msg.sender, address(this), tokenId, amount, "");
189  emit ERC1155Deposited(msg.sender, token, tokenId, amount);
190}

Recommendation:

We advise the referenced checks to be omitted, optimizing the code's gas cost.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

All redundant balance and asset evaluations have been safely removed from the codebase, optimizing its overall gas cost significantly.

MSW-09C: Unclear Orders of Evaluation

Type	Severity	Location
Code Style		MultiSigWallet.sol:L309, L345-L346

Description:

The referenced conditional clauses are unclear in the order they are expected to be evaluated.

Example:

Copy
309if (pending && !transactions[i].executed || executed && transactions[i].executed)

Recommendation:

We advise proper parenthesis statements to be introduced grouping the AND clauses (&&) and performing the OR clause (||) on the parenthesis evaluations, increasing the legibility of the statements significantly.

Alleviation (4f28adbafcd4899bf095e7df054499a7d9839444):

Both referenced clauses have been encased in clear parenthesis statements that signify their order of evaluation, addressing this exhibit.