Mimo August 2022 contest - rbserver's results

[L-01] autoParams.varFee CANNOT BE CONFIGURED TO EQUAL maxVarFee FOR CALLING THE setAutomation FUNCTION

Calling the setAutomation function of the MIMOAutoAction contract will revert when autoParams.varFee and maxVarFee are equal. Yet, the name of maxVarFee suggests that it represents the maximum value of varFee that autoParams.varFee is allowed to be configured to. If this is the case, the autoParams.varFee >= maxVarFee condition in the following code needs to be changed to autoParams.varFee > maxVarFee. Otherwise, maxVarFee needs to be renamed to be more accurate.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoAction.sol#L45-L47

if (autoParams.varFee >= maxVarFee) {
  revert CustomErrors.VARIABLE_FEE_TOO_HIGH(maxVarFee, autoParams.varFee);
}

[L-02] MISSING CHECKS FOR INPUT ARRAY LENGTHS

For a function, when multiple inputs are arrays, and their items correspond to each other, the lengths of these arrays should be checked to be the same. For the following function, the lengths of the targets and data inputs need to be checked to be the same.

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

  function multicall(address[] calldata targets, bytes[] calldata data) external override returns (bytes[] memory) {
    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;
  }

[L-03] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESS

To prevent unintended behaviors, the critical address input should be checked against address(0). Please consider checking _mimoProxyBase in the following constructor code.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxyFactory.sol#L26-L28

  constructor(address _mimoProxyBase) {
    mimoProxyBase = _mimoProxyBase;
  }

[N-01] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers in the following code with constants.

actions\automated\MIMOAutoRebalance.sol
  180: uint256 targetRatio = autoVault.targetRatio + 1e15; // add 0.1% to account for rounding

proxy\MIMOProxy.sol
  30: minGasReserve = 5_000;

[N-02] REDUNDANT NAMED RETURNS

When a function has unused named returns and used return statement, these named returns become redundant. To improve readability and maintainability, these variables for the named returns can be removed while keeping the return statement for the following function.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L142-L154

  function getAmounts(uint256 vaultId, address toCollateral)
    external
    view
    override
    returns (
      uint256 rebalanceAmount,
      uint256 mintAmount,
      uint256 autoFee
    )
  {
    (, VaultState memory vaultState) = _getVaultStats(vaultId);
    return _getAmounts(_automatedVaults[vaultId], vaultState, toCollateral);
  }

[N-03] TYPO IN INTERFACE NAME

In actions\interfaces\IMIMOEmptyVault.sol, the interface is named as IMIMOEmtpyVault. Its name can be changed to IMIMOEmptyVault that matches the file name.

actions\interfaces\IMIMOEmptyVault.sol
  7: interface IMIMOEmtpyVault is IMIMOProxyAction, IMIMOSwap {

actions\MIMOEmptyVault.sol
  14: contract MIMOEmptyVault is MIMOSwap, MIMOFlashloan, IMIMOEmtpyVault {

[N-04] PUBLIC FUNCTIONS NOT CALLED BY OWN CONTRACT CAN BE EXTERNAL

For public functions that are not called by their own contract, their visibilities can be set to external for readability and maintainability. Please consider changing the visibilities of the following functions from public to external.

proxy\MIMOProxy.sol
  54: function execute(address target, bytes calldata data) public payable override returns (bytes memory response) {
  104-109:
    function setPermission(
        address envoy,
        address target,
        bytes4 selector,
        bool permission
    ) public override {

[N-05] MISSING INDEXED PARAMETERS FOR EVENTS

Indexing parameters improves efficiency for filtering events. Please consider indexing parameters for the following events.

actions\automated\interfaces\IMIMOAutoAction.sol
  25: event AutomationSet(uint256 vaultId, AutomatedVault autoVault);

actions\managed\interfaces\IMIMOManagedAction.sol
  22: event ManagerSet(address manager, bool isManager);
  23: event ManagementSet(uint256 vaultId, ManagedVault managedVault);

[N-06] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for these functions.

actions\automated\MIMOAutoAction.sol
  57: function getAutomatedVault(uint256 vaultId) external view override returns (AutomatedVault memory) { 
  64: function getOperationTracker(uint256 vaultId) external view override returns (uint256) { 
  92: function _isVaultVariationAllowed( 

actions\automated\MIMOAutoRebalance.sol
  90: function executeOperation( 

actions\managed\MIMOManagedAction.sol
  70: function getManagedVault(uint256 vaultId) external view override returns (ManagedVault memory) { 
  77: function getOperationTracker(uint256 vaultId) external view override returns (uint256) { 
  84: function getManager(address manager) external view override returns (bool) { 
  92: function _getVaultRatio(uint256 vaultId) internal view returns (uint256) { 
  115: function _isVaultVariationAllowed( 

actions\managed\MIMOManagedRebalance.sol
  142: function _preRebalanceChecks( 

proxy\interfaces\IMIMOProxy.sol
  24: function getPermission(

proxy\interfaces\IMIMOProxyFactory.sol
  21: function VERSION() external view returns (uint256);

proxy\interfaces\IMIMOProxyRegistry.sol
  18: function getCurrentProxy(address owner) external view returns (IMIMOProxy proxy);

[N-07] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

proxy\MIMOProxy.sol
  2: pragma solidity >=0.8.4;

proxy\MIMOProxyFactory.sol
  2: pragma solidity >=0.8.4;

proxy\MIMOProxyRegistry.sol
  2: pragma solidity >=0.8.4;

[G-01] CONDITIONAL REVERSION CAN BE PLACED ABOVE OTHER MORE EXPENSIVE OPERATIONS IF POSSIBLE

When a conditional reversion is placed above other operations that cost more gas, these operations are prevented from running if it does revert.

For the following function, if (msg.sender != address(lendingPool)) {revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/MIMOEmptyVault.sol#L63-L101

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address initiator,
    bytes calldata params
  ) external override returns (bool) {
    (address owner, uint256 vaultId, SwapData memory swapData) = abi.decode(params, (address, uint256, SwapData));
    IMIMOProxy mimoProxy = IMIMOProxy(proxyRegistry.getCurrentProxy(owner));

    if (initiator != address(mimoProxy)) {
      revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(mimoProxy));
    }
    if (msg.sender != address(lendingPool)) {
      revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));
    }
    ...
  }

For the following function, if (msg.sender != address(lendingPool)) {revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/MIMOLeverage.sol#L68-L103

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address initiator,
    bytes calldata params
  ) external override returns (bool) {
    (address owner, uint256 swapAmount, SwapData memory swapData) = abi.decode(params, (address, uint256, SwapData));
    IMIMOProxy mimoProxy = IMIMOProxy(proxyRegistry.getCurrentProxy(owner));

    if (initiator != address(mimoProxy)) {
      revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(mimoProxy));
    }
    if (msg.sender != address(lendingPool)) {
      revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));
    }
    ...
  }

For the following function, if (msg.sender != address(lendingPool)) {revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/MIMORebalance.sol#L63-L104

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address initiator,
    bytes calldata params
  ) external override returns (bool) {
    (address owner, RebalanceData memory rbData, SwapData memory swapData) = abi.decode(
      params,
      (address, RebalanceData, SwapData)
    );
    IMIMOProxy mimoProxy = IMIMOProxy(proxyRegistry.getCurrentProxy(owner));

    if (initiator != address(mimoProxy)) {
      revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(mimoProxy));
    }
    if (msg.sender != address(lendingPool)) {
      revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));
    }
    ...
  }

For the following function, if (initiator != address(this)) {revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(this));} and if (msg.sender != address(lendingPool)) {revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L90-L132

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address initiator,
    bytes calldata params
  ) external override returns (bool) {
    (
      address mimoProxy,
      uint256 managerFee,
      IMIMORebalance.RebalanceData memory rbData,
      IMIMOSwap.SwapData memory swapData
    ) = abi.decode(params, (address, uint256, IMIMORebalance.RebalanceData, IMIMOSwap.SwapData));

    if (initiator != address(this)) {
      revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(this));
    }
    if (msg.sender != address(lendingPool)) {
      revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));
    }
    ...
  }

For the following function, (uint256 vaultARatio, ) = _getVaultStats(vaultId); if (vaultARatio < autoVault.targetRatio) {revert CustomErrors.FINAL_VAULT_RATIO_TOO_LOW(autoVault.targetRatio, vaultARatio);} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L262-L286

  function _postRebalanceChecks(
    AutomatedVault memory autoVault,
    uint256 rebalanceAmount,
    uint256 vaultBBalanceBefore,
    uint256 vaultId,
    address vaultOwner,
    IVaultsDataProvider vaultsData
  ) internal view {
    IPriceFeed priceFeed = a.priceFeed();
    
    ...

    (uint256 vaultARatio, ) = _getVaultStats(vaultId);

    if (vaultARatio < autoVault.targetRatio) {
      revert CustomErrors.FINAL_VAULT_RATIO_TOO_LOW(autoVault.targetRatio, vaultARatio);
    }
  }

For the following function, if (!_managers[mgtParams.manager]) {revert CustomErrors.MANAGER_NOT_LISTED();} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedAction.sol#L33-L47

  function setManagement(uint256 vaultId, ManagedVault calldata mgtParams) external override {
    address vaultOwner = a.vaultsData().vaultOwner(vaultId);
    address mimoProxy = address(proxyRegistry.getCurrentProxy(msg.sender));

    if (mimoProxy != vaultOwner && vaultOwner != msg.sender) {
      revert CustomErrors.CALLER_NOT_VAULT_OWNER(mimoProxy, vaultOwner);
    }
    if (!_managers[mgtParams.manager]) {
      revert CustomErrors.MANAGER_NOT_LISTED();
    }
    ...
  }

For the following function, if (initiator != address(this)) {revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(this));} and if (msg.sender != address(lendingPool)) {revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L91-L133

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address initiator,
    bytes calldata params
  ) external override returns (bool) {
    (
      address mimoProxy,
      uint256 managerFee,
      IMIMORebalance.RebalanceData memory rbData,
      IMIMOSwap.SwapData memory swapData
    ) = abi.decode(params, (address, uint256, IMIMORebalance.RebalanceData, IMIMOSwap.SwapData));

    if (initiator != address(this)) {
      revert CustomErrors.INITIATOR_NOT_AUTHORIZED(initiator, address(this));
    }
    if (msg.sender != address(lendingPool)) {
      revert CustomErrors.CALLER_NOT_LENDING_POOL(msg.sender, address(lendingPool));
    }
    ...
  }

For the following function, uint256 vaultARatioAfter = _getVaultRatio(vaultId); if (vaultARatioAfter < managedVault.minRatio) {revert CustomErrors.FINAL_VAULT_RATIO_TOO_LOW(managedVault.minRatio, vaultARatioAfter);} can be placed at the beginning of the function body.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L179-L211

  function _postRebalanceChecks(
    ManagedVault memory managedVault,
    uint256 rebalanceAmount,
    uint256 vaultBBalanceBefore,
    uint256 vaultId,
    address vaultOwner,
    address toCollateral,
    IVaultsDataProvider vaultsData
  ) internal view {
    IPriceFeed priceFeed = a.priceFeed();

    ...

    uint256 vaultARatioAfter = _getVaultRatio(vaultId);

    if (vaultARatioAfter < managedVault.minRatio) {
      revert CustomErrors.FINAL_VAULT_RATIO_TOO_LOW(managedVault.minRatio, vaultARatioAfter);
    }
	
    ...
  }

[G-02] UNUSED NAMED RETURNS COSTS UNNECESSARY DEPLOYMENT GAS

When a function has unused named returns, unnecessary deployment gas is burned. Please consider removing the named return variables in the following function.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L142-L154

  function getAmounts(uint256 vaultId, address toCollateral)
    external
    view
    override
    returns (
      uint256 rebalanceAmount,
      uint256 mintAmount,
      uint256 autoFee
    )
  {
    (, VaultState memory vaultState) = _getVaultStats(vaultId);
    return _getAmounts(_automatedVaults[vaultId], vaultState, toCollateral);
  }

[G-03] ARITHMETIC OPERATIONS THAT DO NOT UNDERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not underflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

The following code are executed when their internal view functions are called by other external non-view functions. In these code, block.timestamp - 1 days should not underflow and can be wrapped in unchecked {}.

actions\automated\MIMOAutoRebalance.sol
  244: if (_operationTracker[vaultId] > block.timestamp - 1 days) {

actions\managed\MIMOManagedRebalance.sol
  160: if (_operationTracker[rbData.vaultId] > block.timestamp - 1 days) {

[G-04] STATE VARIABLE WHOSE VALUE IS ONLY ASSIGNED IN CONSTRUCTOR CAN BE IMMUTABLE

Providing an immutable keyword to the state variable whose value is only assigned in the constructor can help save gas. factory can be declared as an immutable in the following code.

https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxyRegistry.sol#L15-L28

  /// @inheritdoc IMIMOProxyRegistry
  IMIMOProxyFactory public override factory;

  ...

  /// @param factory_ The base contract of the factory
  constructor(IMIMOProxyFactory factory_) {
    factory = factory_;
  }

[G-05] VISIBILITIES CAN BE SET TO EXTERNAL FOR FUNCTIONS THAT ARE NOT CALLED BY OWN CONTRACTS

Calling an external function is cheaper than that for a public function. For the following functions, their visibilities can be set to external because their contracts do not call them.

proxy\MIMOProxy.sol
  54: function execute(address target, bytes calldata data) public payable override returns (bytes memory response) {
  104-109:
    function setPermission(
        address envoy,
        address target,
        bytes4 selector,
        bool permission
    ) public override {

[G-06] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE() OR REVERT() WITH REASON STRING

revert with custom error can cost less gas than require() or revert() with reason string. Please consider using revert with custom error to replace the following require() and revert().

actions\MIMOEmptyVault.sol
  96: require(flashloanRepayAmount <= vaultCollateral.balanceOf(address(this)), Errors.CANNOT_REPAY_FLASHLOAN);

actions\MIMOLeverage.sol
  130: require(collateralBalanceAfter >= flashloanRepayAmount, Errors.CANNOT_REPAY_FLASHLOAN);

actions\MIMORebalance.sol
  129-132:
    require(
      a.vaultsData().vaultCollateralBalance(rbData.vaultId) >= flashloanRepayAmount,
      Errors.CANNOT_REPAY_FLASHLOAN
    );

actions\MIMOSwap.sol
  47: require(proxy != address(0), Errors.INVALID_AGGREGATOR);
  48: require(router != address(0), Errors.INVALID_AGGREGATOR);
  62: revert(Errors.AGGREGATOR_CALL_FAILED);