Omniscia Rari Capital Audit

CToken Manual Review Findings

CToken Manual Review Findings

CTN-01M: Bypassable Limit Evaluation

TypeSeverityLocation
Logical FaultMinorCToken.sol:L817, L965

Description:

The borrowWithinLimits is meant to guarantee a lower-bound for the amount borrowed, however, a user is able to borrow at the designated amount and then immediately repay a part of the debt to essentially acquire a debt lower than the bound without the transactions reverting.

Example:

contracts/CToken.sol
958(vars.mathErr, vars.accountBorrowsNew) = subUInt(vars.accountBorrows, vars.actualRepayAmount);
959require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_ACCOUNT_BORROW_BALANCE_CALCULATION_FAILED");
960
961(vars.mathErr, vars.totalBorrowsNew) = subUInt(totalBorrows, vars.actualRepayAmount);
962require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_TOTAL_BALANCE_CALCULATION_FAILED");
963
964/* We write the previously calculated values into storage */
965accountBorrows[borrower].principal = vars.accountBorrowsNew;
966accountBorrows[borrower].interestIndex = borrowIndex;
967totalBorrows = vars.totalBorrowsNew;

Recommendation:

We advise that proper sanitization is added in the borrow repay function as well that ensures that the repay either totally wipes the debt off or repays up to the lower bound limit.

Alleviation:

The Rari team has stated that they are currently undecided as to whether they intend this to be expected functionality of the system or whether it should be prevented and as a result the codebase has remained unaltered in the latest iteration.

CTN-02M: Inconsistent Access Control

Description:

The linked require check and if clauses seem to be unadjusted code from the original Compound codebase and do not conform to the new access-control paradigm of utilizing the hasAdminRights() utility function that contains additional checks.

Example:

contracts/CToken.sol
26function initialize(ComptrollerInterface comptroller_,
27                    InterestRateModel interestRateModel_,
28                    uint initialExchangeRateMantissa_,
29                    string memory name_,
30                    string memory symbol_,
31                    uint8 decimals_,
32                    uint256 reserveFactorMantissa_,
33                    uint256 adminFeeMantissa_) public {
34    require(msg.sender == admin, "only admin may initialize the market");
35    require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once");
36
37    // Set initial exchange rate
38    initialExchangeRateMantissa = initialExchangeRateMantissa_;
39    require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");
40
41    // Set the comptroller
42    uint err = _setComptroller(comptroller_);
43    require(err == uint(Error.NO_ERROR), "setting comptroller failed");
44
45    // Initialize block number and borrow index (block number mocks depend on comptroller being set)
46    accrualBlockNumber = getBlockNumber();
47    borrowIndex = mantissaOne;
48
49    // Set the interest rate model (depends on block number / borrow index)
50    err = _setInterestRateModelFresh(interestRateModel_);
51    require(err == uint(Error.NO_ERROR), "setting interest rate model failed");
52
53    name = name_;
54    symbol = symbol_;
55    decimals = decimals_;
56
57    // Set reserve factor
58    err = _setReserveFactorFresh(reserveFactorMantissa_);
59    require(err == uint(Error.NO_ERROR), "setting reserve factor failed");
60
61    // Set admin fee
62    err = _setAdminFeeFresh(adminFeeMantissa_);
63    require(err == uint(Error.NO_ERROR), "setting admin fee failed");
64
65    // Set Fuse fee
66    err = _setFuseFeeFresh(getPendingFuseFeeFromAdmin());
67    require(err == uint(Error.NO_ERROR), "setting Fuse fee failed");
68
69    // The counter starts true to prevent changing it from zero to non-zero (i.e. smaller cost/refund)
70    _notEntered = true;
71}

Recommendation:

We advise them to be replaced by a proper invocation of the hasAdminRights() inherited function.

Alleviation:

The Rari team responded by stating that they slimmed down the codebase prior to the audit and reverted certain changes they had made to the admin-specific access control imposed on certain functions. In the latest commit, they have restored all references to hasAdminRights() ensuring proper enforcement of the new access control system.

CTN-03M: Incorrect Limit Evaluation

TypeSeverityLocation
Logical FaultMinorCToken.sol:L555, L572

Description:

The mintWithinLimits function is invoked by passing in the input mintAmount, however, the actual mint amount may differ as denoted by the comments surrounding the vars.actualMintAmount assignment.

Example:

contracts/CToken.sol
554// Check max supply
555allowed = comptroller.mintWithinLimits(address(this), vars.exchangeRateMantissa, accountTokens[minter], mintAmount);
556if (allowed != 0) {
557    return (failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.MINT_COMPTROLLER_REJECTION, allowed), 0);
558}
559
560/////////////////////////
561// EFFECTS & INTERACTIONS
562// (No safe failures beyond this point)
563
564/*
565 *  We call `doTransferIn` for the minter and the mintAmount.
566 *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
567 *  `doTransferIn` reverts if anything goes wrong, since we can't be sure if
568 *  side-effects occurred. The function returns the amount actually transferred,
569 *  in case of a fee. On success, the cToken holds an additional `actualMintAmount`
570 *  of cash.
571 */
572vars.actualMintAmount = doTransferIn(minter, mintAmount);

Recommendation:

We advise the limit to be evaluated with the actual mint amount rather than the estimated mint amount, perhaps via a require evaluation to ensure tokens are returned to the sender.

Alleviation:

The Rari team has stated that these minor inconsistences due to f.e. token fees should have negligible impact to the overall system and thus can be ignored.

CTN-04M: Inexistent Access Control

TypeSeverityLocation
Logical FaultMinorCToken.sol:L1494, L1601

Description:

The _withdrawFuseFees and _withdrawAdminFees functions do not contain any access control with regards to the caller of the function permitting them to be arbitrarily called even against the will of the recipients of the fees.

Example:

contracts/CToken.sol
1551function _withdrawAdminFees(uint withdrawAmount) external nonReentrant returns (uint) {
1552    uint error = accrueInterest();
1553    if (error != uint(Error.NO_ERROR)) {
1554        // accrueInterest emits logs on errors, but on top of that we want to log the fact that an attempted admin fee withdrawal failed.
1555        return fail(Error(error), FailureInfo.WITHDRAW_ADMIN_FEES_ACCRUE_INTEREST_FAILED);
1556    }
1557    // _withdrawAdminFeesFresh emits reserve-reduction-specific logs on errors, so we don't need to.
1558    return _withdrawAdminFeesFresh(withdrawAmount);
1559}

Recommendation:

We advise that proper access control is introduced here as, for example, the admin may have lost his private key, the fuse address is waiting a proxy upgrade and more that can lead to fees accumulated being permanently lost.

Alleviation:

The Rari team has responded by stating that they wish these functions to be easily invoke-able in contrast to admin related functions to ensure that a powerful key is not pulled from cold storage every time they desire to withdraw fees.

CTN-05M: Literal enum Comparison

Description:

The linked segments evaluate the result of a comptroller call that returns the Error enum casted to a uint and return an error if the returned casted enum is not equal to 0.

Example:

contracts/CToken.sol
536/* Fail if mint not allowed */
537uint allowed = comptroller.mintAllowed(address(this), minter, mintAmount);
538if (allowed != 0) {
539    return (failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.MINT_COMPTROLLER_REJECTION, allowed), 0);
540}
541
542/* Verify market's block number equals current block number */
543if (accrualBlockNumber != getBlockNumber()) {
544    return (fail(Error.MARKET_NOT_FRESH, FailureInfo.MINT_FRESHNESS_CHECK), 0);
545}
546
547MintLocalVars memory vars;
548
549(vars.mathErr, vars.exchangeRateMantissa) = exchangeRateStoredInternal();
550if (vars.mathErr != MathError.NO_ERROR) {
551    return (failOpaque(Error.MATH_ERROR, FailureInfo.MINT_EXCHANGE_RATE_READ_FAILED, uint(vars.mathErr)), 0);
552}
553
554// Check max supply
555allowed = comptroller.mintWithinLimits(address(this), vars.exchangeRateMantissa, accountTokens[minter], mintAmount);
556if (allowed != 0) {
557    return (failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.MINT_COMPTROLLER_REJECTION, allowed), 0);
558}

Recommendation:

This comparison can be dangerous as it relies on the order of declaration within the enum to remain the same which is not a good programming paradigm. As the Error enum is accessible here, we advise these changes to be altered to instead conduct an actual cast of Error.NO_ERROR to a uint on the 0 part of the comparison to ensure the code blocks will function as expected regardless of how the code is altered.

Alleviation:

The Rari team has stated that they expect Error.NO_ERROR to remain unchanged even in future iterations of the codebase and as such, the codebase remained as is in the latest iteration.

Prev

CEtherDelegator.sol (CON-M)

Next

CTokenInterfaces.sol (CTI-M)

On this page