Omniscia Rari Capital Audit
Comptroller Manual Review Findings
Comptroller Manual Review Findings
COM-01M: Inconsistent Access Control
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Minor | Comptroller.sol:L885, L909, L940, L981, L1000, L1036, L1085, L1103, L1104, L1113, L1114, L1122, L1123, L1131, L1132 |
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:
883function _setPriceOracle(PriceOracle newOracle) public returns (uint) {884 // Check caller is admin885 if (msg.sender != admin) {886 return fail(Error.UNAUTHORIZED, FailureInfo.SET_PRICE_ORACLE_OWNER_CHECK);887 }888889 // Track the old oracle for the comptroller890 PriceOracle oldOracle = oracle;891892 // Set comptroller's oracle to newOracle893 oracle = newOracle;894895 // Emit NewPriceOracle(oldOracle, newOracle)896 emit NewPriceOracle(oldOracle, newOracle);897898 return uint(Error.NO_ERROR);899}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.
COM-02M: Inconsistent Limit Evaluation
| Type | Severity | Location |
|---|---|---|
| Mathematical Operations | Minor | Comptroller.sol:L419 |
Description:
The linked segment conducts a mulScalarTruncateAddUint execution by passing in the existing accountTokens and then simply adding the mintAmount instead of conducting the actual calculation of CToken whereby the mintAmount is divided instead.
Example:
414function mintWithinLimits(address cToken, uint exchangeRateMantissa, uint accountTokens, uint mintAmount) external returns (uint) {415 // Check max supply416 uint maxSupplyEth = fuseAdmin.maxSupplyEth();417418 if (maxSupplyEth < uint(-1)) {419 (MathError mathErr, uint newUnderlyingBalance) = mulScalarTruncateAddUInt(Exp({mantissa: exchangeRateMantissa}), accountTokens, mintAmount);420 if (mathErr != MathError.NO_ERROR) return uint(Error.MATH_ERROR);421 uint newEthBalance;422 (mathErr, newEthBalance) = mulScalarTruncate(Exp({mantissa: oracle.getUnderlyingPrice(CToken(cToken))}), newUnderlyingBalance);423 if (mathErr != MathError.NO_ERROR) return uint(Error.MATH_ERROR);424 if (newEthBalance > maxSupplyEth) return uint(Error.SUPPLY_ABOVE_MAX);425 }426}Recommendation:
We advise that the same calculations are carried out in both code segments as truncation can lead to inconsistencies across the two checks (i.e. the check passing yet the final result could have lead to the check failing).
Alleviation:
After discussing with Rari, we have concluded that the inconsistencies caused by these truncations are negligible and will not create significant security risks.
COM-03M: Inexistent Error Handling
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Minor | Comptroller.sol:L401, L422 |
Description:
The codebase of Compound contains an error-handling path of getUnderlyingPrice calls whereby the Error.PRICE_ERROR is returned if the reported price is zero.
Example:
400function borrowWithinLimits(address cToken, uint accountBorrowsNew) external returns (uint) {401 (MathError mathErr, uint borrowBalanceEth) = mulScalarTruncate(Exp({mantissa: oracle.getUnderlyingPrice(CToken(cToken))}), accountBorrowsNew);402 if (mathErr != MathError.NO_ERROR) return uint(Error.MATH_ERROR);403 if (borrowBalanceEth < fuseAdmin.minBorrowEth()) return uint(Error.BORROW_BELOW_MIN);404 return uint(Error.NO_ERROR);405}Recommendation:
We advise a similar check to be imposed in these newly introduced segments of code to ensure the code behaves as expected even in the case of misbehaviour by the oracles.
Alleviation:
Error handling code was introduced to ensure that failures of the oracles are accounted for in the newly introduced code segments.
COM-04M: Misconstrued Data Erasure
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Minor | Comptroller.sol:L231-L238 |
Description:
The linked segment of the exitMarket function is meant to remove a borrower from the allBorrowers address list, however, in doing so uses a redundant storedList2 variable and does not zero out the borrowersIndexes of the borrower (msg.sender) themselves.
Example:
230// If the user has exited all markets, remove them from the `allBorrowers` array231if (storedList.length == 0) {232 // copy last item in list to location of item to be removed, reduce length by 1233 address[] storage storedList2 = allBorrowers;234 storedList2[borrowerIndexes[msg.sender]] = storedList2[storedList2.length - 1];235 storedList2.length--;236 borrowerIndexes[storedList2[borrowerIndexes[msg.sender]]] = borrowerIndexes[msg.sender];237 borrowers[msg.sender] = false;238}Recommendation:
We advise that the local storedList2 is rendered redundant and that the borrowerIndexes of msg.sender is also zeroed out during this code block.
Alleviation:
We advise this finding to be evaluated prior to launch.
COM-05M: Unutilized Function Return
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Minor | Comptroller.sol:L1046 |
Description:
The isCToken function returns a bool that remains unused in the linked code segment.
Example:
1034function _supportMarket(CToken cToken) public returns (uint) {1035 // Check caller is admin1036 if (msg.sender != admin) {1037 return fail(Error.UNAUTHORIZED, FailureInfo.SUPPORT_MARKET_OWNER_CHECK);1038 }10391040 // Is market already listed?1041 if (markets[address(cToken)].isListed) {1042 return fail(Error.MARKET_ALREADY_LISTED, FailureInfo.SUPPORT_MARKET_EXISTS);1043 }10441045 // Sanity check to make sure its really a CToken1046 cToken.isCToken();10471048 // Check cToken.comptroller == this1049 require(address(cToken.comptroller()) == address(this), "Cannot support a market with a different Comptroller.");10501051 // Make sure market is not already listed1052 address underlying = cToken.isCEther() ? address(0) : CErc20(address(cToken)).underlying();10531054 if (address(cTokensByUnderlying[underlying]) != address(0)) {1055 return fail(Error.MARKET_ALREADY_LISTED, FailureInfo.SUPPORT_MARKET_EXISTS);1056 }10571058 // List market and emit event1059 markets[address(cToken)] = Market({isListed: true, collateralFactorMantissa: 0});1060 allMarkets.push(cToken);1061 cTokensByUnderlying[underlying] = cToken;1062 emit MarketListed(cToken);10631064 return uint(Error.NO_ERROR);1065}Recommendation:
We advise that it is correctly utilized so by adding a require check.
Alleviation:
The Rari team stated that a require check would incur a redundant gas overhead as the isCToken function is not meant to be utilized for what it returns, it is meant to be utilized as a utility function the contract should implement.