Mimo August 2022 contest - TomJ's results

Table of Contents

Low Risk Issues

• Admin Address Change should be a 2-Step Process
• Missing Zero Address Check

Non-critical Issues

• Event is Missing Indexed Fields

 

Low Risk Issues

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process. This can be a concern since an admin / owner role has a high privilege in the contract and mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

Total of 1 issue found.

1. owner privilege of MIMOProxy.sol https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L117-L124

Mitigation

This can be fixed by implementing 2-step process. We can do this by following. First make the setAdmin function approve a new address as a pending admin. Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

 

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally.

PoC

Total of 1 issue found.

• MIMOProxyFactory.sol: 1 address of "mimoProxyBase" https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyFactory.sol#L26-L28

Mitigation

Add 0-address check for above addresses.

 

Non-critical Issues

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

Total of 5 issues found.

./proxy/interfaces/IMIMOProxy.sol:9:  event Execute(address indexed target, bytes data, bytes response);
./proxy/interfaces/IMIMOProxyFactory.sol:11:  event DeployProxy(address indexed deployer, address indexed owner, address proxy);
./actions/managed/interfaces/IMIMOManagedAction.sol:22:  event ManagerSet(address manager, bool isManager);
./actions/managed/interfaces/IMIMOManagedAction.sol:23:  event ManagementSet(uint256 vaultId, ManagedVault managedVault);
./actions/automated/interfaces/IMIMOAutoAction.sol:25:  event AutomationSet(uint256 vaultId, AutomatedVault autoVault);

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

• Should Use Unchecked Block where Over/Underflow is not Possible
• Minimize the Number of SLOADs by Caching State Variable
• Redundant Use of Variable
• Both named returns and return statement are used
• Unchanging State Variable Should be Immutable
• Change Function Visibility Public to External
• Internal Function Called Only Once can be Inlined
• Unnecessary Default Value Initialization
• ++i Costs Less Gas than i++
• Empty Blocks Should Emit Something or be Removed

 

Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

PoC

Total of 2 issues found.

1. leverageOperation() of MIMOLeverage.sol

Wrap line 133 and 134 with unchecked since underflow is not possible due to line 132 check
132:    if (collateralBalanceAfter > flashloanRepayAmount) {
133:      token.safeIncreaseAllowance(address(core), collateralBalanceAfter - flashloanRepayAmount);
134:      core.deposit(address(token), collateralBalanceAfter - flashloanRepayAmount);

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

Mitigation

Wrap the code with uncheck like described in above PoC.

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

Total of 2 issue found.

1. Cache owner of setPermission() of MIMOProxy.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxy.sol#L110-L111

2. Cache owner of multicall() of MIMOProxy.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxy.sol#L128-L129

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Redundant Use of Variable

Issue

Below has redundant use of variables. Instead of defining a new variable, emit the event first and then update the variable.

PoC

Total of 1 issue found.

1. transferOwnership function of MIMOProxy.sol https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxy.sol#L117-L124 Change it to

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

Mitigation

Instead of defining a new variable, emit the event first and then update the variable like shown in above PoC.

 

Both named returns and return statement are used

Issue

In some function return statement are used even though named returns is set. This is redundant because return statement is not needed when using named returns and named returns is not needed when using return statement. Removing unused named returns variable in below code can save gas and improve code readability.

PoC

Total of 3 issues found.

1. Remove returns variable "rebalanceAmount", "mintAmount" and "autoFee" of getAmounts() of MIMOAutoRebalance.sol https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L147-L149

Mitigation

Remove unused named returns variable as mentioned in above PoC.

 

Unchanging State Variable Should be Immutable

Issue

State variable that is only set in the constructor/initializer and can't be changed afterwards, should be declared as immutable.

PoC

Total of 2 issues found.

1. minGasReserve variable of MIMOProxy.sol https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L19

2. factory variable of MIMOProxyRegistry.sol https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyRegistry.sol#L16

 

Change Function Visibility Public to External

Issue

If the function is not called internally, it is cheaper to set your function visibility to external instead of public.

PoC

Total of 1 issue found.

MIMOProxy.sol:setPermission() https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/proxy/MIMOProxy.sol#L104-L114

104:  function setPermission(
105:    address envoy,
106:    address target,
107:    bytes4 selector,
108:    bool permission
109:  ) public override {

Mitigation

Change the function visibility to external.

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 6 issues found.

1. _isVaultVariationAllowed() of MIMOManagedAction.sol Called only once at line 195 of MIMOManagedRebalance.sol https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedAction.sol#L115-L122 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L195

2. _isVaultVariationAllowed() of MIMOAutoAction.sol Called only once at line 227 of MIMOAutoRebalance.sol https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoAction.sol#L92-L99 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L277

3. _preRebalanceChecks() of MIMOManagedRebalance.sol Called only once at line 58 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L142-L166 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L58

4. _preRebalanceChecks() of /MIMOAutoRebalance.sol Called only once at line 59 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L236-L250 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L59

5. _postRebalanceChecks() of MIMOManagedRebalance.sol Called only once at line 67 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L179-L211 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/managed/MIMOManagedRebalance.sol#L67

6. _postRebalanceChecks() of MIMOAutoRebalance.sol Called only once at line 74 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L262-L286 https://github.com/code-423n4/2022-08-mimo/blob/main/contracts/actions/automated/MIMOAutoRebalance.sol#L74

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 1 issue found.

./proxy/MIMOProxy.sol:132:    for (uint256 i = 0; i < targets.length; i++) {

Mitigation

I suggest removing default value initialization. For example,

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

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

Total of 1 issue found.

./proxy/MIMOProxy.sol:132:    for (uint256 i = 0; i < targets.length; i++) {

Mitigation

Change it to postfix increments/decrements. It saves 6 gas per loop. For example,

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

 

Empty Blocks Should Emit Something or be Removed

Issue

There are function with empty blocks. These should do something like emit an event or reverting. If not it should be removed from the contract.

PoC

Total of 2 issues found.

./proxy/MIMOProxy.sol:38:  receive() external payable {}
./actions/MIMOFlashloan.sol:44:  ) external virtual override returns (bool) {}

Mitigation

Add emit or revert in the function block.