MIMOProxy accepts mismatched targets and data arrays resulting in unexpected behaviour

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L127-L147

Vulnerability details

Impact

A call to MIMOProxy.multicall may terminate without performing all the actions expected by the user. This can leave the vault in a state where it leaks value if it performing an action which opens a temporary MEV opportunity.

Proof of Concept

If we snip away access control and error handling from MIMOProxy.multicall we have the function as shown:

function multicall(address[] calldata targets, bytes[] calldata data) external override returns (bytes[] memory) {
    /* snip */
    bytes[] memory results = new bytes[](data.length);
    for (uint256 i = 0; i < targets.length; i++) {
      (bool success, bytes memory response) = targets[i].call(data[i]);
      /* snip */
      results[i] = response;
    }
    return results;
  }

Note that we write targets.length elements into an array of length data.length.

Consider the case where a user wants to perform an action temporarily creates a MEV opportunity. e.g. interacting with a contract which performs "lazy" minting of tokens such as Element finance.

In this example the contract would perform two calls, one to the token contract to perform a transfer to Element finance and another to mint the corresponding amount of wrapped position tokens, and the arguments (in psuedocode) are then

targets = [0xtokenContract, 0xelementFinance]
data=[0xtransfer, 0xprefundedDeposit]

However if the user manages to mangle their arguments so that they actually pass

targets = [0xtokenContract]
data=[0xtransfer, 0xprefundedDeposit]

then multicall will still accept this input but will terminate after the first external call, the remaining elements of the data array will not be used. MIMOProxy will then transfer the tokens out of the user's vault but not receive anything in return.

Recommended Mitigation Steps

Add an explicit check for the lengths of these two arrays to be equal.

function multicall(address[] calldata targets, bytes[] calldata data) external override returns (bytes[] memory) {
+   if (targets.length != data.length) {
+     revert CustomErrors.TARGETS_LENGTH_DIFFERENT_THAN_DATA_LENGTH(targets.length, data.length);
+   }
    if (msg.sender != owner) {
      revert CustomErrors.NOT_OWNER(owner, msg.sender);
    }
    bytes[] memory results = new bytes[](data.length);
    for (uint256 i = 0; i < targets.length; i++) {
      (bool success, bytes memory response) = targets[i].call(data[i]);
      if (!success) {
        if (response.length > 0) {
          assembly {
            let returndata_size := mload(response)
            revert(add(32, response), returndata_size)
          }
        } else {
          revert CustomErrors.LOW_LEVEL_CALL_FAILED();
        }
      }
      results[i] = response;
    }
    return results;
  }

Funnily enough, the custom error CustomErrors.TARGETS_LENGTH_DIFFERENT_THAN_DATA_LENGTH already exists but is unused.

[RayXpub]

Although we recongize this could be a user generated issue we disagree with the severity and think it should be downgraded to QA. We do intend to add an explicit check for the lengths of the two arrays to be equal as recommended.

[RayXpub]

This is actually a duplicate of #113 which is marked as QA

[gzeoneth]

Agree this is QA for input sanitization.