Mimo August 2022 contest - c3phas's results

QA

Missing checks for address(0x0) when assigning values to address state variables

File: MIMOProxyFactory.sol Line 27

    mimoProxyBase = _mimoProxyBase;

Constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: MIMOAutoRebalance.sol Line 180 1e15

    uint256 targetRatio = autoVault.targetRatio + 1e15; // add 0.1% to account for rounding

Typos

File: MIMORebalance.sol Line 57

    @param assets Address array with one element corresponding to the address of the reblanced asset

reblanced instead of rebalanced

File: MIMOAutoRebalance.sol Line 84

    @param assets Address array with one element corresponding to the address of the reblanced asset

reblanced instead of rebalanced

File: MIMOAutoRebalance.sol Line 157

    @notice Helper function calculating the amount to rebalance from vault A and to mint from vault B with rebalnce formula

rebalnce instead of rebalance

FINDINGS

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File: MIMOProxy.sol Line 132

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

see resource

Cache the length of arrays in loops (saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: MIMOProxy.sol Line 132

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

The above should be modified to

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

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++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

Instances include: File: MIMOProxy.sol Line 132

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

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.

See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Instances affected include File: MIMOManagedAction.sol Line 17

  mapping(address => bool) internal _managers;

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource

File:MIMOLeverage.sol Line 133

      token.safeIncreaseAllowance(address(core), collateralBalanceAfter - flashloanRepayAmount);

The operation collateralBalanceAfter - flashloanRepayAmount cannot underflow due to the check on Line 132 which ensures that collateralBalanceAfter is greater than flashloanRepayAmount before perfoming the subtraction

File:MIMOLeverage.sol Line 134

      core.deposit(address(token), collateralBalanceAfter - flashloanRepayAmount);

The operation collateralBalanceAfter - flashloanRepayAmount cannot underflow due to the check on Line 132 which ensures that collateralBalanceAfter is greater than flashloanRepayAmount before perfoming the subtraction