Mimo DeFi contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L126 https://github.com/Uniswap/v2-periphery/blob/2efa12e0f2d808d9b49737927f0e416fafa5af68/contracts/UniswapV2Router02.sol#L299 https://github.com/Uniswap/solidity-lib/blob/c01640b0f0f1d8a85cba8de378cc48469fcfd9a6/contracts/libraries/TransferHelper.sol#L47-L50

Vulnerability details

Impact

A malicious user is able to steal all collateral of an unhealthy position in PARMinerV2.sol. The code for the liquidate() function is written so that the following steps are followed:

• User calls PARMinerV2.liquidate()
• PARMinerV2 performs the liquidation with _a.parallel().core().liquidatePartial()
• PARMinerV2 receives the liquidated collateral
• An arbitrary router function is called to swap the collateral to PAR
• Finally, PARMinerV2.liquidate() checks that PARMinerV2's PAR balance is higher than the balance at the beginning of the function call.

The exploit occurs with the arbitrary router call. The malicious user is able to supply the dexTxnData parameter which dictates the function call to the router. If the user supplied a function such as UniswapV2Router's swapExactTokenForETH(), then control flow will be given to the user, allowing them to perform the exploit.

Note: The Mimo developers have stated that the routers used by the protocol will be DEX Aggregators such as 1inch and Paraswap, but this submission will be referring to UniswapV2Router for simplicity. It can be assumed that the dex aggregators currently allow swapping tokens for ETH.

Continuing the exploit, once the attacker has gained control due to the ETH transfer, they are able to swap the ETH for PAR. Finally, they deposit the PAR with PARMinerV2.deposit(). This will cause the final check of liquidate() to pass because PARMinerV2's PAR balance will be larger than the start of the liquidation call.

The attacker is able to steal all collateral from every unhealthy position that they liquidate. In the most extreme case, the attacker is able to open their own risky positions with the hope that the position becomes unhealthy. They will borrow the PAR and then liquidate themselves to take back the collateral. Thus effectively stealing PAR.

Proof of Concept

Steps for exploit:

• Attacker monitors unhealthy positions. Finds a position to liquidate.
• Attacker calls PARMinerV2.liquidate()
• Position liquidated. Collateral transferred back to PARMinerV2
• In the liquidate() function, attacker supplies bytes for UniswapV2Router.swapExactTokensForETH(uint amountIn, uint amountOutMin, address[] calldata path, address to, uint deadline). For to, they supply the attacker contract.
• swapExactTokensForETH() firstly swaps the collateral for ETH and then transfers the ETH to the user with TransferHelper.safeTransferETH(to, amounts[amounts.length - 1]);
• TransferHelper.safeTransferETH() contains a call to the receiver via (bool success, ) = to.call{value: value}(new bytes(0));
• Therefore, the attacker contract will indeed gain control of execution.

The attacker contract will then perform the following steps:

• Swap the received ETH to PAR.
• Deposit the PAR in PARMinerV2
• Withdraw the deposited PAR.

Tools Used

Static review.

Recommended Mitigation Steps

The arbitrary call to the router contracts is risky because of the various functions that they can contain. Perhaps a solution is to only allow certain calls such as swapping tokens to tokens, not ETH. This would require frequently updated knowledge of the router's functions, though would be beneficial for security.

Also, adding a check that the _totalStake variable has not increased during the liquidation call will mitigate the risk of the attacker depositing the PAR to increase the contract's balance. The attacker would have no option but to transfer the PAR to PARMinerV2 as is intended.

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

Issue https://github.com/code-423n4/2022-04-mimo-findings/issues/3 (Low) - Some tokens do not allow for Non-Zero to Non-Zero value approvals USDT, for example, will not allow approving a non-zero amount and then approving another non-zero amount. The approval must be reset to zero before approving another amount. The following functions will fail on their subsequent calls.

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

Issue #1 (Low) - Follow Check-Effects-Interactions Pattern

There is no reentrancy risk in PARMinerV2.withdraw(), though it is a recommended practice to follow the CEI pattern regardless. Send the withdrawal amount after decreasing the user's stake.

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L101-L102

Issue #2 (Low) - All input addresses should be checked for the zero address

In the following initialize function, the input address for _owner should be required to not equal address(0).

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

Issue #3 (Low) - Some tokens do not allow for Non-Zero to Non-Zero value approvals

USDT, for example, will not allow approving a non-zero amount and then approving another non-zero amount. The approval must be reset to zero before approving another amount. The following functions will fail on their subsequent calls.

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

Issue #4 (Low) - Front-runnable initializers

A couple initialize functions lack access control. They can be frontrun allowing for crucial variables to be improperly set. This would lead to the contract needing to be redeployed.

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/inception/InceptionVaultsCore.sol#L40-L58 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L49-L67

Issue #5 (Non-critical) - Consider renaming stake() to getStaked()

stake() sounds like an action, e.g. for a user to stake the token. Changing to something like getStaked() would indicate that it's a getter function.

Issue #6 (Non-critical) - Code Consistency: Change _userInfo.accAmountPerShare to _userInfo.accMimoAmountPerShare

    _userInfo.accAmountPerShare = _accMimoAmountPerShare;
    _userInfo.accParAmountPerShare = _accParAmountPerShare;

Both PAR variables contain "PAR". Only the global MIMO variable contains "MIMO". The user variable should contain "MIMO" in the name as well.

Issue #7 (Non-critical) - Typo in word "Collateral"

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L106

Issue #8 (Non-critical) - Typo in word "Threshold"

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

Issue #9 (Non-critical) - Recommended practice to begin internal variables with underscore

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L102 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L137 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L187 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L368 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L354

Issue #10 (Non-critical) - Missing require exception messages

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L255 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L247

Issue #1 - Multiple require statements saves gas over &&

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L71-L71

Issue #2 - Unnecessary multiple transfers

In the following code, a transfer is made from msg.sender to inceptionVaultsCore. Then another transfer is made from inceptionVaultsCore to address(_adminInceptionVault). Might as well just transfer from msg.sender to address(_adminInceptionVault).

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

Issue #3 -

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/oracles/BalancerV2LPOracle.sol#L118-L124

The updatedAt variable is firstly set, then checked if it should be overwritten. Change to a ternary operator and only write the variable once.

updatedAt = assetUpdatedAtA > assetUpdatedAtB ? assetUpdatedAtB : assetUpdatedAtA;