Mimo DeFi contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L125 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L97 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L151 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L275 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L291 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L325 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L344

Vulnerability details

Impact

Some tokens, like USDT (see requirement line 199), require first reducing the address allowance to 0 by calling approve(_spender, 0) and then approve the actual allowance.

When using one of these unsupported tokens, all transactions revert and the protocol cannot be used.

Proof of Concept

core/contracts/liquidityMining/v2/PARMinerV2.sol#L125
supervaults/contracts/SuperVault.sol#L97
supervaults/contracts/SuperVault.sol#L151
supervaults/contracts/SuperVault.sol#L275
supervaults/contracts/SuperVault.sol#L291
supervaults/contracts/SuperVault.sol#L325
supervaults/contracts/SuperVault.sol#L344

Tools Used

Manual review

Recommended mitigation steps

Approve with a zero amount first before setting the actual amount. e.g:

token.approve(proxy, 0); // @audit-info add this line to reduce allowance to 0 first
token.approve(proxy, amount);

QA Report

Table of Contents

• Non-Critical Findings
  • [NC-01] - Consider upgrading Solidity version
  • [NC-02]: Events not indexed
• Low Risk
  • [L-01] - Functions not returning declared values

Non-Critical Findings

[NC-01] - Consider upgrading Solidity version

Description

Consider using a newer Solidity version instead of using 0.6.12.

More recent versions of solidity have compiler optimizations, user defined types (this would be very useful in the concentratedLiquidityPools code), overriding interface functions, reading from immutables, among other things. This could help reading and writing safe and clean code.

Findings

Excerpt of findings:

liquidityMining/v2/GenericMinerV2.sol#L4
liquidityMining/v2/SupplyMinerV2.sol#L4
liquidityMining/v2/DemandMinerV2.sol#L4
liquidityMining/v2/PARMinerV2.sol#L4
liquidityMining/v2/VotingMinerV2.sol#L4
liquidityMining/v2/interfaces/IDemandMinerV2.sol#L4
liquidityMining/v2/interfaces/IGenericMinerV2.sol#L4
liquidityMining/v2/interfaces/ISupplyMinerV2.sol#L4
liquidityMining/v2/interfaces/IVotingMinerV2.sol#L4
oracles/GUniLPOracle.sol#L3
oracles/BalancerV2LPOracle.sol#L3

Recommended mitigation steps

Inherit from the missing interface or contract.

[NC-02]: Events not indexed

Description

Indexed event parameters are stored in the topics part of the log instead of the data part, which allows for faster indexing/querying because of the use of bloom filters for topics. Up to three parameters in every event can be indexed. While this costs a little extra gas, doing so allows for faster and more efficient/effective event lookups.

Findings

liquidityMining/v2/interfaces/IDemandMinerV2.sol#L9
inception/interfaces/IInceptionVaultFactory.sol#L26
inception/interfaces/IInceptionVaultFactory.sol#L33
inception/interfaces/IInceptionVaultPriceFeed.sol#L10

Recommended mitigation steps

Add indexed parameter especially for address parameters where their faster lookup for security monitoring issues can be a good trade-off for the extra gas consumed.

Low Risk

[L-01] - Functions not returning declared values

Description

Functions mentioned in findings declare multiple return values without actually returning them and therefore do not return anything so they always get assigned a default value of 0.

Findings

core/contracts/oracles/GUniLPOracle.sol#L96
core/contracts/oracles/GUniLPOracle.sol#L98
core/contracts/oracles/GUniLPOracle.sol#L100
core/contracts/oracles/BalancerV2LPOracle.sol#L93
core/contracts/oracles/BalancerV2LPOracle.sol#L95
core/contracts/oracles/BalancerV2LPOracle.sol#L97

Recommended mitigation steps

Either remove the return declarations or return the intended values.