Mimo August 2022 contest - 0x1f8b's results

• Low
  • 1. Outdated and mixed pragmas
  • 2. Lack of ACK during owner change
  • 3. Nothing prevents the proxy from being compromised
  • 4. Lack of checks
• Non-Critical
  • 5. Codding style
    • Non descriptive method name
  • 6. Improve design
    • Return the output of the call

Low

1. Outdated and mixed pragmas

The pragma version used are:

pragma solidity 0.8.10;
pragma solidity ^0.8.4;
pragma solidity >=0.8.4;

Note that mixing pragma are not recommended.

The minimum required version must be 0.8.15; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• MIMOProxy.sol#L117

3. Nothing prevents the proxy from being compromised

The execute method of the MIMOProxy contract contains the following condition to prevent a malicious call from altering the original owner.

if (owner_ != owner) {
    revert CustomErrors.OWNER_CHANGED(owner_, owner);
}

However, via the permissions system, it is easily bypassed by adding an execute permission with setPermission and calling it later with the desired selector.

if (owner != msg.sender) {
    bytes4 selector;
    assembly {
    selector := calldataload(data.offset)
    }
    if (!_permissions[msg.sender][target][selector]) {
    revert CustomErrors.EXECUTION_NOT_AUTHORIZED(owner, msg.sender, target, selector);
    }
}

So the security of the proxy can be compromised in the same way.

Affected source code:

• MIMOProxy.sol#L82-L84

4. Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• MIMOProxyFactory.sol#L27
• MIMOProxyRegistry.sol#L27

---

Non-Critical

5. Codding style

Non descriptive method name

The following method names are not descriptive, consider changing the name.

function a() external view returns (IAddressProvider);:

• IMIMOAutoAction.sol#L29
• IMIMOManagedAction.sol#L29

6. Improve design

Return the output of the call

The method _aggregatorSwap does not returns the output of the call, it could be interesting in the future to have this information, for debugging or for other purposes.

  function _aggregatorSwap(
    IERC20 token,
    uint256 amount,
    SwapData calldata swapData
- ) internal {
+ ) internal returns (bytes memory response){
    (address proxy, address router) = dexAP.getDex(swapData.dexIndex);

    require(proxy != address(0), Errors.INVALID_AGGREGATOR);
    require(router != address(0), Errors.INVALID_AGGREGATOR);

    token.safeIncreaseAllowance(proxy, amount);

-   (bool success, bytes memory response) = router.call(swapData.dexTxData);
+   (bool success, response) = router.call(swapData.dexTxData);

Affected source code:

• MIMOSwap.sol#L40

• Gas
  • 1. Remove event index
  • 2. Gas saving using immutable
  • 3. ++i costs less gas compared to i++ or i += 1
  • 4. Don't use the length of an array for loops condition
  • 5. There's no need to set default values for variables
  • 6. Remove redundant checks
  • 7. Use Custom Errors instead of Revert Strings to save Gas

Gas

1. Remove event index

TransferOwnership is not an event that need to be filtered, These indexes can be removed:

• IMIMOProxy.sol#L11

2. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• MIMOProxy.sol#L19

3. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• MIMOProxy.sol#L132

4. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• MIMOProxy.sol#L132

5. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• MIMOProxy.sol#L132

6. Remove redundant checks

Some conditions can be removed to remove gas because they are always met or it is impossible for them to do so.

  function deployFor(address owner) public override returns (IMIMOProxy proxy) {
    IMIMOProxy currentProxy = _currentProxies[owner];

    // Do not deploy if the proxy already exists and the owner is the same.
-   if (address(currentProxy) != address(0) && currentProxy.owner() == owner) {
+   if (address(currentProxy) != address(0)) {
      revert CustomErrors.PROXY_ALREADY_EXISTS(owner);
    }

    // Deploy the proxy via the factory.
    proxy = factory.deployFor(owner);

    // Set or override the current proxy for the owner.
    _currentProxies[owner] = IMIMOProxy(proxy);
  }

Affected source code:

• MIMOProxyRegistry.sol#L49

7. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In this case, the statements are short, so it is recommended to use custom errors.

Affected source code:

• MIMOSwap.sol#L47-L48
• MIMOEmptyVault.sol#L96