Mimo August 2022 contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyFactory.sol#L45

Vulnerability details

Impact

The deployFor() function in MIMOProxyFactory.sol can be called directly instead of being called within MIMOProxyRegistry.sol. This results in the ability to create many MIMOProxies that are not registered within the registry. The proxies deployed directly through the factory will lack the ability to call certain actions such as leveraging and emptying the vault, but will be able to call all functions in MIMOVaultAction.sol.

This inconsistency doesn't feel natural and would be remedied by adding an onlyRegistry modifier to the ProxyFactory.deployFor() function.

Proof of Concept

MIMOProxyFactory.deployFor() lacking any access control:

  function deployFor(address owner) public override returns (IMIMOProxy proxy) {
    proxy = IMIMOProxy(mimoProxyBase.clone());
    proxy.initialize();


    // Transfer the ownership from this factory contract to the specified owner.
    proxy.transferOwnership(owner);


    // Mark the proxy as deployed.
    _proxies[address(proxy)] = true;


    // Log the proxy via en event.
    emit DeployProxy(msg.sender, owner, address(proxy));
  }
}

Example of reduced functionality: MIMOEmptyVault.executeOperation() checks proxy existence in the proxy registry therefore can't be called.

  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));

Tools Used

Manual review.

Recommended Mitigation Steps

Adding access control to ensure that the factory deployFor function is called from the proxy registry would mitigate this issue.

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Floating Pragma

A floating pragma might result in contract being tested/deployed with different or inconsistent compiler versions possibly leading to unexpected behaviour. 3 instances of this issue have been found:

[L-01] MIMOProxyRegistry.sol#L2-L3

pragma solidity >=0.8.4;

[L-01b] MIMOProxyFactory.sol#L2-L3

pragma solidity >=0.8.4;

[L-01c] MIMOProxy.sol#L2-L3

pragma solidity >=0.8.4;

[L-02] Empty fallback()/receive() function

If intended functionality is to make use of ETH the receive() function should implement some operation, if not it should revert the transaction. 1 instance of this issue has been found:

[L-02] MIMOProxy.sol#L38

receive() external payable {}

[L-03] Ownership transfer should be a two-step process

If ownership is accidentally transferred to an inactive account all functionalities depending on it will be lost. 1 instance of this issue has been found:

[L-03] MIMOProxy.sol#L117-L123

  function transferOwnership(address newOwner) external override {
    address oldOwner = owner;
    if (oldOwner != msg.sender) {
      revert CustomErrors.NOT_OWNER(oldOwner, msg.sender);
    }
    owner = newOwner;
    emit TransferOwnership(oldOwner, newOwner);

[L-04] No check for msg.value > 0

Missing check might lead to waist of gas by calling function with msg.value = 0. 1 instance of this issue has been found:

[L-04] MIMOVaultActions.sol#L54-L56

  function depositETH() external payable override {
    core.depositETH{ value: msg.value }();
  }

[L-05] Fee-on-transfer token not supported

Tokens that incur a fee on transfer will lead to loss of funds to either user or protocol. The transfer amount is based on user input rather than the difference in balance before and after the transfer. If token incurs a fee-on-transfer the difference in balance before and after the transfer might lead to loss of funds to user or protocol. 3 instance of this issue has been found:

[L-05a] MIMOLeverage.sol#L50-L52

[L-05b] MIMOVaultActions.sol#L48-L49

[L-05c] MIMOVaultActions.sol#L71-L72

    if (depositAmount > 0) {
      IERC20(flData.asset).safeTransferFrom(msg.sender, address(this), depositAmount);
    }

[L-06] Lack of access control might lead to loss of funds

Any tokens accidentally sent to implementation contracts address can be stolen/claimed by anyone. 1 instance of this issue has been found:

[L-06] MIMOEmptyVault.sol#L111-L129

  function emptyVaultOperation(
    IERC20 vaultCollateral,
    uint256 vaultId,
    uint256 swapAmount,
    SwapData calldata swapData
  ) external {
    IVaultsCore core = a.core();

    _aggregatorSwap(vaultCollateral, swapAmount, swapData);

    IERC20 stablex = IERC20(a.stablex());
    stablex.safeIncreaseAllowance(address(core), stablex.balanceOf(address(this)));
    core.repayAll(vaultId);

    uint256 withdrawAmount = a.vaultsData().vaultCollateralBalance(vaultId);

    core.withdraw(vaultId, withdrawAmount);
    vaultCollateral.safeTransfer(msg.sender, withdrawAmount);
  }

Non-critical Findings

[N-01] Typos

Please fix typos. 5 instances of this issue have been found:

[N-01] MIMOManagedRebalance.sol#L13-L14

@title A `SuperVault V2` action contract for configuring a vault to have a manged rebalance. -> A `SuperVault V2` action contract for configuring a vault to have a managed rebalance. 

[N-01b] MIMOManagedAction.sol#L29-L30

    @dev Can only be called by vault owner and can only appoint whitelisting managers as manger -> Can only be called by vault owner and can only appoint whitelisted managers as manager

[N-01c] MIMOAutoRebalance.sol#L49-L50

    @notice Vault must have been created though a MIMOProxy -> Vault must have been created through a MIMOProxy

[N-01d] MIMOVaultActions.sol#L14-L15

  @notice Only intended to be hold the logic for paking delegateCalls from a MIMOProxy -> Only intended to hold the logic for taking delegateCalls from a MIMOProxy

[N-01e] MIMOAutoAction.sol#L27-L28

    @notice Sets a vault automation parameters -> Sets vault automation parameters

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

1 instance of this issue has been found:

[G-01] MIMOProxy.sol#L132-L133

    for (uint256 i = 0; i < targets.length; i++) {

[G-02] Using prefix(++i) instead of postfix (i++) saves gas

It saves 6 gas per iteration. 1 instance of this issue has been found:

[G-02] MIMOProxy.sol#L132-L133

    for (uint256 i = 0; i < targets.length; i++) {

[G-03] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

1 instance of this issue has been found:

[G-03] MIMOProxy.sol#L132-L133

    for (uint256 i = 0; i < targets.length; i++) {

[G-04] Setting variable to default value is redundant

Setting variable to default value is redundant. 1 instance of this issue has been found:

[G-04] MIMOProxy.sol#L132-L133

    for (uint256 i = 0; i < targets.length; i++) {

[G-05] Immutable variable missing zero address check

If variable is accidentally set to zero the whole contract will have to be redeployed. 2 instances of this issue have been found:

[G-05] MIMOProxyRegistry.sol#L26-L28

  constructor(IMIMOProxyFactory factory_) {
    factory = factory_;
  }

[G-05b] MIMOProxyFactory.sol#L26-L28

  constructor(address _mimoProxyBase) {
    mimoProxyBase = _mimoProxyBase;
  }