Mimo DeFi contest - Picodes's results

Lines of code

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#L169

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept - rebalance

When using Aave's flashLoan, a recipient can be specified. Therefore the access control of executeOperation in SuperVault is useless as anyone could take a flashloan to call it.

https://github.com/aave/protocol-v2/blob/61c2273a992f655c6d3e7d716a0c2f1b97a55a92/contracts/protocol/lendingpool/LendingPool.sol#L471

Now assume using I use this to call rebalanceOperation in someone else's vault, with a tiny swap and a tiny rebalancing, I can call withdraw here https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L154.

This will repay my flashloan ! Therefore anyone could bypass the Access Control and make operations on behalf of the vault owner.

Proof of Concept - emptyVault

Same goes for emptyVaultOperation: anyone can call it through a flashloan to withdraw from the owner's vault. Here's how you would do it.

Do a flashloan of collateral to call executeOperation and then emptyVaultOperation. Here, swap some of the borrowed collateral to PAR, so that the debt is repaid. Then use the vault's collateral to reimburse the flashloan.

This would repay debt on behalf of users, which is in itself a critical finding.

Extract funds

But also there may be some idle assets on the contract at the end of such attacks. Then these assets could be extracted using a call to aggregatorSwap (using the same process, that is to say through executeOperations . During this call to aggregatorSwap, idle assets would be swapped and the proceed would go a specified address. For example using destReceiver of OneInch: https://docs.1inch.io/docs/aggregation-protocol/api/swap-params

Mitigation steps

In emptyVault, rebalance, leverage, use a temporary variable isExecutingOperation that is set to true at the beginning of the execution and reset to false at the end, and then in executeOperation add a require(isExecutingOperation). This way we can be sure that it's the flashloan called by this contract that is running.

[NC-01] - Enforce call to initialize in factory

At proxy deployment, the code simply transfers a received data to the new proxy: https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVaultFactory.sol#L23. As the fonction to call is known in advance, it would be less error prone to enforce the call to initialize. Here, one could deploy the proxy with a call to a view function for example, which would lead to an incorrect deployment.

[NC-02] - Incorrect Flashloan interface imported

Although this is correct like this as the interface did not changed, it is specified multiple times in SuperVault that it uses Aave V2, but the interface for flashloans is taken from Aave V3: https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L9.

Out of correctness, it would be better to use the V2 interface that can be found here: https://github.com/aave/protocol-v2/blob/master/contracts/interfaces/ILendingPool.sol

[Question] - Why not respecting the convention _name for internal functions ?

Developpers choose to not respect the usual naming convention for internal functions. Is there any reason for doing so ? If not readibility could be improved.

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

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

Use Custom error message instead of revert strings

As the code is already using a compiler version >= 0.8.4, there is a more convenient and gas-efficient way to handle revert strings: custom errors. Custom errors decrease both deploy and runtime gas costs. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/

Recommended Mitigation Steps Refactor code and use Solidity custom errors.

Useless access control check (also a high risk finding)

The following check is useless: https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L83 as anyone could take a flashloan of a tiny amount to call this function with the arguments of its choice. Indeed when taking a flashloan you can choose the target and data to pass to executeOperation

See https://docs.aave.com/developers/v/2.0/guides/flash-loans: