The scoped contracts use approve() method which is prone to attack vectors as described here A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0:
require(allowed[msg.sender][_spender] == 0)
The whole project has different solidity compiler ranges ^0.5.16 to ^0.8.0 referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.
Floating Pragma used in CEther.sol, CNft.sol, ERC1155Enumerable.sol ,CErc20.sol , CToken.sol , Comptroller.sol , CNftPriceOracle.sol , UniswapV2PriceOracle.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) 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.
Reference
The function requireNoError() of CEther.sol contains 2 checks on errCode == uint(Error.NO_ERROR).
After the first check it returns. After this errCode == uint(Error.NO_ERROR) will never be true, so doesn't have to be checked. Team might consider to replace require(errCode == uint(Error.NO_ERROR), string(fullMessage)); with require(false, string(fullMessage));
transfer method is used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract participates in the auction.
Reference Link -1, Reference Link -2
The project files are having Solidity compiler version is 0.5.16 which is outdated. Currently the compiler version is v0.8.13.
At Comptroller.sol#L346, in order to reach the full cap, the logic should be;
require(nextTotalBorrows <= borrowCap, "market borrow cap reached");
instead of
require(nextTotalBorrows < borrowCap, "market borrow cap reached");
The contract require statements are based on NO_ERROR is equivalent to 0 condition. However in most locations there is an explicit check for NO_ERROR and comparing with 0 allows for possible future mistakes (especially if the enums would change in ErrorReporter.sol) The team might consider to replace 0 with the appropriate version of ...NO_ERROR
References:
CToken.sol:
function transferTokens(...if (allowed != 0) {
function mintFresh(...if (allowed != 0) {
function redeemFresh(...if (allowed != 0) {
function borrowFresh(...if (allowed != 0) {
function repayBorrowFresh(...if (allowed != 0) {
function liquidateBorrowFresh(...if (allowed != 0) {
function seizeInternal(...if (allowed != 0) {
Comptroller.sol:
function exitMarket(...if (allowed != 0) {...require(oErr == 0, "exitMarket: getAccountSnapshot failed"); // semi-opaque error code
function getHypotheticalAccountLiquidityInternal(...if (oErr != 0) { // semi-opaque error code, we assume NO_ERROR == 0 is invariant between upgrades
requireNoError() of CEther.sol is used to check for errors. This is used most of the time, however it isn't used in below;
The team might consider to replace require(err == MathError.NO_ERROR); with: requireNoError(err, "getCashPrior failed");
There are expressions in CToken.sol like uint(-1) for max. values. However, the team can consider to implicitly state the infinite allowance by using type(uint256).max instead of uint(-1)
ComptrollerInterface is declared with contract keyword. However it should be declared with interface keyword.
At CNFt.sol initialize function, there is no address(0) check for _comptroller no zero bytes check for _uri,
At ERC1155Enumerable.sol, there is no zero value check for_uri in __ERC1155Enumerable_init function.
The codebase is having lack of NatSpec comments such as @notice @dev @param on many places. It's recommended to fully annote the contract by using NatSpec. Reference
There is no address(0) check at PriceOracleImplementation.sol constructor function.
There is no address(0) check at CNftPriceOracle.sol constructor function for _admin, _uniswapV2Oracle_uniswapV2Factory , _baseToken , newAdmin for changeAdmin() function.
At CNftPriceOracle.sol, addAddressMapping() function, an expensive loop used by using SSTORE's. Incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas and halting the process.
for (uint256 i = 0; i < cNfts.length; ++i) {
address underlying = cNfts[i].underlying();
require(
underlyingNftxTokenAddress[underlying] == address(0),
"CNftPriceOracle: Cannot overwrite existing address mappings."
);
underlyingNftxTokenAddress[underlying] = nftxTokens[i];
}
QA (LOW / NON-CRITICAL)
The scoped contracts use
approve()
method which is prone to attack vectors as described here A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0:require(allowed[msg.sender][_spender] == 0)
The whole project has different solidity compiler ranges ^0.5.16 to ^0.8.0 referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.
Floating Pragma used in
CEther.sol
,CNft.sol
,ERC1155Enumerable.sol
,CErc20.sol
,CToken.sol
,Comptroller.sol
,CNftPriceOracle.sol
,UniswapV2PriceOracle.sol
. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) 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. ReferenceThe function
requireNoError()
ofCEther.sol
contains 2 checks onerrCode == uint(Error.NO_ERROR)
. After the first check it returns. After thiserrCode == uint(Error.NO_ERROR)
will never be true, so doesn't have to be checked. Team might consider to replacerequire(errCode == uint(Error.NO_ERROR), string(fullMessage));
withrequire(false, string(fullMessage));
transfer
method is used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract participates in the auction. Reference Link -1, Reference Link -2The project files are having Solidity compiler version is 0.5.16 which is outdated. Currently the compiler version is v0.8.13.
At
Comptroller.sol#L346
, in order to reach the full cap, the logic should be;instead of
The contract require statements are based on NO_ERROR is equivalent to 0 condition. However in most locations there is an explicit check for NO_ERROR and comparing with 0 allows for possible future mistakes (especially if the enums would change in
ErrorReporter.sol
) The team might consider to replace 0 with the appropriate version of ...NO_ERROR References: CToken.sol: function transferTokens(...if (allowed != 0) { function mintFresh(...if (allowed != 0) { function redeemFresh(...if (allowed != 0) { function borrowFresh(...if (allowed != 0) { function repayBorrowFresh(...if (allowed != 0) { function liquidateBorrowFresh(...if (allowed != 0) { function seizeInternal(...if (allowed != 0) { Comptroller.sol: function exitMarket(...if (allowed != 0) {...require(oErr == 0, "exitMarket: getAccountSnapshot failed"); // semi-opaque error code function getHypotheticalAccountLiquidityInternal(...if (oErr != 0) { // semi-opaque error code, we assume NO_ERROR == 0 is invariant between upgradesrequireNoError()
ofCEther.sol
is used to check for errors. This is used most of the time, however it isn't used in below;The team might consider to replace
require(err == MathError.NO_ERROR);
with:requireNoError(err, "getCashPrior failed");
There are expressions in
CToken.sol
likeuint(-1)
for max. values. However, the team can consider to implicitly state the infinite allowance by usingtype(uint256).max
instead ofuint(-1)
ComptrollerInterface
is declared withcontract
keyword. However it should be declared withinterface
keyword.At
CNFt.sol
initialize function, there is no address(0) check for_comptroller
no zero bytes check for_uri
,At
ERC1155Enumerable.sol
, there is no zero value check for_uri
in__ERC1155Enumerable_init
function.The codebase is having lack of NatSpec comments such as @notice @dev @param on many places. It's recommended to fully annote the contract by using NatSpec. Reference
There is no address(0) check at
PriceOracleImplementation.sol
constructor function.There is no address(0) check at
CNftPriceOracle.sol
constructor function for_admin
,_uniswapV2Oracle
_uniswapV2Factory
,_baseToken
,newAdmin
forchangeAdmin()
function.At
CNftPriceOracle.sol
,addAddressMapping()
function, an expensive loop used by using SSTORE's. Incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas and halting the process.