Mimo DeFi contest - pauliax's results

Judge has assessed an item in Issue #137 as Medium risk. The relevant finding follows:

It does not even check the return value and a low-level call succeeds if the address is empty or non-existent. dexMapping is a manually operated config so it may not contain info for all collateral tokens, and in such case, the collateral will remain in PARMinerV2 contract.

It is not clear why AdminInceptionVault and InceptionVaultFactory have a variable _weth, because it is not used in any meaningful way.

function _updateStake declares to return bool but actually does not return anything. Either remove the return declaration or return true/false as per intentions.

SafeMath is only meant to be used with uint256 type, using it with other types does not prevent overflows/underflows:

contract ChainlinkInceptionPriceFeed using SafeMath for uint8;

oracleDecimals.add(collateralDecimals) In this specific case, I did not find any serious issue with this, as you use it in a limited way but still you should at least know it and consider casting decimals to uint256 before adding them.

Careful with possible re-entrancy paths, e.g. in function releaseRewards, make sure you trust all the external contracts and tokens that you call. There are many places where Check-Effects-Interactions pattern is not followed opening gates for potential re-entrancy. Because this is a pretty well-known attack path I expect that you are aware of it and know how to protect from it.

Because depositToVault or depositAndBorrowFromVault accept any asset, it might be better to validate the balance before/after to know the actual amount transferred which may differ with deflationary or other weird tokens:

function depositToVault(address asset, uint256 amount) external { ... token.transferFrom(msg.sender, address(this), amount); ... } Consider using the Safe ERC20 library and its safeApprove to handle approvals of tokens. But then do not forget to set approval to 0 before setting it to another value.

  function _getNormalizedBalance(address token, uint256 balance) internal view returns (uint256) {
    uint8 decimals = ERC20(token).decimals();
    return balance.mul(MathPow.pow(10, 18 - decimals));
  }

• Please give more meaningful names to variables to ease the work of auditing:

  IAddressProvider private _a;

• Misleading variable name:

  uint256 internal constant _MAX_INT = 2**256 - 1;

First, it is UINT, not INT, and second, it is not a max value, it is max - 1. There is a built-in keyword for a max value: type(uint256).max

• DexSet is never emitted, probably an intention was to emit on setDexMapping:

  event DexSet(uint8);

• function _getNormalizedBalance in BalancerV2LPOracle should validate that decimals <= 18 or use SafeMath to prevent unexpected underflow here:

  function _getNormalizedBalance(address token, uint256 balance) internal view returns (uint256) {
    uint8 decimals = ERC20(token).decimals();
    return balance.mul(MathPow.pow(10, 18 - decimals));
  }

Constructor of the GUniLPOracle should also validate that here:

  _tokenDecimalsOffsetA = 10**(18 - decimalsA);
  _tokenDecimalsOffsetB = 10**(18 - decimalsB);

• Do not cast explicitly without proper checks, better use SafeCast library to prevent unexpected results:

  answer = int256(fairResA.mul(pxA).add(fairResB.mul(pxB)).div(pool.totalSupply()));

• function claimMimo iterates over the _collateralCount which in theory is unbounded because it is incremented every time an owner calls depositAndBorrow or borrow with a new _collateralType. If the _collateralCount grows too large, it may lead to a denial of service when claimMimo cannot succeed due to the gas exceeding block limits. Make sure to prevent that, one possible solution is to have a reasonable limit for the maximum _collateralCount.

• PARMinerV2 function liquidate does not validate that dexMapping is set, that is proxy and router addresses are not empty:

  (address proxy, address router) = _dexAP.dexMapping(dexIndex);
  collateralToken.approve(proxy, collateralToken.balanceOf(address(this)));
  router.call(dexTxData);

It does not even check the return value and a low-level call succeeds if the address is empty or non-existent. dexMapping is a manually operated config so it may not contain info for all collateral tokens, and in such case, the collateral will remain in PARMinerV2 contract.

• It is not clear why AdminInceptionVault and InceptionVaultFactory have a variable _weth, because it is not used in any meaningful way.

• function _updateStake declares to return bool but actually does not return anything. Either remove the return declaration or return true/false as per intentions.

• SafeMath is only meant to be used with uint256 type, using it with other types does not prevent overflows/underflows:

  contract ChainlinkInceptionPriceFeed
  using SafeMath for uint8;
  
  oracleDecimals.add(collateralDecimals)

In this specific case, I did not find any serious issue with this, as you use it in a limited way but still you should at least know it and consider casting decimals to uint256 before adding them.

• Careful with possible re-entrancy paths, e.g. in function releaseRewards, make sure you trust all the external contracts and tokens that you call. There are many places where Check-Effects-Interactions pattern is not followed opening gates for potential re-entrancy. Because this is a pretty well-known attack path I expect that you are aware of it and know how to protect from it.

• Because depositToVault or depositAndBorrowFromVault accept any asset, it might be better to validate the balance before/after to know the actual amount transferred which may differ with deflationary or other weird tokens:

 function depositToVault(address asset, uint256 amount) external {
    ...
    token.transferFrom(msg.sender, address(this), amount);
    ...
  }

• Consider using the Safe ERC20 library and its safeApprove to handle approvals of tokens. But then do not forget to set approval to 0 before setting it to another value.

• Repeated external calls should be cached when the values between calls do not change, e.g. here ga.mimo() is queried 3 times:

  if (ga.mimo().balanceOf(address(this)) > 0) {
    require(ga.mimo().transfer(msg.sender, ga.mimo().balanceOf(address(this))));
  }

a.core() queried twice:

  token.approve(address(a.core()), amount);
  ...
  a.core().deposit(asset, amount);

a.stablex() twice:

  require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));

_a.controller() twice:

  require(_a.controller().hasRole(_a.controller().MANAGER_ROLE(), msg.sender), "LM010");

token.balanceOf(address(this) twice:

    require(token.balanceOf(address(this)) >= flashloanRepayAmount, "SV101");
    a.core().deposit(address(token), token.balanceOf(address(this)) - flashloanRepayAmount);

a.core() 4 times:

    IERC20(toCollateral).approve(address(a.core()), depositAmount);

    a.core().depositAndBorrow(toCollateral, depositAmount, parAmount);
    a.core().repay(vaultId, parAmount);

    a.core().withdraw(vaultId, flashloanRepayAmount);

There are many more places where this gas-consuming pattern is used, please consider refactoring it by caching these values.

• Repeated storage access could also be cached to reduce gas usage, e.g. _feeConfig depositFee/withdrawFee read twice here:

    if (_feeConfig.depositFee > 0) {
      uint256 fee = amount.wadMul(_feeConfig.depositFee);

    if (_feeConfig.withdrawFee > 0) {
      uint256 fee = amount.wadMul(_feeConfig.withdrawFee);

• Using uint8 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

  uint8 private _collateralCount;
  mapping(uint8 => address) private _collaterals;
  mapping(address => uint8) private _collateralId;

• Value of _a.controller().MANAGER_ROLE() never changes, it is a constant, so no need for repeatedly calling, it can be queried and cached once in the constructor:

   modifier onlyManager() {
    require(_a.controller().hasRole(_a.controller().MANAGER_ROLE(), msg.sender), "LM010");
    _;
  }

• It would be cheaper to fail early, e.g. in function withdraw of DemandMinerV2 it might first _decreaseStake and only then perform other actions, so in case the amount is not sufficient, it will waste less gas for the user. Also, it will follow a general good practice of Check-Effects-Interaction that is more robust against re-entrancy attacks.

• No need for SafeMath operations when you know it can't overflow/underflow, e.g. here:

    if (stake > oldStake) {
      _increaseStake(user, stake.sub(oldStake));
    }
    if (stake < oldStake) {
      _decreaseStake(user, oldStake.sub(stake));
    }

• It should be cheapier to cache the storage variable that is accessed inside the loop and eliminate math operation here:

    for (uint8 i = 1; i < _collateralCount + 1; i++)

Proposed improvement:

    uint256 _collateralCountLocal = _collateralCount;
    for (uint8 i = 1; i <= _collateralCountLocal; i++)

• You might want to re-order these lines so that in case the transfer from user fails it will not charge extra gas for the approval:

    token.approve(address(a.core()), amount);
    token.transferFrom(msg.sender, address(this), amount);

    token.approve(address(a.core()), depositAmount);
    token.transferFrom(msg.sender, address(this), depositAmount);

or when amount is not enough or other problems arise, it will not execute the external transfer upfront:

  function withdraw(uint256 amount) public {
    _par.safeTransfer(msg.sender, amount);
    _decreaseStake(msg.sender, amount);
  }