MulticallFacet Code Style Findings

MFT-01C: Loop Iterator Optimization

Type	Severity	Location
Gas Optimization		MulticallFacet.sol:L16

Description:

The linked for loop increments / decrements the iterator "safely" due to Solidity's built-in safe arithmetics (post-0.8.X).

Example:

Copy
16for (uint256 i = 0; i < data.length; i++) {

Recommendation:

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

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

All referenced loop iterators have been optimized as advised, removing their for declaration increment statement and instead performing it in an unchecked code block wherever needed (i.e. before a continue statement or at the end of the for loop's body).

MFT-02C: Potentially Dangerous Multicall Paradigm

Type	Severity	Location
Language Specific		MulticallFacet.sol:L17

Description:

While the present implementation of MulticallFacet is not insecure, the MulticallFacet::multicall can have devastating consequences if it is ever adjusted to be payable.

Example:

Copy
9contract MulticallFacet is IMulticall {
10  /// @inheritdoc   IMulticall
11  function multicall(
12    bytes[] calldata data
13  ) external override returns (bytes[] memory results) {
14    results = new bytes[](data.length);
15
16    for (uint256 i = 0; i < data.length; i++) {
17      results[i] = CallLib.delegateCallFunc(address(this), data[i]);
18    }
19
20    return results;
21  }
22}

Recommendation:

We advise the code to be annotated adequately, signifying that the MulticallFacet::multicall function must remain non-payable for it to be compatible with the Diamond as otherwise it can significantly affect how other facets behave and consume their msg.value evaluations.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

A custom annotation tag was introduced to the documentation of the function that specifies it should never be payable, addressing this exhibit's concerns.