QA Report #124

code423n4 · 2022-05-02T21:56:19Z

C4-001 :Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Impact - LOW

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

Proof of Concept

Navigate to the following contract.
transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.

  2022-04-mimo-main/core/contracts/liquidityMining/MinerPayer.sol::91 => a.mimo().transfer(_payee, payment);
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::77 => require(a.mimo().transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::91 => require(par.transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::184 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::187 => require(par.transfer(user, pendingPAR));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::232 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PolygonDistributor.sol::47 => a.mimo().approve(erc20Predicate, payment);
  2022-04-mimo-main/core/contracts/liquidityMining/VotingMiner.sol::33 => require(a.mimo().transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::220 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::223 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::58 => _par.approve(address(_a.parallel().core()), uint256(-1));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::125 => collateralToken.approve(proxy, collateralToken.balanceOf(address(this)));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::320 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::323 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::44 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::47 => require(_par.transfer(_user, pendingPAR), "LM100");

Tools Used

Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

C4-002 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

Navigate to the following contract.

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/inception/InceptionVaultsCore.sol#L51

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-003 : # Missing Re-entrancy Guard

Impact - LOW

The re-entrancy guard is missing on the some of the functions. The external interactions can cause to the re-entrancy vulnerability.

Proof of Concept

Navigate to the following contract.

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/liquidityMining/PARMiner.sol#L52

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/liquidityMining/PARMiner.sol#L61

Tools Used

Code Review

Recommended Mitigation Steps

Follow the check effect interaction pattern or put re-entrancy guard.

C4-004 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

Navigate to the following contract.

 2022-04-mimo-main/supervaults/contracts/SuperVault.sol::97 => asset.approve(address(lendingPool), flashloanRepayAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::129 => IERC20(asset).transferFrom(msg.sender, address(this), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::149 => IERC20(toCollateral).approve(address(a.core()), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::199 => par.approve(address(a.core()), par.balanceOf(address(this)));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::233 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::237 => collateral.transfer(msg.sender, collateral.balanceOf(address(this)));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::247 => require(asset.transfer(msg.sender, amount));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::255 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::264 => require(token.transfer(msg.sender, token.balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::273 => token.approve(address(a.core()), amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::274 => token.transferFrom(msg.sender, address(this), amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::289 => token.approve(address(a.core()), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::290 => token.transferFrom(msg.sender, address(this), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::292 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::313 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::326 => token.approve(address(a.core()), 2**256 - 1);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::345 => token.approve(proxy, amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::370 => require(ga.mimo().transfer(msg.sender, ga.mimo().balanceOf(address(this))));

Tools Used

Manual Code Review

Recommended Mitigation Steps

Ensure that to check previous balance/after balance equals to amount for any rebasing/inflation/deflation
Add support in contracts for such tokens before accepting user-supplied tokens
Consider supporting deflationary / rebasing / etc tokens by extra checking the balances before/after or strictly inform your users not to use such tokens if they don't want to lose them.

C4-005 : # Pragma Version

Impact

In the contracts, floating pragmas should not be used. 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.

## Proof of Concept

https://swcregistry.io/docs/SWC-103

All Contracts

Tools Used

Manual code review

Recommended Mitigation Steps

Lock the pragma version: delete pragma solidity 0.8.10 in favor of pragma solidity 0.8.10

C4-006 : # The Contract Should Approve(0) first

Impact

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).approve(address(operator), 0);
IERC20(token).approve(address(operator), amount);

Proof of Concept

Navigate to the following contracts.

 2022-04-mimo-main/supervaults/contracts/SuperVault.sol::97 => asset.approve(address(lendingPool), flashloanRepayAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::129 => IERC20(asset).transferFrom(msg.sender, address(this), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::149 => IERC20(toCollateral).approve(address(a.core()), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::199 => par.approve(address(a.core()), par.balanceOf(address(this)));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::233 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::237 => collateral.transfer(msg.sender, collateral.balanceOf(address(this)));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::247 => require(asset.transfer(msg.sender, amount));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::255 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::264 => require(token.transfer(msg.sender, token.balanceOf(address(this))));
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::273 => token.approve(address(a.core()), amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::274 => token.transferFrom(msg.sender, address(this), amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::289 => token.approve(address(a.core()), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::290 => token.transferFrom(msg.sender, address(this), depositAmount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::292 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::313 => require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::326 => token.approve(address(a.core()), 2**256 - 1);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::345 => token.approve(proxy, amount);
  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::370 => require(ga.mimo().transfer(msg.sender, ga.mimo().balanceOf(address(this))));

  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::220 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::223 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::58 => _par.approve(address(_a.parallel().core()), uint256(-1));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::125 => collateralToken.approve(proxy, collateralToken.balanceOf(address(this)));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::320 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::323 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::44 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::47 => require(_par.transfer(_user, pendingPAR), "LM100");

When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-007 : # USE SAFEERC20.SAFEAPPROVE

Impact

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().

Proof of Concept

  2022-04-mimo-main/core/contracts/liquidityMining/GenericMiner.sol::47 => require(a.mimo().transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/GenericMiner.sol::102 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/GenericMiner.sol::129 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/MIMOBuyBack.sol::36 => PAR.approve(address(balancer), 2**256 - 1);
  2022-04-mimo-main/core/contracts/liquidityMining/MIMOBuyBack.sol::51 => require(MIMO.transfer(destination, MIMO.balanceOf(address(this))));
  2022-04-mimo-main/core/contracts/liquidityMining/MIMOBuybackUniswapV2.sol::35 => PAR.approve(address(router), 2**256 - 1);
  2022-04-mimo-main/core/contracts/liquidityMining/MIMOBuybackUniswapV2.sol::50 => require(MIMO.transfer(destination, MIMO.balanceOf(address(this))));
  2022-04-mimo-main/core/contracts/liquidityMining/MinerPayer.sol::91 => a.mimo().transfer(_payee, payment);
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::77 => require(a.mimo().transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::91 => require(par.transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::184 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::187 => require(par.transfer(user, pendingPAR));
  2022-04-mimo-main/core/contracts/liquidityMining/PARMiner.sol::232 => require(a.mimo().transfer(user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/PolygonDistributor.sol::47 => a.mimo().approve(erc20Predicate, payment);
  2022-04-mimo-main/core/contracts/liquidityMining/VotingMiner.sol::33 => require(a.mimo().transfer(_user, pending));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::220 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/GenericMinerV2.sol::223 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::58 => _par.approve(address(_a.parallel().core()), uint256(-1));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::125 => collateralToken.approve(proxy, collateralToken.balanceOf(address(this)));
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::320 => require(_par.transfer(_user, pendingPAR), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/PARMinerV2.sol::323 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::44 => require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
  2022-04-mimo-main/core/contracts/liquidityMining/v2/VotingMinerV2.sol::47 => require(_par.transfer(_user, pendingPAR), "LM100");

Tools Used

Manual Code Review

Recommended Mitigation Steps

Update to _token.safeApprove(spender, type(uint256).max)

C4-008 : # USE OF DEPRECATED _SETUPROLE FUNCTION

Impact

The contract SuperVault.sol make use of the deprecated function _setupRole from the AccessControl contract. As per the AccessControl.sol contract documentation, this function is deprecated:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L183

Using deprecated functions may eventually produce an unwanted behaviour, for example, if OpenZeppelin decides to remove or update the function.

Proof of Concept

  2022-04-mimo-main/supervaults/contracts/SuperVault.sol::66 => _setupRole(DEFAULT_ADMIN_ROLE, _owner);

Tools Used

Manual testing

Recommended Mitigation Steps

It is recommended to use the _grantRole function instead.

C4-009 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

Navigate to the following contracts.

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/inception/InceptionVaultsCore.sol#L40

https://github.com/code-423n4/2022-04-mimo/blob/main/supervaults/contracts/SuperVault.sol#L49

initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-010 : Missing events for only functions that change critical parameters

Impact - Non critical

The functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.

Proof of Concept

Navigate to the following contract.

https://github.com/code-423n4/2022-04-mimo/blob/main/supervaults/contracts/SuperVault.sol#L264

https://github.com/code-423n4/2022-04-mimo/blob/main/supervaults/contracts/SuperVault.sol#L253

https://github.com/code-423n4/2022-04-mimo/blob/main/supervaults/contracts/SuperVault.sol#L244

See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)

Tools Used

None

Recommended Mitigation Steps

Add events to all functions that change critical parameters.

The text was updated successfully, but these errors were encountered:

m19 · 2022-05-09T02:32:11Z

This QA report also stands out as it reports almost 50% of all issues found during this audit.

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 2, 2022

code423n4 added a commit that referenced this issue May 2, 2022

defsec issue #124

476c87e

This was referenced Jun 5, 2022

Upgraded Q -> M from 124 [1654442937681] #168

Closed

Upgraded Q -> M from 124 [1654443300023] #171

Closed

Upgraded Q -> M from 124 [1654443719989] #172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #124

QA Report #124

code423n4 commented May 2, 2022

m19 commented May 9, 2022

QA Report #124

QA Report #124

Comments

code423n4 commented May 2, 2022

C4-001 :Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Impact - LOW

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-002 : Use of Block.timestamp

Impact - Non-Critical

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-003 : # Missing Re-entrancy Guard

Impact - LOW

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-004 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-005 : # Pragma Version

Impact

## Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-006 : # The Contract Should Approve(0) first

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-007 : # USE SAFEERC20.SAFEAPPROVE

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-008 : # USE OF DEPRECATED _SETUPROLE FUNCTION

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-009 : Front-runnable Initializers

Impact - LOW

Proof of Concept

Tools Used

Recommended Mitigation Steps

C4-010 : Missing events for only functions that change critical parameters

Impact - Non critical

Proof of Concept

Tools Used

Recommended Mitigation Steps

m19 commented May 9, 2022