Mimo August 2022 contest - JohnSmith's results

[L-01] Use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Findings

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

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

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

Mitigation

Lock the pragma version to the same version as used in the other contracts and also consider known bugs for the compiler version that is chosen.

[L-02] Low level calls don't check for contract existence

Low level calls return success if called on a destructed contract. See OpenZeppelin's Address, which checks address.code.length

Findings

contracts/proxy/MIMOProxy.sol
133:       (bool success, bytes memory response) = targets[i].call(data[i]);

Mitigation

Add is contract check

[L-03] Misleading comment in interface comments

The interface comments imply that new proxies are deployed via CREATE2, this is false;

Findings

contracts/proxy/interfaces/IMIMOProxyFactory.sol
7: /// @notice Deploys new proxies with CREATE2. @audit NO.

Recommended Mitigation Steps

Change the comment.

[L-04] Input array lengths may differ

If the caller makes a copy-paste error, the lengths may be mismatched and an operation believed to have been completed may not in fact have been completed.

Findings

contracts/proxy/MIMOProxy.sol
127:   function multicall(address[] calldata targets, bytes[] calldata data) external override returns (bytes[] memory) {
128:     if (msg.sender != owner) {
129:       revert CustomErrors.NOT_OWNER(owner, msg.sender);
130:     }
131:     bytes[] memory results = new bytes[](data.length);
132:     for (uint256 i = 0; i < targets.length; i++) {
133:       (bool success, bytes memory response) = targets[i].call(data[i]);

Mitigation

Add a check: if (targets.length != data.length) revert CustomErrors.CustomError();

[N-01] File is missing NatSpec

Findings

contracts/actions/managed/interfaces/IMIMOManagedRebalance.sol

contracts/actions/managed/interfaces/IMIMOManagedAction.sol

contracts/actions/automated/interfaces/IMIMOAutoRebalance.sol

contracts/actions/automated/interfaces/IMIMOAutoAction.sol

[N-02] NatSpec is incomplete

Check @audit comments for details

Findings

contracts/actions/automated/MIMOAutoAction.sol
			//@audit missing @param vaultId
54:   /**
55:     @return AutomatedVault struct of a specific vault id
56:    */
57:   function getAutomatedVault(uint256 vaultId) external view override returns (AutomatedVault memory) {

			//@audit missing @param vaultId
61:   /**
62:     @return Timestamp of the last performed operation
63:    */
64:   function getOperationTracker(uint256 vaultId) external view override returns (uint256) {

			//@audit missing @param autoVault, @param rebalanceValue, @param swapResultValue
88:   /**
89:     @notice Helper function determining if a vault value variation is within vault's management parameters
90:     @return True if value change is below allowedVariation and false if it is above
91:    */
92:   function _isVaultVariationAllowed(
93:     AutomatedVault memory autoVault,
94:     uint256 rebalanceValue,
95:     uint256 swapResultValue
96:   ) internal pure returns (bool) {

contracts/actions/managed/MIMOManagedAction.sol
			//@audit missing @param vaultId
67:   /**
68:     @return ManagedVault struct of a specific vault id
69:    */
70:   function getManagedVault(uint256 vaultId) external view override returns (ManagedVault memory) {

			//@audit missing @param vaultId
74:   /**
75:     @return Timestamp of the last performed operation
76:    */
77:   function getOperationTracker(uint256 vaultId) external view override returns (uint256) {

			//@audit missing @param vaultId
81:   /**
82:     @return Bool value indicating if an address is allowed to manage user vaults or not
83:    */
84:   function getManager(address manager) external view override returns (bool) {

			//@audit missing @param vaultId
88:   /**
89:     @notice Helper function calculating LTV ratio
90:     @return Vault collateral value / vault debt
91:    */
92:   function _getVaultRatio(uint256 vaultId) internal view returns (uint256) {

				//@audit missing @param managedVault, @param rebalanceValue, @param swapResultValue
111:   /**
112:     @notice Helper function determining if a vault value variation is within vault's management parameters
113:     @return True if value change is below allowedVariation and false if it is above
114:    */
115:   function _isVaultVariationAllowed(
116:     ManagedVault memory managedVault,
117:     uint256 rebalanceValue,
118:     uint256 swapResultValue
119:   ) internal pure returns (bool) {

contracts/actions/managed/MIMOManagedRebalance.sol
				//@audit missing @param rebalanceAmount
135:   /**
136:     @notice Helper function performing pre rebalance operation sanity checks
137:     @dev Checks that vault is managed, that rebalance was called by manager, and maximum daily operation was not reached 
138:     @param managedVault ManagedVault struct of the vault to rebalance
139:     @param rbData RebalanceData struct of the vault to rebalance
140:     @param vaultsData Cached VaultsDataProvider interface for gas saving
141:    */
142:   function _preRebalanceChecks(
143:     ManagedVault memory managedVault,
144:     IMIMORebalance.RebalanceData calldata rbData,
145:     IVaultsDataProvider vaultsData,
146:     uint256 rebalanceAmount
147:   ) internal view {

contracts/proxy/interfaces/IMIMOProxy.sol
			//@audit missing @return memory
76:   /// @notice Batches multiple proxy calls within a same transaction. 
77:   /// @param targets An array of contract addresses to call 
78:   /// @param data The bytes for each batched function call 
79:   function multicall(address[] calldata targets, bytes[] calldata data) external returns (bytes[] memory);

contracts/proxy/interfaces/IMIMOProxyFactory.sol
			//@audit missing @return result
15:   /// @notice Mapping to track all deployed proxies.
16:   /// @param proxy The address of the proxy to make the check for.
17:   function isProxy(address proxy) external view returns (bool result);

			//@audit missing @return 
19:   /// @notice The release version of PRBProxy.
20:   /// @dev This is stored in the factory rather than the proxy to save gas for end users.
21:   function VERSION() external view returns (uint256);

contracts/proxy/interfaces/IMIMOProxyRegistry.sol
			//@audit missing @return proxy
16:   /// @notice Gets the current proxy of the given owner.
17:   /// @param owner The address of the owner of the current proxy.
18:   function getCurrentProxy(address owner) external view returns (IMIMOProxy proxy);

[N-03] Constructor is missing NatSpec

Findings

contracts/actions/managed/MIMOManagedAction.sol
19:   constructor(IAddressProvider _a, IMIMOProxyRegistry _proxyRegistry) {

contracts/proxy/MIMOProxyFactory.sol
26:   constructor(address _mimoProxyBase) {

[N-04] Typos

Findings

contracts/actions/MIMOVaultActions.sol
				//@audit typo paking
14:   @notice Only intended to be hold the logic for paking delegateCalls from a MIMOProxy 

[N-05] Consider making contract Pausable to have some protection against ongoing exploits

Findings

contracts/proxy/MIMOProxy.sol
12: contract MIMOProxy is IMIMOProxy, Initializable, BoringBatchable {

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

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Findings

Findings

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

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

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

contracts/proxy/interfaces/IMIMOProxyRegistry.sol
2: pragma solidity ^0.8.4;

contracts/proxy/interfaces/IMIMOProxyFactory.sol
2: pragma solidity ^0.8.4;

contracts/proxy/interfaces/IMIMOProxy.sol
2: pragma solidity ^0.8.4;

contracts/actions/MIMOVaultActions.sol
3: pragma solidity 0.8.10;

contracts/actions/MIMOSwap.sol
2: pragma solidity 0.8.10;

contracts/actions/MIMORebalance.sol
2: pragma solidity 0.8.10;

contracts/actions/MIMOLeverage.sol
2: pragma solidity 0.8.10;

contracts/actions/MIMOFlashloan.sol
2: pragma solidity 0.8.10;

contracts/actions/MIMOEmptyVault.sol
2: pragma solidity 0.8.10;

contracts/actions/managed/MIMOManagedRebalance.sol
2: pragma solidity 0.8.10;

contracts/actions/managed/MIMOManagedAction.sol
2: pragma solidity 0.8.10;

contracts/actions/managed/interfaces/IMIMOManagedRebalance.sol
2: pragma solidity 0.8.10;

contracts/actions/managed/interfaces/IMIMOManagedAction.sol
2: pragma solidity 0.8.10;

contracts/actions/automated/MIMOAutoRebalance.sol
2: pragma solidity 0.8.10;

contracts/actions/automated/MIMOAutoAction.sol
2: pragma solidity 0.8.10;

contracts/actions/automated/interfaces/IMIMOAutoRebalance.sol
2: pragma solidity 0.8.10;

contracts/actions/automated/interfaces/IMIMOAutoAction.sol
2: pragma solidity 0.8.10;

[N-07] Event is missing indexed fields

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

Findings

contracts/proxy/interfaces/IMIMOProxyFactory.sol
11:   event DeployProxy(address indexed deployer, address indexed owner, address proxy);

contracts/proxy/interfaces/IMIMOProxy.sol
9:   event Execute(address indexed target, bytes data, bytes response);

[N-08] Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact

Findings

contracts/actions/interfaces/IMIMOEmptyVault.sol
07: interface IMIMOEmtpyVault is IMIMOProxyAction, IMIMOSwap {
08:   function emptyVaultOperation(
09:     IERC20 vaultCollateral,
10:     uint256 vaultId,
11:     uint256 swapAmount,
12:     SwapData calldata swapData
13:   ) external;
14: }

function emptyVaultOperation is without override keyword

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

111:   function emptyVaultOperation(
112:     IERC20 vaultCollateral,
113:     uint256 vaultId,
114:     uint256 swapAmount,
115:     SwapData calldata swapData
116:   ) external {//@audit no override 

[G-01] public functions to external

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

Findings

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

Mitigation

Change public keyword to external;

[G-02] Amounts should be checked for 0 before calling another contract

Checking non-zero values can avoid an expensive external call and save gas.

Findings

contracts/actions/MIMOEmptyVault.sol
82:     vaultCollateral.safeTransfer(address(mimoProxy), amounts[0]);

128:     vaultCollateral.safeTransfer(msg.sender, withdrawAmount);

contracts/actions/MIMORebalance.sol
85:     fromCollateral.safeTransfer(address(mimoProxy), amounts[0]);

128:     core.repay(rbData.vaultId, rbData.mintAmount);

133:     core.withdraw(rbData.vaultId, flashloanRepayAmount);
134:     fromCollateral.safeTransfer(msg.sender, flashloanRepayAmount);

contracts/actions/MIMOVaultActions.sol
48:     collateral.safeTransferFrom(msg.sender, address(this), amount);
49:     collateral.safeIncreaseAllowance(address(core), amount);
50:     core.deposit(address(collateral), amount);

71:     collateral.safeTransferFrom(msg.sender, address(this), depositAmount);
72:     collateral.safeIncreaseAllowance(address(core_), depositAmount);
73:     core_.depositAndBorrow(address(collateral), depositAmount, borrowAmount);

81:     core.depositETHAndBorrow{ value: msg.value }(borrowAmount);

92:     core.withdraw(vaultId, amount);

101:     core.withdrawETH(vaultId, amount);

110:     core.borrow(vaultId, amount);

contracts/actions/MIMOSwap.sol
50:     token.safeIncreaseAllowance(proxy, amount);

contracts/actions/MIMOLeverage.sol
86:     asset.safeTransfer(address(mimoProxy), amounts[0]);

122:     core.depositAndBorrow(address(token), collateralBalanceBefore, swapAmount);

137:     token.safeTransfer(msg.sender, flashloanRepayAmount);

contracts/actions/managed/MIMOManagedRebalance.sol
79:     IERC20(a.stablex()).safeTransfer(managedVault.manager, managerFee);

114:     fromCollateral.safeTransfer(address(mimoProxy), amounts[0]);

contracts/actions/automated/MIMOAutoRebalance.sol
78:     IERC20(a.stablex()).safeTransfer(msg.sender, autoFee);

113:     fromCollateral.safeTransfer(address(mimoProxy), amounts[0]);

Mitigation

Add some amount != 0 checks

[G-03] 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 Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 - 2600 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value.

Findings

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

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

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

contracts/proxy/interfaces/IMIMOProxyRegistry.sol
2: pragma solidity ^0.8.4;

contracts/proxy/interfaces/IMIMOProxyFactory.sol
2: pragma solidity ^0.8.4;

contracts/proxy/interfaces/IMIMOProxy.sol
2: pragma solidity ^0.8.4;

Mitigation

Use a solidity version of at least 0.8.10

[G-04] Unchecked arithmetic

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected. if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas. Check //@audit for details

Findings

contracts/actions/MIMOLeverage.sol
132:     if (collateralBalanceAfter > flashloanRepayAmount) {  //@audit (collateralBalanceAfter - flashloanRepayAmount) can't underflow because of line 132
133:       token.safeIncreaseAllowance(address(core), collateralBalanceAfter - flashloanRepayAmount);
134:       core.deposit(address(token), collateralBalanceAfter - flashloanRepayAmount);

contracts/actions/automated/MIMOAutoAction.sol
097:     if (swapResultValue >= rebalanceValue) {
098:       return true;
099:     }
100: 			//@audit (rebalanceValue - swapResultValue) can't underflow because of L097
101:     uint256 vaultVariation = (rebalanceValue - swapResultValue).wadDiv(rebalanceValue);

contracts/actions/managed/MIMOManagedAction.sol
120:     if (swapResultValue >= rebalanceValue) {
121:       return true;
122:     }
123: 			//@audit (rebalanceValue - swapResultValue) can't underflow because of L120
124:     uint256 vaultVariation = (rebalanceValue - swapResultValue).wadDiv(rebalanceValue);

contracts/actions/automated/MIMOAutoRebalance.sol
244:     if (_operationTracker[vaultId] > block.timestamp - 1 days) {//@audit  block.timestamp - 1 days can't underflow

contracts/actions/managed/MIMOManagedRebalance.sol
160:     if (_operationTracker[rbData.vaultId] > block.timestamp - 1 days) {//@audit  block.timestamp - 1 days can't underflow

Mitigation

Put expressions, that can't underflow/overflow into unchecked{} block.

[G-05] Multiple evaluation of same expression

Calculating same expression more than once wastes gas, especially those, that use checked arithmetic

Findings

contracts/actions/MIMOLeverage.sol
133:       token.safeIncreaseAllowance(address(core), collateralBalanceAfter - flashloanRepayAmount);
134:       core.deposit(address(token), collateralBalanceAfter - flashloanRepayAmount);

Mitigation

Store result of collateralBalanceAfter - flashloanRepayAmount in a variable and use it.

[G-06] Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

Findings

contracts/actions/managed/MIMOManagedAction.sol
17:   mapping(address => bool) internal _managers;

contracts/proxy/MIMOProxy.sol
24:   mapping(address => mapping(address => mapping(bytes4 => bool))) internal _permissions;

Mitigation

Use uint256(1) and uint256(2) for true/false

[G-07] Remove or replace unused components

contracts/proxy/MIMOProxyFactory.sol
18:   /// @inheritdoc IMIMOProxyFactory
19:   uint256 public constant override VERSION = 1;

contracts/proxy/interfaces/IMIMOProxyFactory.sol
19:   /// @notice The release version of PRBProxy.
20:   /// @dev This is stored in the factory rather than the proxy to save gas for end users.
21:   function VERSION() external view returns (uint256);

Remove it to save deployment gas.

[G-08] Use custom errors rather than revert()/require() 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) while providing the same amount of information, as explained here https://blog.soliditylang.org/2021/04/21/custom-errors/

Custom errors are defined using the error statement

Findings

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

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

contracts/actions/MIMORebalance.sol
129:     require(
130:       a.vaultsData().vaultCollateralBalance(rbData.vaultId) >= flashloanRepayAmount,
131:       Errors.CANNOT_REPAY_FLASHLOAN
132:     );

contracts/actions/MIMOSwap.sol
47:     require(proxy != address(0), Errors.INVALID_AGGREGATOR);
48:     require(router != address(0), Errors.INVALID_AGGREGATOR);

Mitigation

Replace require statements with custom errors.

[G-09] Caching storage variables in memory to save gas

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

Findings

contracts/proxy/MIMOProxy.sol
56:     if (owner != msg.sender) {

62:         revert CustomErrors.EXECUTION_NOT_AUTHORIZED(owner, msg.sender, target, selector);

72:     address owner_ = owner;

82:     if (owner_ != owner) {

83:       revert CustomErrors.OWNER_CHANGED(owner_, owner);

owner is read five times, could be:

contracts/proxy/MIMOProxy.sol
55:				address _owner = owner;
56:     if (_owner != msg.sender) {

62:         revert CustomErrors.EXECUTION_NOT_AUTHORIZED(_owner, msg.sender, target, selector);

72:     address owner_ = owner;

82:     if (owner_ != _owner) {

83:       revert CustomErrors.OWNER_CHANGED(owner_, _owner); //@audit now owner is read 2 times

contracts/proxy/MIMOProxy.sol
119:     if (oldOwner != msg.sender) {
120:       revert CustomErrors.NOT_OWNER(oldOwner, msg.sender);

owner is read two times

contracts/proxy/MIMOProxy.sol
128:     if (msg.sender != owner) {
129:       revert CustomErrors.NOT_OWNER(owner, msg.sender);

owner is read two times

Mitigation

Assign storage value to memory variable and use this variable instead of loading from storage multiple times.

[G-10] Variable should be constant

Variable minGasReserve is not changed anywhere, value known at compile time, so it should be constant

Findings

contracts/proxy/MIMOProxy.sol
19:   uint256 public override minGasReserve;

30:     minGasReserve = 5_000;

Mitigation

Change it to: uint256 public constant override minGasReserve = 5_000;