Title: Use safe math for solidity version <8
Severity: Low Risk
You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.
The contract Comptroller.sol doesn't use safe math and is of solidity version < 8
The contract MasterPriceOracle.sol doesn't use safe math and is of solidity version < 8
The contract CToken.sol doesn't use safe math and is of solidity version < 8
Title: Assert instead require to validate user inputs
Severity: Low Risk
From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
Comptroller.sol : reachable assert in line 402
UniswapTwapOracle.sol : reachable assert in line 195
Comptroller.sol : reachable assert in line 1309
Comptroller.sol : reachable assert in line 213
Comptroller.sol : reachable assert in line 272
UniV3LpVault.sol : reachable assert in line 777
Title: Two arrays length mismatch
Severity: Low/Med Risk
The functions below fail to perform input validation on arrays to verify the lengths match.
A mismatch could lead to an exception or undefined behavior.
Consider making this a medium risk please.
Title: transfer return value of a general ERC20 is ignored
Severity: Low/High Risk
Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must
Check the transfer return value
Another popular possibility is to add a whiteList.
Those are the appearances (solidity file, line number, actual line):
Title: approve return value is ignored
Severity: Low/Med Risk
Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean.
Calling these functions with the correct EIP20 function signatures will always revert.
Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value.
We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens.
Title: ETH send return value is ignored while is gas limited
Severity: Low/Med Risk
The use of send() / call() to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender or feeRecipient is a smart contract. Funds can potentially be lost if;
The smart contract fails to implement the payable fallback function
The fallback function uses more than 2300 gas units
Different from .transfer(...), .send(...) and call(...) doesn't revert when it fails, and therefore its retrun value is extremely important.
The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the transaction.
A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now
This is not just a best-practice advice since the return value isn't considered!!! If you would consider it then it was just a best practice.
Title: Must approve 0 first
Severity: Low/Med Risk
Some tokens (like USDT) 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.
approve without approving 0 first UniV3LpVault.sol, 417, IERC20Detailed(params.asset).approve(msg.sender, owedBack);
approve without approving 0 first UniV3LpVault.sol, 532, IERC20Detailed(params.underlying).approve(address(params.debtCToken), params.repayAmount);
Title: Duplicates in array
Severity: Low Risk
You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.
Comptroller.addToMarketInternal pushed (cToken)
The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
Title: Missing non reentrancy modifier
Severity: Low Risk
The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer.
Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..
CToken.sol, accrueInterest is missing a reentrancy modifier
UniV3LpVault.sol, _setFlashLoan is missing a reentrancy modifier
CToken.sol, _setNameAndSymbol is missing a reentrancy modifier
CToken.sol, initialize is missing a reentrancy modifier
CToken.sol, balanceOfUnderlying is missing a reentrancy modifier
UniV3LpVault.sol, _pausePeripheryFunctions is missing a reentrancy modifier
UniV3LpVault.sol, _sweep is missing a reentrancy modifier
UniV3LpVault.sol, flashFocusCall is missing a reentrancy modifier
UniV3LpVault.sol, _setUserTokensMax is missing a reentrancy modifier
CToken.sol, _setInterestRateModel is missing a reentrancy modifier
UniV3LpVault.sol, _sweepNFT is missing a reentrancy modifier
CToken.sol, approve is missing a reentrancy modifier
UniV3LpVault.sol, _pauseDeposits is missing a reentrancy modifier
CToken.sol, _acceptAdmin is missing a reentrancy modifier
Title: Check transfer receiver is not 0 to avoid burned money
Severity: Low Risk
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
owner param should be validated to make sure the owner address is not address(0).
Otherwise if not given the right input all only owner accessible functions will be unaccessible.
CToken.sol.allowance owner
CToken.sol.balanceOf owner
CToken.sol.balanceOfUnderlying owner
Title: Never used parameters
Severity: Low Risk
Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.
Comptroller.sol: function seizeAllowedUniV3 parameter liquidator isn't used. (seizeAllowedUniV3 is external)
Comptroller.sol: function seizeAllowedUniV3 parameter borrower isn't used. (seizeAllowedUniV3 is external)
Comptroller.sol: function transferAllowed parameter dst isn't used. (transferAllowed is external)
Title: Not verified input
Severity: Low Risk
external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.
UniswapTwapOracle.sol.constructor _factory
Comptroller.sol.borrowAllowed borrower
CToken.sol.redeemBehalfInternal redeemer
Comptroller.sol.repayBorrowAllowed borrower
Title: Open TODOs
Severity: Low Risk
Open TODOs can hint at programming or architectural errors that still need to be fixed.
These files has open TODOs:
Open TODO in UniV3LpVault.sol line 165 : // TODO: do we want a "decreaseLiquidityAndCollect" function?
Open TODO in UniswapTwapOracle.sol line 52 : // period used to calculate our TWAPs, in TODO units
Open TODO in UniV3LpVault.sol line 141 : // TODO: do we want some Comptroller like error handling/messaging here?
Open TODO in Comptroller.sol line 1011 : // TODO: custom liquidation incentive for LP shares
Title: Named return issue
Severity: Low Risk
Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.
Title: Use safe math for solidity version <8 Severity: Low Risk
You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity. The contract Comptroller.sol doesn't use safe math and is of solidity version < 8 The contract MasterPriceOracle.sol doesn't use safe math and is of solidity version < 8 The contract CToken.sol doesn't use safe math and is of solidity version < 8
Title: Assert instead require to validate user inputs Severity: Low Risk
Title: Two arrays length mismatch Severity: Low/Med Risk
The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior. Consider making this a medium risk please.
Title: transfer return value of a general ERC20 is ignored Severity: Low/High Risk
Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must
Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):
Title: approve return value is ignored Severity: Low/Med Risk
Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens.
Title: ETH send return value is ignored while is gas limited Severity: Low/Med Risk
The use of send() / call() to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender or feeRecipient is a smart contract. Funds can potentially be lost if;
The fallback function uses more than 2300 gas units Different from .transfer(...), .send(...) and call(...) doesn't revert when it fails, and therefore its retrun value is extremely important. The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the transaction. A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now This is not just a best-practice advice since the return value isn't considered!!! If you would consider it then it was just a best practice.
CToken.sol, 1626, address(newInterestRateModel).call(abi.encodeWithSignature("checkpointInterest()")); CToken.sol, 1623, address(oldInterestRateModel).call(abi.encodeWithSignature("resetInterestCheckpoints()")); CToken.sol, 473, address(interestRateModel).call(abi.encodeWithSignature("checkpointInterest(uint256)", borrowRateMantissa));
Title: Must approve 0 first Severity: Low/Med Risk
Some tokens (like USDT) 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.
Title: Duplicates in array Severity: Low Risk
Title: Solidity compiler versions mismatch Severity: Low Risk
The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
Title: Missing non reentrancy modifier Severity: Low Risk
The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..
Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
Title: Does not validate the input fee parameter Severity: Low Risk
Some fee parameters of functions are not checked for invalid values. Validate the parameters:
Title: Not verified owner Severity: Low Risk
Title: Never used parameters Severity: Low Risk
Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.
Title: Not verified input Severity: Low Risk
Title: Open TODOs Severity: Low Risk
Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:
Open TODO in UniV3LpVault.sol line 165 : // TODO: do we want a "decreaseLiquidityAndCollect" function?
Open TODO in UniswapTwapOracle.sol line 52 : // period used to calculate our TWAPs, in TODO units
Open TODO in UniV3LpVault.sol line 141 : // TODO: do we want some Comptroller like error handling/messaging here?
Open TODO in Comptroller.sol line 1011 : // TODO: custom liquidation incentive for LP shares
Title: Named return issue Severity: Low Risk
Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.