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

In the contracts `CToken/Unitroller/CErc20Delegator`, 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,

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

Centralization related risks

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

The given input is missing the check for the non-zero address.

For example,
- contract `Comptroller`: `newPauseGuardian` in function `_setPauseGuardian()`,
- contract `CToken`: `newPendingAdmin` in function `_setPendingAdmin()`,
- contract `Unitroller`: `newPendingImplementation` in function `_setPendingImplementation()`, `newPendingAdmin` in function `_setPendingAdmin()`

Missing input validation

The compiler version utilized throughout the project uses the "^" prefix specifier, denoting that a compiler version which is greater than the version will be used to compile the contracts. 

Unlocked compiler version declaration

“public” functions that are never called by the contract should be declared “external”. When the inputs are arrays, “external” functions are more efficient than “public” functions.

Examples: 

Functions like : 

- contract `Comptroller`: `enterMarkets()`, `getAccountLiquidity()`, `getHypotheticalAccountLiquidity()`, `_setPriceOracle()`, `_setPauseGuardian()`, `_setMintPaused()`, `_setBorrowPaused()`, `_setTransferPaused()`, `_setSeizePaused()`, `_become()`, `claimComp()`, `getAllMarkets()`, 
- contract `CToken`: `initialize()`, `_setInterestRateModel()`,
- contract `Comp`: `delegate()`, `delegateBySig()`, `getPriorVotes()`
- contract `WJST`: `deposit()`, `withdraw()`, `getPriorVotes()`, `voteFresh()`, `withdrawVotes()`, `setGovernorAlpha()`, `transferOwnership()`,
- contract `Unitroller`: `_setPendingImplementation()`, `_setPendingAdmin()`, `_acceptAdmin()`,
- contract `CErc20`: `initialize()`,
- contract `CCErc20Delegate`: `_becomeImplementation()`, `_resignImplementation()`,
- contract `CErc20Delegator`: `borrowBalanceStored()`, `exchangeRateCurrent()`, `exchangeRateStored()`, `accrueInterest()`, `_setComptroller()`, `_setInterestRateModel()`,
- contract `GovernorAlpha`, `propose()`.

Proper usage of “public” and “external” type

Solidity defines a naming convention that should be followed. In general, the following naming conventions should be utilized in a Solidity file:

Constants should be named with all capital letters with underscores separating words UPPER_CASE_WITH_UNDERSCORES

refer to https://solidity.readthedocs.io/en/v0.5.17/style-guide.html#naming-conventions

Examples:
 
Constants like : 

- contract `CTokenStorage`: `borrowRateMaxMantissa`, `reserveFactorMaxMantissa`,
- contract `CTokenInterface`: `isCToken`,
- contract `Comptroller`: `compClaimThreshold`,  `compInitialIndex`, `closeFactorMinMantissa`, `closeFactorMaxMantissa`, `collateralFactorMaxMantissa`, `liquidationIncentiveMinMantissa`, `liquidationIncentiveMaxMantissa`,
- contract `ComptrollerInterface`: `isComptroller`,
- contract `Exponential` `expScale`, `doubleScale`, `halfExpScale`, `mantissaOne`,
- contract `InterestRateModel`: `isInterestRateModel`,
- contract `PriceOracle`: `isPriceOracle`,

 Incorrect naming convention utilization 

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.

For example: 

```solidity
        if (false) {
            maxAssets = maxAssets;
        }
```

Misuse of a boolean constant

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();
  ...
}
```


Return value not stored

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

For example:

```solidity
        if (marketToJoin.accountMembership[borrower] == true) {
            // already joined
            return Error.NO_ERROR;
        }
```

Boolean equality

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

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

        /* We write previously calculated values into storage */
        totalSupply = vars.totalSupplyNew;
        accountTokens[redeemer] = vars.accountTokensNew;
```

```solidity=800
        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 `cToken` 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 aforementioned line, the formula for the calculation of `exchangeRate` is as follows after cToken is minted:

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


```solidity=344
    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()`

In the contract `GovernorAlpha` the role `guardian` has authority over the functions shown in the diagram below.

- `cancel()`, to cancel the proposal.
- `__acceptAdmin()`, to accept admin of the `Timelock` contract.
- `__abdicate()`, to renounce `guardian`.
- `__queueSetTimelockPendingAdmin()`, to queue the transaction for `Timelock.setPendingAdmin()`.
- `__executeSetTimelockPendingAdmin()`, to execute the transaction for `Timelock.setPendingAdmin()`.

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

![](https://accelerator-tasks-prod.s3.amazonaws.com/d443422032dc4e25897401782494a0b0/diagrams/centralization-GovernorAlpha-guardian.svg)

In the contracts `PriceOracle`, the role `anchorAdmin` has the authority over the following function:

- `_setPendingAnchor()`: set the anchor price for an asset,
- `_setPaused()`: pause or resume the market,

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

In the contracts `PriceOracle`, the role `poster` has the authority over the following function:

- `setPrice()`: set price for an asset,
- `setPrices()`:  set prices for a variable number of assets

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

In the function `setPriceInternal()`, it will not save the posted price when it exceeds the maximum swing(currently: 10%). So it cannot guarantee that the posted price is between the `price - max swing` and `price + max swing`, and the posted price could be changed to the max price when it exceeds the max swing. The real asset price may exceed the maximum swing.

```solidity=760
if (localVars.pendingAnchorMantissa != 0) {
	// let's explicitly set to 0 rather than relying on default of declaration
	localVars.anchorPeriod = 0;
	localVars.anchorPrice = Exp({mantissa : localVars.pendingAnchorMantissa});

	// Verify movement is within max swing of pending anchor (currently: 10%)
	(err, localVars.swing) = calculateSwing(localVars.anchorPrice, localVars.price);
	if (err != Error.NO_ERROR) {
		return failOracleWithDetails(asset, OracleError.FAILED_TO_SET_PRICE, OracleFailureInfo.SET_PRICE_CALCULATE_SWING, uint(err));
	}

	// Fail when swing > maxSwing
	if (greaterThanExp(localVars.swing, maxSwing)) {
		return failOracleWithDetails(asset, OracleError.FAILED_TO_SET_PRICE, OracleFailureInfo.SET_PRICE_MAX_SWING_CHECK, localVars.swing.mantissa);
	}
} else {
	localVars.anchorPeriod = anchors[asset].period;
	localVars.anchorPrice = Exp({mantissa : anchors[asset].priceMantissa});

	if (localVars.anchorPeriod != 0) {
		(err, localVars.priceCapped, localVars.price) = capToMax(localVars.anchorPrice, localVars.price);
		if (err != Error.NO_ERROR) {
			return failOracleWithDetails(asset, OracleError.FAILED_TO_SET_PRICE, OracleFailureInfo.SET_PRICE_CAP_TO_MAX, uint(err));
		}
		if (localVars.priceCapped) {
			// save for use in log
			localVars.cappingAnchorPriceMantissa = localVars.anchorPrice.mantissa;
		}
	} else {
		// Setting first price. Accept as is (already assigned above from requestedPriceMantissa) and use as anchor
		localVars.anchorPrice = Exp({mantissa : requestedPriceMantissa});
	}
}
```


Logical issue of `setPriceInternal()`

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

- `setGovernorAlpha()`, to set the `GovernorAlphaInterface` contract.
- `transferOwnership()`, to transfer ownership to another account.

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

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

The users will deposit `JST` token to mint `WJST`. They need to lock `WJST` token to the `WJST` contract to vote for a proposal. If there are multiple active proposals, the user's votes are locked in the voted proposal, but cannot vote for the others.


JustLend

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

16

0

11

3

2