Mimo August 2022 contest - oyc_109's results

[L-01] Hard coded gas limits

Hardcoded gas limits can be problematic as the past has shown that gas economics in ethereum have changed, and may change again potentially rendering the contract system unusable in the future.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::30 => minGasReserve = 5_000;

[L-02] minGasReserve can be changed to cause DOS

MIMOProxy makes a delegate call

79: (success, response) = target.delegatecall{ gas: stipend }(data);

then checks if the owner has changed

82:     if (owner_ != owner) {
83:       revert CustomErrors.OWNER_CHANGED(owner_, owner);
84:     }

However a malcious contract may have modified minGasReserve during delegatecall, which creates DOS on this line

75: uint256 stipend = gasleft() - minGasReserve;

[L-03] No Transfer Ownership Pattern

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

2022-08-mimo/contracts/proxy/MIMOProxy.sol

117:   function transferOwnership(address newOwner) external override {
118:     address oldOwner = owner;
119:     if (oldOwner != msg.sender) {
120:       revert CustomErrors.NOT_OWNER(oldOwner, msg.sender);
121:     }
122:     owner = newOwner;
123:     emit TransferOwnership(oldOwner, newOwner);
124:   }

[L-04] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyFactory.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyRegistry.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxy.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyFactory.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyRegistry.sol::2 => pragma solidity ^0.8.4;

[L-05] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol::76 => _operationTracker[vaultId] = block.timestamp;
2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol::244 => if (_operationTracker[vaultId] > block.timestamp - 1 days) {
2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::77 => _operationTracker[rbData.vaultId] = block.timestamp;
2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::160 => if (_operationTracker[rbData.vaultId] > block.timestamp - 1 days) {

[L-06] Unused receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

2022-08-mimo/contracts/proxy/MIMOProxy.sol::38 => receive() external payable {}

[L-07] Missing Zero address checks

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters. Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

2022-08-mimo/contracts/proxy/MIMOProxyFactory.sol::27 => mimoProxyBase = _mimoProxyBase;
2022-08-mimo/contracts/proxy/MIMOProxyRegistry.sol::27 => factory = factory_;

[L-08] Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::104 => function setPermission(

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

2022-08-mimo/contracts/actions/MIMOEmptyVault.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/MIMOFlashloan.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/MIMOLeverage.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/MIMORebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/MIMOSwap.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/MIMOVaultActions.sol::3 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/automated/interfaces/IMIMOAutoAction.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/automated/interfaces/IMIMOAutoRebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOEmptyVault.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOFlashloan.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOLeverage.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOProxyAction.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMORebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOSwap.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/interfaces/IMIMOVaultActions.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/managed/MIMOManagedAction.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedAction.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedRebalance.sol::2 => pragma solidity 0.8.10;
2022-08-mimo/contracts/proxy/MIMOProxy.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyFactory.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyRegistry.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxy.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyFactory.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyRegistry.sol::2 => pragma solidity ^0.8.4;

[N-02] require()/revert() statements should have descriptive reason strings

Descriptive reason strings should be used so that users can troubleshot any reverted calls

2022-08-mimo/contracts/actions/MIMOSwap.sol::62 => revert(Errors.AGGREGATOR_CALL_FAILED);

[N-03] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

2022-08-mimo/contracts/actions/automated/interfaces/IMIMOAutoAction.sol::25 => event AutomationSet(uint256 vaultId, AutomatedVault autoVault);
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedAction.sol::22 => event ManagerSet(address manager, bool isManager);
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedAction.sol::23 => event ManagementSet(uint256 vaultId, ManagedVault managedVault);

[N-04] Missing NatSpec

Code should include NatSpec

2022-08-mimo/contracts/actions/automated/interfaces/IMIMOAutoAction.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/automated/interfaces/IMIMOAutoRebalance.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOEmptyVault.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOFlashloan.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOLeverage.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOProxyAction.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMORebalance.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOSwap.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/interfaces/IMIMOVaultActions.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedAction.sol::1 => // SPDX-License-Identifier: Unlicense
2022-08-mimo/contracts/actions/managed/interfaces/IMIMOManagedRebalance.sol::1 => // SPDX-License-Identifier: Unlicense

[N-05] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::54 => function execute(address target, bytes calldata data) public payable override returns (bytes memory response) {

[N-06] Missing event for critical parameter change

Emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following changes to the contract.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::104 => function setPermission(

[N-07] Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::14 => @notice This contract only serves to change the access control and enforce the `managedRebalance` configuration; the actual rebalance logic is done through the `MIMORebalance` contract through a `delegateCall` from a `MIMOProxy` clone

[N-08] Use of magic numbers

constants should be declared rather than use magic numbers

2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol
244: if (_operationTracker[vaultId] > block.timestamp - 1 days) {

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::132 => for (uint256 i = 0; i < targets.length; i++) {

[G-02] Cache Array Length Outside of Loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::132 => for (uint256 i = 0; i < targets.length; i++) {

[G-03] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

2022-08-mimo/contracts/actions/MIMOEmptyVault.sol::17 => IMIMOProxyRegistry public immutable proxyRegistry;
2022-08-mimo/contracts/actions/MIMOFlashloan.sol::18 => IPool public immutable lendingPool;
2022-08-mimo/contracts/actions/MIMOLeverage.sol::17 => IMIMOProxyRegistry public immutable proxyRegistry;
2022-08-mimo/contracts/actions/MIMORebalance.sol::17 => IMIMOProxyRegistry public immutable proxyRegistry;
2022-08-mimo/contracts/actions/MIMOSwap.sol::19 => IAddressProvider public immutable a;
2022-08-mimo/contracts/actions/MIMOSwap.sol::20 => IDexAddressProvider public immutable dexAP;
2022-08-mimo/contracts/actions/MIMOVaultActions.sol::19 => IVaultsCore public immutable core;
2022-08-mimo/contracts/actions/MIMOVaultActions.sol::20 => IVaultsDataProvider public immutable vaultsData;
2022-08-mimo/contracts/actions/MIMOVaultActions.sol::21 => IERC20 public immutable stablex;
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::12 => IAddressProvider public immutable a;
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::13 => IMIMOProxyRegistry public immutable proxyRegistry;
2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol::27 => address public immutable mimoRebalance;
2022-08-mimo/contracts/actions/managed/MIMOManagedAction.sol::12 => IAddressProvider public immutable a;
2022-08-mimo/contracts/actions/managed/MIMOManagedAction.sol::13 => IMIMOProxyRegistry public immutable proxyRegistry;
2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::20 => address public immutable mimoRebalance;
2022-08-mimo/contracts/proxy/MIMOProxyFactory.sol::16 => address public immutable mimoProxyBase;

[G-04] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

2022-08-mimo/contracts/actions/MIMOFlashloan.sol::44 => ) external virtual override returns (bool) {}
2022-08-mimo/contracts/proxy/MIMOProxy.sol::38 => receive() external payable {}

[G-05] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

2022-08-mimo/contracts/proxy/MIMOProxy.sol::132 => for (uint256 i = 0; i < targets.length; i++) {

[G-06] abi.encode() is less efficient than abi.encodePacked()

use abi.encodePacked() where possible to save gas

2022-08-mimo/contracts/actions/MIMOEmptyVault.sol::49 => bytes memory params = abi.encode(msg.sender, vaultId, swapData);
2022-08-mimo/contracts/actions/MIMOLeverage.sol::54 => bytes memory params = abi.encode(msg.sender, swapAmount, swapData);
2022-08-mimo/contracts/actions/MIMORebalance.sol::49 => bytes memory params = abi.encode(msg.sender, rbData, swapData);
2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol::73 => _takeFlashLoan(flData, abi.encode(vaultOwner, autoFee, rbData, swapData));
2022-08-mimo/contracts/actions/managed/MIMOManagedRebalance.sol::65 => _takeFlashLoan(flData, abi.encode(vaultsData.vaultOwner(rbData.vaultId), managerFee, rbData, swapData));

[G-07] Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

2022-08-mimo/contracts/proxy/MIMOProxy.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyFactory.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/MIMOProxyRegistry.sol::2 => pragma solidity >=0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxy.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyFactory.sol::2 => pragma solidity ^0.8.4;
2022-08-mimo/contracts/proxy/interfaces/IMIMOProxyRegistry.sol::2 => pragma solidity ^0.8.4;

[G-08] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) Saves 5 gas PER LOOP

2022-08-mimo/contracts/proxy/MIMOProxy.sol::132 => for (uint256 i = 0; i < targets.length; i++) {

[G-09] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

2022-08-mimo/contracts/proxy/MIMOProxy.sol::54 => function execute(address target, bytes calldata data) public payable override returns (bytes memory response) {

[G-10] Use assembly to check for address(0)

Saves 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

instances:

2022-08-mimo/contracts/actions/MIMOSwap.sol::47 => require(proxy != address(0), Errors.INVALID_AGGREGATOR);
2022-08-mimo/contracts/actions/MIMOSwap.sol::48 => require(router != address(0), Errors.INVALID_AGGREGATOR);
2022-08-mimo/contracts/proxy/MIMOProxyRegistry.sol::49 => if (address(currentProxy) != address(0) && currentProxy.owner() == owner) {

[G-11] internal functions not called by the contract should be removed to save deployment gas

If the functions are required by an interface, the contract should inherit from that interface and use the override keyword

2022-08-mimo/contracts/actions/MIMOFlashloan.sol::51 => function _takeFlashLoan(FlashLoanData memory flData, bytes memory params) internal {
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::74 => function _getVaultStats(uint256 vaultId) internal view returns (uint256 vaultRatio, VaultState memory vaultState) {
2022-08-mimo/contracts/actions/managed/MIMOManagedAction.sol::92 => function _getVaultRatio(uint256 vaultId) internal view returns (uint256) {

[G-12] Do not cache variable used only once

For variables only used once, changing it to inline saves gas

2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::40 => uint256 toVaultMcr
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::79 => uint256 collateralBalance
2022-08-mimo/contracts/actions/automated/MIMOAutoAction.sol::101 => uint256 vaultVariation

2022-08-mimo/contracts/actions/automated/MIMOAutoRebalance.sol
271:     address fromCollateral = vaultsData.vaultCollateralType(vaultId);
272:     uint256 rebalanceValue = priceFeed.convertFrom(fromCollateral, rebalanceAmount);
273:     uint256 vaultBId = vaultsData.vaultId(autoVault.toCollateral, vaultOwner);
274:     uint256 vaultBBalanceAfter = vaultsData.vaultCollateralBalance(vaultBId);
275:     uint256 swapResultValue = priceFeed.convertFrom(autoVault.toCollateral, vaultBBalanceAfter - vaultBBalanceBefore);