Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

A serious issue was caused by Compound’s centralized oracle solution which pulls market data from only a single exchange, Coinbase, with Uniswap TWAP used as a backstop. 

Using Uniswap TWAP as a backstop is better than no backstop in this situation, but it introduces a false sense of security as it too can trivially be manipulated (as we saw during this event). 

Price oracle feed

In the contracts `CToken/Unitroller/CErc20Delegator/GovernorBravoDelegator/CDaiDelegate`, the role `admin` has the authority over the following function:

- `_setComptroller()`: change the implementation of `Comptroller` with any contracts,
- `_setPendingImplementation()/_acceptImplementation()`: change the implementation of `Unitroller` with any contracts,
- `_setImplementation()`: change the implementation of `CErc20` with any contracts,
- `_setImplementation()`: change the implementation of `GovernorBravo` with any contracts,
- `_resignImplementation()`: change the implementation of the `CDai` with any contracts,

Any compromise to the `admin` account may allow the hacker to take advantage of this and users' assets may suffer loss.

Centralization Related Risks

The CToken contract is serving as the underlying entity to interact with third-party underlying asset protocols. The scope of the audit treats 3rd party entities as black boxes and assumes their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, migrating to new LP pools, etc.

Third Party Dependencies in the contract `CToken`

In the contract `BaseJumpRateModelV2` the role `owner` has authority over the functions shown in the diagram below.

Any compromise to the `owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-BaseJumpRateModelV2-owner.svg)

All of the `Comp` tokens are sent to the given address `account` when deploying the contract. This could be a centralization risk as the deployer can distribute 10 million tokens without obtaining the consensus of the community.

Initial Token Distribution

In the history of the compound protocol, the calculation of the Comp rewards becomes incorrect after proposal 062（`Split COMP rewards distribution and bug fixes`） is executed. 

In response to this problem, the function `fixBadAccruals()` has been temporarily added in proposal 065(`Correct Over-Accrued COMP`) and removed later.

The function `fixBadAccruals()` is a centralized function. It will update the users' unclaimed Comp rewards to decrease the incorrect rewards, and record the COMP debt in the variable `compReceivable` for the users whose unclaimed Comp rewards are not enough to decrease.

The function `fixBadAccruals()` is only used to handle the error caused by this upgrade migration in the Compound protocol, which is useless in the newly deployed Compound forks.

Refer to: 

- https://compound.finance/governance/proposals/62
- https://compound.finance/governance/proposals/65


Centralization Risks in the function `fixBadAccruals()`

In the contract `Comptroller` the role `admin` has authority over the functions shown in the diagram below.

Any compromise to the `admin` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Comptroller-admin.svg)

In the contract `Comptroller` the role `borrowCapGuardian` has authority over the functions shown in the diagram below.

Any compromise to the `borrowCapGuardian` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Comptroller-borrowCapGuardian.svg)

In the contract `Comptroller` the role `pauseGuardian` has authority over the functions shown in the diagram below.

Any compromise to the `pauseGuardian` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Comptroller-pauseGuardian.svg)

According to the codes in the function `distributeSupplierComp()`, the function is used to calculate the amount of Comp that needs to distribute to the supplier. The amount is calculated by the `deltaIndex`, which is calculated by the block-related parameters `supplyIndex(compSupplyState[cToken].index)` and `supplierIndex`. `supplierIndex` may be the value of `compInitialIndex`.

```solidity=1236
    function distributeSupplierComp(address cToken, address supplier) internal {
        // TODO: Don't distribute supplier COMP if the user is not in the supplier market.
        // This check should be as gas efficient as possible as distributeSupplierComp is called in many places.
        // - We really don't want to call an external contract as that's quite expensive.

        CompMarketState storage supplyState = compSupplyState[cToken];
        uint supplyIndex = supplyState.index;
        uint supplierIndex = compSupplierIndex[cToken][supplier];

        // Update supplier's index to the current index since we are distributing accrued COMP
        compSupplierIndex[cToken][supplier] = supplyIndex;

        if (supplierIndex == 0 && supplyIndex >= compInitialIndex) {
            // Covers the case where users supplied tokens before the market's supply state index was set.
            // Rewards the user with COMP accrued from the start of when supplier rewards were first
            // set for the market.
            supplierIndex = compInitialIndex;
        }

        // Calculate change in the cumulative sum of the COMP per cToken accrued
        Double memory deltaIndex = Double({mantissa: sub_(supplyIndex, supplierIndex)});

        uint supplierTokens = CToken(cToken).balanceOf(supplier);

        // Calculate COMP accrued: cTokenAmount * accruedPerCToken
        uint supplierDelta = mul_(supplierTokens, deltaIndex);

        uint supplierAccrued = add_(compAccrued[supplier], supplierDelta);
        compAccrued[supplier] = supplierAccrued;

        emit DistributedSupplierComp(CToken(cToken), supplier, supplierDelta, supplyIndex);
    }
```

According to the codes in the function `updateCompSupplyIndex()`,  `compSupplyState[cToken].index` is calculated by the block and the `supplySpeed`, which may be smaller the value of `compInitialIndex` in case `compSupplyState[cToken]` is initialized incorrectly.

```solidity=1194
    function updateCompSupplyIndex(address cToken) internal {
        CompMarketState storage supplyState = compSupplyState[cToken];
        uint supplySpeed = compSupplySpeeds[cToken];
        uint32 blockNumber = safe32(getBlockNumber(), "block number exceeds 32 bits");
        uint deltaBlocks = sub_(uint(blockNumber), uint(supplyState.block));
        if (deltaBlocks > 0 && supplySpeed > 0) {
            uint supplyTokens = CToken(cToken).totalSupply();
            uint compAccrued = mul_(deltaBlocks, supplySpeed);
            Double memory ratio = supplyTokens > 0 ? fraction(compAccrued, supplyTokens) : Double({mantissa: 0});
            supplyState.index = safe224(add_(Double({mantissa: supplyState.index}), ratio).mantissa, "new index exceeds 224 bits");
            supplyState.block = blockNumber;
        } else if (deltaBlocks > 0) {
            supplyState.block = blockNumber;
        }
    }
```

As a result, the function `distributeSupplierComp()` called in the functions `mintAllowed()/redeemAllowed()/seizeAllowed()/transferAllowed()` will fail as subtraction overflow may be caused when calculating `deltaIndex`.

Potential `mint/redeem/seize/transfer` failure possible

The return value of an external call is not stored in a local or state variable.

Examples: 

```solidity
function _supportMarket(CToken cToken) external returns (uint) {
  ...
  cToken.isCToken(); // Sanity check to make sure its really a CToken
  ...
}
```


Return value not stored

In the aforementioned line, the formula for the calculation of `exchangeRate` is as follows after cToken is minted:

$$
exchangeRate =\\
 {totalCash + totalBorrows - totalReserves \over totalSupply }
$$


```solidity=339
    function exchangeRateStoredInternal() internal view returns (MathError, uint) {
        uint _totalSupply = totalSupply;
        if (_totalSupply == 0) {
            /*
             * If there are no tokens minted:
             *  exchangeRate = initialExchangeRate
             */
            return (MathError.NO_ERROR, initialExchangeRateMantissa);
        } else {
            /*
             * Otherwise:
             *  exchangeRate = (totalCash + totalBorrows - totalReserves) / totalSupply
             */
            uint totalCash = getCashPrior();
            uint cashPlusBorrowsMinusReserves;
            Exp memory exchangeRate;
            MathError mathErr;

            (mathErr, cashPlusBorrowsMinusReserves) = addThenSubUInt(totalCash, totalBorrows, totalReserves);
            if (mathErr != MathError.NO_ERROR) {
                return (mathErr, 0);
            }

            (mathErr, exchangeRate) = getExp(cashPlusBorrowsMinusReserves, _totalSupply);
            if (mathErr != MathError.NO_ERROR) {
                return (mathErr, 0);
            }

            return (MathError.NO_ERROR, exchangeRate.mantissa);
        }
    }
```

In solidity, division calculations have truncation problems. The `totalSupply` will be 1 and `exchangeRate` will be much smaller than `initialExchangeRate` in case the last user redeems (`accountTokens[redeemer] - 1`) cToken.

As a result, the `exchangeRate` would be extremely small.

When the value of `exchangeRate` is much smaller than `initialExchangeRate`, the user can mint cTokens well above normal values, and then the value of `exchangeRate` will be normal with the interest generating. In other words, the users can use this arbitrage to take away the underlying tokens in this pool.

For example, the user can mint the amount of 1e8 CToken with one underlying token in case `exchangeRate = 1/1e8`.

Logical issue of function `exchangeRateStoredInternal()`

The following codes in the function `redeemFresh()/borrowFresh()` do not meet the Checks-Effects-Interactions pattern.

```solidity=694
        doTransferOut(redeemer, vars.redeemAmount);

        /* We write previously calculated values into storage */
        totalSupply = vars.totalSupplyNew;
        accountTokens[redeemer] = vars.accountTokensNew;
```
```solidity=786
        doTransferOut(borrower, borrowAmount);

        /* We write the previously calculated values into storage */
        accountBorrows[borrower].principal = vars.accountBorrowsNew;
        accountBorrows[borrower].interestIndex = borrowIndex;
        totalBorrows = vars.totalBorrowsNew;
```

It only has a reentrancy lock as there is no lock at the controller level, only the CToken level.

If the `rToken` is an ERC777 protocol, the reentrancy can happen in function levels of an ERC777 based contract, i.e. multiple function calls that are triggered by the hook mechanism of ERC777.

This issue is possible to happen with all compound forks, but Compound is not affected as they do not list tokens with callback functionality.

Checks-Effects-Interactions pattern violations

In the contract `DAIInterestRateModelV3` the role `owner` has authority over the functions shown in the diagram below.

Any compromise to the `owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-DAIInterestRateModelV3-owner.svg)

Addresses should be checked before assignment or external call to make sure they are not zero addresses.

File: contracts/CErc20.sol (Line 36, Function `CErc20.initialize`)
- `underlying_` is not zero-checked before being used.


---
File: contracts/CErc20Delegator.sol (Line 51, Function `CErc20Delegator.constructor`)
- `admin_` is not zero-checked before being used.


---
File: contracts/CErc20Delegator.sol (Line 68, Function `CErc20Delegator._setImplementation`)
- `implementation_` is not zero-checked before being used.


---
File: contracts/CErc20Immutable.sol (Line 37, Function `CErc20Immutable.constructor`)
- `admin_` is not zero-checked before being used.


---
File: contracts/CEther.sol (Line 34, Function `CEther.constructor`)
- `admin_` is not zero-checked before being used.


---
File: contracts/CToken.sol (Line 1142, Function `CToken._setPendingAdmin`)
- `newPendingAdmin` is not zero-checked before being used.


---
File: contracts/Comptroller.sol (Line 1027, Function `Comptroller._setBorrowCapGuardian`)
- `newBorrowCapGuardian` is not zero-checked before being used.


---
File: contracts/Comptroller.sol (Line 1047, Function `Comptroller._setPauseGuardian`)
- `newPauseGuardian` is not zero-checked before being used.


---
File: contracts/Comptroller.sol (Line 1452, Function `Comptroller._setCompAddress`)
- `newCompAddress` is not zero-checked before being used.

---
File: contracts/Governance/GovernorAlpha.sol (Line 133, Function `GovernorAlpha.constructor`)
- `guardian_` is not zero-checked before being used.


---
File: contracts/Governance/GovernorBravoDelegate.sol (Line 357, Function `GovernorBravoDelegate._setWhitelistGuardian`)
- `account` is not zero-checked before being used.


---
File: contracts/Governance/GovernorBravoDelegate.sol (Line 388, Function `GovernorBravoDelegate._setPendingAdmin`)
- `newPendingAdmin` is not zero-checked before being used.


---
File: contracts/Governance/GovernorBravoDelegateG1.sol (Line 344, Function `GovernorBravoDelegate._setPendingAdmin`)
- `newPendingAdmin` is not zero-checked before being used.


---
File: contracts/Governance/GovernorBravoDelegator.sol (Line 28, Function `GovernorBravoDelegator.constructor`)
- `admin_` is not zero-checked before being used.


---
File: contracts/Reservoir.sol (Line 36, Function `Reservoir.constructor`)
- `target_` is not zero-checked before being used.


---
File: contracts/Timelock.sol (Line 30, Function `Timelock.constructor`)
- `admin_` is not zero-checked before being used.


---
File: contracts/Timelock.sol (Line 55, Function `Timelock.setPendingAdmin`)
- `pendingAdmin_` is not zero-checked before being used.


---
File: contracts/Timelock.sol (Line 99, Function `Timelock.executeTransaction`)
- `target` is not zero-checked before being used.


---
File: contracts/USDT.sol (Line 389, Function `USDT.deprecate`)
- `_upgradedAddress` is not zero-checked before being used.


---
File: contracts/Uni.sol (Line 281, Function `Uni.constructor`)
- `minter_` is not zero-checked before being used.


---
File: contracts/Uni.sol (Line 293, Function `Uni.setMinter`)
- `minter_` is not zero-checked before being used.


---
File: contracts/Unitroller.sol (Line 46, Function `Unitroller._setPendingImplementation`)
- `newPendingImplementation` is not zero-checked before being used.


---
File: contracts/Unitroller.sol (Line 95, Function `Unitroller._setPendingAdmin`)
- `newPendingAdmin` is not zero-checked before being used.



Missing Zero Address Validation

Boolean constants can be used directly and do not need to be compared to true or false.

File: contracts/Comptroller.sol (Line 1058, Function `Comptroller._setMintPaused`)
```solidity
        require(msg.sender == admin || state == true, "only admin can unpause");
```


---
File: contracts/Comptroller.sol (Line 1068, Function `Comptroller._setBorrowPaused`)
```solidity
        require(msg.sender == admin || state == true, "only admin can unpause");
```


---
File: contracts/Comptroller.sol (Line 1077, Function `Comptroller._setTransferPaused`)
```solidity
        require(msg.sender == admin || state == true, "only admin can unpause");
```


---
File: contracts/Comptroller.sol (Line 1086, Function `Comptroller._setSeizePaused`)
```solidity
        require(msg.sender == admin || state == true, "only admin can unpause");
```


---
File: contracts/Comptroller.sol (Line 1355, Function `Comptroller.claimComp`)
```solidity
            if (borrowers == true) {
```


---
File: contracts/Comptroller.sol (Line 1362, Function `Comptroller.claimComp`)
```solidity
            if (suppliers == true) {
```


---
File: contracts/Comptroller.sol (Line 1387, Function `Comptroller.grantCompInternal`)
```solidity
        if (amount > 0 && amount <= compRemaining && compClaimPaused == false && compGainFactorMantissa > 0) {
```


---
File: contracts/Comptroller.sol (Line 1495-1498, Function `Comptroller.isDeprecated`)
```solidity
        return
            markets[address(cToken)].collateralFactorMantissa == 0 &&
            borrowGuardianPaused[address(cToken)] == true &&
            cToken.reserveFactorMantissa() == 1e18
```


---
File: contracts/Comptroller.sol (Line 157, Function `Comptroller.addToMarketInternal`)
```solidity
        if (marketToJoin.accountMembership[borrower] == true) {
```


---
File: contracts/Governance/GovernorAlpha.sol (Line 265, Function `GovernorAlpha._castVote`)
```solidity
        require(receipt.hasVoted == false, "GovernorAlpha::_castVote: voter already voted");
```


---
File: contracts/Governance/GovernorBravoDelegate.sol (Line 271, Function `GovernorBravoDelegate.castVoteInternal`)
```solidity
        require(receipt.hasVoted == false, "GovernorBravo::castVoteInternal: voter already voted");
```


---
File: contracts/Governance/GovernorBravoDelegateG1.sol (Line 260, Function `GovernorBravoDelegate.castVoteInternal`)
```solidity
        require(receipt.hasVoted == false, "GovernorBravo::castVoteInternal: voter already voted");
```



Comparison to Boolean Constant

One or more declarations do not conform to the [Solidity style guide](https://solidity.readthedocs.io/en/v0.7.3/style-guide.html#naming-conventions) with regards to its naming convention.

Particularly:
- `camelCase`: Should be applied to function names, argument names, local and state variable names, modifiers
- `UPPER_CASE`: Should be applied to `constant` variables
- `CapWords`: Should be applied to contract names, struct names, event names and enums

Examples:
 
Constants are not in `UPPER_CASE`: 

- contract `BaseJumpRateModelV2`: `blocksPerYear`
- contract `CTokenInterfaces`: `protocolSeizeShareMantissa`, `isCToken`, `borrowRateMaxMantissa`, `reserveFactorMaxMantissa`
- contract `Comptroller`: `compInitialIndex`, `closeFactorMinMantissa`, `closeFactorMaxMantissa`, `collateralFactorMaxMantissa`, `isComptroller`
- contract `DAIInterestRateModelV3`: `assumedOneMinusReserveFactorMantissa`
- contract `Dai`: `version`
- contract `ExponentialNoError`: `expScale`, `doubleScale`, `halfExpScale`, `mantissaOne`
- contract `Comp`: `totalSupply`
- contract `GovernorBravoDelegate`: `quorumVotes`, `proposalMaxOperations`
- contract `GovernorBravoDelegateG1`: `quorumVotes`, `proposalMaxOperations`
- contract `InterestRateModel`: `isInterestRateModel`
- contract `JumpRateModel`: `blocksPerYear`
- contract `LegacyInterestRateModel`: `isInterestRateModel`
- contract `PriceOracle`: `isPriceOracle`
- contract `Uni`: `minimumTimeBetweenMints`, `mintCap`
- contract `WhitePaperInterestRateModel`: `blocksPerYear`

Variables are not in `camelCase`: 

- contract `Dai`: `DOMAIN_SEPARATOR`

Functions are not in `camelCase`

- contract `ExponentialNoError`: `mul_ScalarTruncate()`, `mul_ScalarTruncateAddUInt()`
- contract `GovernorAlpha`: `GRACE_PERIOD()`
- contract `GovernorBravoInterfaces`: `GRACE_PERIOD()`

Declaration Naming Convention

Boolean constants in code have only a few legitimate uses. Other uses (in complex expressions, as conditionals) indicate either an error or, most likely, the persistence of faulty code.

File: contracts/CErc20Delegate.sol (Line 25, Function `CErc20Delegate._becomeImplementation`)

File: contracts/CErc20Delegate.sol (Line 37, Function `CErc20Delegate._resignImplementation`)

File: contracts/Comptroller.sol (Line 413, Function `Comptroller.borrowVerify`)

File: contracts/Comptroller.sol (Line 544, Function `Comptroller.liquidateBorrowVerify`)

File: contracts/Comptroller.sol (Line 277, Function `Comptroller.mintVerify`)

File: contracts/Comptroller.sol (Line 469, Function `Comptroller.repayBorrowVerify`)

File: contracts/Comptroller.sol (Line 607, Function `Comptroller.seizeVerify`)

File: contracts/Comptroller.sol (Line 654, Function `Comptroller.transferVerify`)

Misuse of Boolean Constant

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to different compiler versions. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

Improper Usage of `public` and `external` Type

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

In the contract `Uni` the role `minter` has authority over the functions shown in the diagram below.

Any compromise to the `minter` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Uni-minter.svg)

In the contract `BlackList` the role `_owner` has authority over the functions shown in the diagram below.

Any compromise to the `_owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-BlackList-_owner.svg)

In the contract `Ownable` the role `owner` has authority over the functions shown in the diagram below.

Any compromise to the `owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Ownable-owner.svg)

In the contract `Pausable` the role `_owner` has authority over the functions shown in the diagram below.

Any compromise to the `_owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-Pausable-_owner.svg)

In the contract `USDT` the role `_owner` has authority over the functions shown in the diagram below.

Any compromise to the `_owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-USDT-_owner.svg)

The linked variables could be declared as `constant` since these state variables are never modified.

Variables That Could Be Declared as `constant`

In the contract `WitnetPriceOracle` the role `owner` has authority over the functions shown in the diagram below.

Any compromise to the `owner` account may allow the hacker to take advantage of this authority.

![](https://accelerator-tasks-prod.s3.amazonaws.com/d18fd69e3c064e3e831441707d5a507f/diagrams/centralization-WitnetPriceOracle-owner.svg)

The contract is serving as the underlying entity to interact with third-party Witnet protocol. The scope of the audit treats 3rd party entities as black boxes and assumes their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts.


0xLend

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

24

0

17

0

7