Omniscia Rari Capital Audit

CToken Code Style Findings

CToken Code Style Findings

CTN-01C: Dead Code

TypeSeverityLocation
Code StyleInformationalCToken.sol:L286-L289

Description:

The linked code segment is leftover code from Compound that meant to guarantee that the mintVerify function is never marked as pure or view, however, this is guaranteed by the newly introduced assignment of true to suppliers[minter].

Recommendation:

We advise the linked code to be completely omitted from the codebase.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

CTN-02C: Inefficient Logic Split

TypeSeverityLocation
Gas OptimizationInformationalCToken.sol:L405-L500

Description:

The linked functions were part of the same accrueInterest function of the original codebase that was utilizing memory caching by retrieving the variables from storage once and storing them.

Example:

contracts/CToken.sol
405function accrueInterest() public returns (uint) {
406    /* Remember the initial block number */
407    uint currentBlockNumber = getBlockNumber();
408
409    /* Short-circuit accumulating 0 interest */
410    if (accrualBlockNumber == currentBlockNumber) {
411        return uint(Error.NO_ERROR);
412    }
413
414    /* Read the previous values out of storage */
415    uint cashPrior = getCashPrior();
416
417    /* Calculate the current borrow interest rate */
418    uint borrowRateMantissa = interestRateModel.getBorrowRate(cashPrior, totalBorrows, totalReserves + totalFuseFees + totalAdminFees);
419    require(borrowRateMantissa <= borrowRateMaxMantissa, "borrow rate is absurdly high");
420
421    /* Calculate the number of blocks elapsed since the last accrual */
422    (MathError mathErr, uint blockDelta) = subUInt(currentBlockNumber, accrualBlockNumber);
423    require(mathErr == MathError.NO_ERROR, "could not calculate block delta");
424
425    return finishInterestAccrual(currentBlockNumber, cashPrior, borrowRateMantissa, blockDelta);
426}

Recommendation:

As the current seperate design is inefficient as it retrieves variables from storage multiple times redundantly, we advise the logical split to be evaluated and potentially merged again.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

CTN-03C: Limit Evaluation Optimization

TypeSeverityLocation
Gas OptimizationInformationalCToken.sol:L817

Description:

The borrowWithinLimits sanitization check only evaluates the lower bound of the amount and thus can be optimized by evaluating whether the vars.accountBorrows was non-zero perior to adding the new borrowAmount.

Example:

contracts/CToken.sol
806(vars.mathErr, vars.accountBorrows) = borrowBalanceStoredInternal(borrower);
807if (vars.mathErr != MathError.NO_ERROR) {
808    return failOpaque(Error.MATH_ERROR, FailureInfo.BORROW_ACCUMULATED_BALANCE_CALCULATION_FAILED, uint(vars.mathErr));
809}
810
811(vars.mathErr, vars.accountBorrowsNew) = addUInt(vars.accountBorrows, borrowAmount);
812if (vars.mathErr != MathError.NO_ERROR) {
813    return failOpaque(Error.MATH_ERROR, FailureInfo.BORROW_NEW_ACCOUNT_BORROW_BALANCE_CALCULATION_FAILED, uint(vars.mathErr));
814}
815
816// Check min borrow for this user for this asset
817allowed = comptroller.borrowWithinLimits(address(this), vars.accountBorrowsNew);
818if (allowed != 0) {
819    return failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.BORROW_COMPTROLLER_REJECTION, allowed);
820}

Recommendation:

We advise that such a ternary operator is introduced to optimize the gas cost of the function and render the external call redundant in most cases.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

CTN-04C: Misleading Comments

TypeSeverityLocation
Code StyleInformationalCToken.sol:L1262, L1306, L1350

Description:

The linked comments precede the respective if clauses of the mantissa setter functions and denote the validity of a lower-than-or-equal condition with an inexistent variable.

Example:

contracts/CToken.sol
1339function _setReserveFactorFresh(uint newReserveFactorMantissa) internal returns (uint) {
1340    // Check caller is admin
1341    if (msg.sender != admin) {
1342        return fail(Error.UNAUTHORIZED, FailureInfo.SET_RESERVE_FACTOR_ADMIN_CHECK);
1343    }
1344
1345    // Verify market's block number equals current block number
1346    if (accrualBlockNumber != getBlockNumber()) {
1347        return fail(Error.MARKET_NOT_FRESH, FailureInfo.SET_RESERVE_FACTOR_FRESH_CHECK);
1348    }
1349
1350    // Check newReserveFactor ≤ maxReserveFactor
1351    if (newReserveFactorMantissa + adminFeeMantissa + fuseFeeMantissa > reserveFactorPlusFeesMaxMantissa) {
1352        return fail(Error.BAD_INPUT, FailureInfo.SET_RESERVE_FACTOR_BOUNDS_CHECK);
1353    }
1354
1355    uint oldReserveFactorMantissa = reserveFactorMantissa;
1356    reserveFactorMantissa = newReserveFactorMantissa;
1357
1358    emit NewReserveFactor(oldReserveFactorMantissa, newReserveFactorMantissa);
1359
1360    return uint(Error.NO_ERROR);
1361}

Recommendation:

We advise them to be properly adjusted to indicate that the comparison is done between all mantissas cumulatively being lower-than-or-equal-to the reserveFactorPlusFeesMaxMantissa limit. Additionally, the comment of L1306 also uses an incorrect variable name on the left-hand side of the comparison.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

Prev

CEtherImmutable.sol (CON-C)

Next

ChainlinkPriceOracle.sol (CPO-S)

On this page