compound_rari_fork/Comptroller.sol:1012: // TODO: custom liquidation incentive for LP shares
vault_and_oracles/UniV3LpVault.sol:142: // TODO: do we want some Comptroller like error handling/messaging here?
vault_and_oracles/UniV3LpVault.sol:166: // TODO: do we want a "decreaseLiquidityAndCollect" function?
CErc20.sol:initialize() is front-runnable
approve should be replaced with safeApprove
approve is subject to a known front-running attack. Consider using safeApprove instead:
vault_and_oracles/FlashLoan.sol:20: ILendingPoolAddressesProvider public immutable override ADDRESSES_PROVIDER;
vault_and_oracles/FlashLoan.sol:21: ILendingPool public immutable override LENDING_POOL;
vault_and_oracles/FlashLoan.sol:22: IUniV3LpVault public immutable LP_VAULT;
vault_and_oracles/MasterPriceOracle.sol:13: address public immutable WETH_ADDRESS;
vault_and_oracles/UniswapTwapOracle.sol:31: address public immutable WETH_ADDRESS;
vault_and_oracles/UniswapTwapOracle.sol:36: address public immutable nfpManager;
vault_and_oracles/UniswapTwapOracle.sol:41: address public immutable factory;
Missing comments
The following comments are missing (see @audit tags):
contracts/compound_rari_fork/Comptroller.sol:
471: * @param repayAmount The amount of underlying being repaid //@audit missing @return uint256
521: * @param repayAmount The amount of underlying being repaid //@audit missing @return uint256
573: * @param seizeTokens The number of collateral tokens to seize //@audit missing @return uint256
692: * @return (possible error code (semi-opaque), //@audit missing @param account
717: * @return (possible error code, //@audit missing @param account
1062: * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details) //@audit missing @param newOracle
1103: * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details) //@audit missing @param newVault
1518: * @param cToken The market to check if deprecated //@audit missing @return bool
contracts/compound_rari_fork/CToken.sol:
24: * @param decimals_ EIP-20 decimal precision of this token //@audit missing @param reserveFactorMantissa_
1399: * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details) //@audit missing @param newComptroller
1418: * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details) //@audit missing @param newReserveFactorMantissa
1433: * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details) //@audit missing @param newReserveFactorMantissa
1711: * @param errorMessage The revert string to return on failure. //@audit missing @param target //@audit missing @return bytes
contracts/libs/LiquidityLibrary.sol:
23: * @param _tick current price tick used for calculation //@audit missing @param _liquidity
contracts/libs/LpBreakdownLibrary.sol:
34: * @param isTwap Whether to use a TWAP or instantaneous tick for calculations //@audit missing @param * 3 addresses
86: * @param tokenId The tokenId to generate position information of //@audit missing @param * 2 addresses
123: * @param vars The in-memory struct containing all most recently updated information about our tokenId //@audit missing @param tick
contracts/libs/UniswapTwapLibrary.sol:
32: * @param _period number of seconds in the past to start calculating time-weighted average //@audit missing @param _checkPeriod
contracts/vault_and_oracles/MasterPriceOracle.sol:
33: * @param _oracles The `PriceOracle` contracts to be assigned to `underlyings`. //@audit missing @param _wethAddress
103: * @return Price in ETH of the token underlying `cToken`, scaled by `10 ** (36 - underlyingDecimals)`. //@audit missing @param cToken
contracts/vault_and_oracles/UniswapTwapOracle.sol:
68: * @param _canAdminOverwrite Controls if `admin` can overwrite existing assignments of pools to tokens. //@audit missing @param * 3
111: * @return Price in ETH of the token underlying `cToken`, scaled by `10 ** (36 - underlyingDecimals)`. //@audit missing @param cToken
Avoid floating pragmas: the versions should be locked in the solution (0.5.16 and 0.7.6)
require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking
Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:
Additionally, as the Solidity versions are < 0.8.0, the remaining gas would be consumed instead of being refunded in case of failure.
Comment says "public" instead of "external" :
File: CErc20.sol
137: /**
138: * @notice A public function to sweep accidental ERC-20 transfers to this contract. Tokens are sent to admin //@audit should say "An external function"
139: * @param token The address of the ERC-20 token to sweep
140: */
141: function sweepToken(EIP20NonStandardInterface token) external {
UniswapTwapLibrary.sol:getTimeWeightedAverageTickSafe() declares a named returns but then are uses a return statement. I suggest choosing only one for readability reasons.
Style: No need to initialize variables with default values
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern.
compound_rari_fork/Comptroller.sol:119: for (uint256 i = 0; i < len; i++) {
compound_rari_fork/Comptroller.sol:206: for (uint256 i = 0; i < len; i++) {
compound_rari_fork/Comptroller.sol:768: for (uint256 i = 0; i < userTokensLength; i++) {
compound_rari_fork/Comptroller.sol:846: for (uint256 i = 0; i < assets.length; i++) {
compound_rari_fork/Comptroller.sol:1302: for (uint256 i = 0; i < len; i++) {
compound_rari_fork/Comptroller.sol:1339: for (uint256 i = 0; i < numMarkets; i++) {
compound_rari_fork/Comptroller.sol:1362: for (uint256 i = 0; i < numMarkets; i++) {
compound_rari_fork/Comptroller.sol:1471: for (uint256 i = 0; i < pools.length; i++) {
compound_rari_fork/Comptroller.sol:1490: for (uint256 i = 0; i < pools.length; i++) {
compound_rari_fork/CToken.sol:93: uint256 startingAllowance = 0;
vault_and_oracles/MasterPriceOracle.sol:51: for (uint256 i = 0; i < underlyings.length; i++) oracles[underlyings[i]] = _oracles[i];
vault_and_oracles/MasterPriceOracle.sol:68: for (uint256 i = 0; i < underlyings.length; i++) {
vault_and_oracles/UniswapTwapOracle.sol:87: for (uint256 i = 0; i < _tokens.length; i++) {
vault_and_oracles/UniswapTwapOracle.sol:209: for (uint256 i = 0; i < tokens.length; i++) {
vault_and_oracles/UniswapTwapOracle.sol:220: isSupportedPool[referencePools[tokens[i]]] = false;
vault_and_oracles/UniV3LpVault.sol:466: uint16 referralCode = 0;
vault_and_oracles/UniV3LpVault.sol:770: for (uint256 i = 0; i < len; i++) {
QA Report
Consider resolving the TODOs before deploying.
CErc20.sol:initialize()is front-runnableapproveshould be replaced withsafeApproveapproveis subject to a known front-running attack. Consider usingsafeApproveinstead:Consider adding an
address(0)check here:The following comments are missing (see
@audittags):Avoid floating pragmas: the versions should be locked in the solution (
0.5.16and0.7.6)require()should be used for checking error conditions on inputs and return values whileassert()should be used for invariant checkingProperly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:
Additionally, as the Solidity versions are < 0.8.0, the remaining gas would be consumed instead of being refunded in case of failure.
UniswapTwapLibrary.sol:getTimeWeightedAverageTickSafe()declares a named returns but then are uses a return statement. I suggest choosing only one for readability reasons.