Perpetual Protocol

1 Executive Summary

This report presents the results of our engagement with the Perpetual Protocol team to review the implementation of the Perpetual Protocol smart contract system.

The review was conducted over the course of 3 weeks, from September 21st to October 9th by John Mardlin, Valentin Wustholz, and Joran Honig.

1.1 Timeline

During the first week, much of our effort was invested in understanding the intended functioning and design of the system, as perpetual swaps are a complex financial instrument. We also ran automated analysis using the MythX platform, which helped to identify two significant arithmetic underflows.

During the second week we focused more deeply on manual code review and considered potential attacks on the system as implemented.

During the final week we focused more attention on reviewing the staking system and token issuance schedule.

1.2 Scope

Our review focused on the commit hash b972dce4f8a36627a3c9941fd2e2c00da9ecd2cf. The list of files in scope can be found in the Appendix.

1.3 Objectives

Together with the development team, we identified the following priorities for our review:

2 Recommendations

The Perpetual Protocol project is ambitious in scope and functionality, making it non-trivial to safely implement on-chain. Our opinion is that the number and severity of findings in the current system suggests a need for significant rethinking.

Rather than suggesting specific changes, the following recommendations are general in nature. They indicate the mindset that will steer the system, and the development team’s processes, towards a more secure orientation. Many of these recommendations will have the additional benefit of reducing gas costs.

2.1 Reduce the number of external calls

There are many calls made between contracts which could be avoided. For example, every external function in the DecimalERC20 library makes at least one call to token.decimals(). Since the number of decimals on a token should never change these calls are unnecessary. If the number of decimals on a token can change without significant warning, it is not safe to use.

2.2 Attempt to Reduce the number of Contracts

Combining separate contracts into one can significantly reduce the complexity by:

  • Reducing the number of roles required for access control
  • Reducing the number external calls required to access state in other contracts
  • Avoiding duplication of logic between similar contracts

An example of a tightly connected set of contracts is ClearingHouse, ClearingHouseVault, InsuranceFund, and AmmMgr. We suggest striving to pare down the functionality to fit these into one or two contracts. Even if this turns out to be impossible, the attempt will result in a simpler system with fewer possible edge cases.

2.3 Reduce the depth of wrapped contracts and abstraction

The system we reviewed is largely self contained in the sense that it interacts with very few external contracts. The main ones being the quote token (USDT), and the exchange used by the insurance fund (Balancer). However both of these external contracts have an additional “wrapper” contract between them and the system.

Although these wrappers will facilitate interacting with a variety of external systems, it may be solving a problem that does not yet exist.

Consider the possibility of removing ExchangeWrapper, DecimalERC20 and even the various decimal math libraries.

2.4 Be aware of the risks with loops

The codebase makes frequent use of for loops, to iterate over different tokens and instances of the AMM. At worst this can result in a Denial of Service if the gas required to complete a call exceeds that gas limit, at best it will result in increasing gas costs for interacting with the system.

It’s especially important to avoid looping over arrays which can be extended by users, as this would open up a direct DoS attack vector. From our observations loops were always over variables controlled by the owner, though we may have missed an exception due to lengthy call graphs from the site of the loop back to the source of the input.

There is also a reentrancy risk when an external call is made from within a loop. The current system interacts with very few untrusted contracts and otherwise protects against this using either the nonReentrant() modifier, or access controls.

2.5 Look for operations that can be done off-chain

In some cases it is possible to perform operations off-chain without reducing trustlessness. For example there are many on-chain calls made to the AmmMgr for the list of active AMMs. A more efficient approach would be to provide a getter so that the list can be read off-chain, and then the addresses can be passed as an argument in calldata when necessary for on-chain computation.

2.6 Specify and Review issuance tracking functionality in the contracts controlling PERP token minting

The desired functionality of the PERP token minting code is not well specified. This should be done, and then the functionality of Minter.sol, InflationMonitor.sol, SupplySchedule.sol reviewed against the specification.

2.7 Consider economic modelling

Economic modelling of the system could help to identify possible economic and market manipulation based attacks on the system.

3 System Overview

The system is quite complex, and has many different contracts which are heavily interconnected.

We used various visualization methods to facilitate comprehension, including diagrams which can be automatically generated using Surya.

The following diagram was created manually, and helps to illustrate the many connections and interdependencies in the system.

4 Findings

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

4.1 An attacker can sandwich liquidity migrations for profit. Critical

Resolution

Fixed in https://github.com/perpetual-protocol/perp-contract/pull/5/files and in https://github.com/perpetual-protocol/perp-contract/pull/54/files by making sure there is no profit or loss for any open positions after migrating liquidity.

(This fix is as reported by the developer team, but has not been verified by Diligence).

Description

Whenever liquidity is migrated this creates profit loss for the actors holding open positions. An attacker that could anticipate a liquidity migration can profit from this opportunity.

An attacker would open two positions (one long and one short), just before the liquidity migration. Once a liquidity migration has occurred, both of their positions will be multiplied with the expansion ratio.

If the expansion ratio is bigger than 1, then this will result in profit for the attacker, as the attacker can close both of his positions simultaneously after the expansion ratio is applied.

The only risk that the attacker has is the possibility that the market fluctuates to such an extent that his positions are put under water. Based on the risk that an attacker is willing to accept they can modify the leverage of their opened position.

Trader loss

Note that this same problem results in loss for regular traders whenever the expansion ratio is negative. The table below describes exactly such a scenario where Alice and Bob are put at a loss.

image

4.2 An attacker might extract funds from the Insurance Fund Critical

Resolution

Fixed in https://github.com/perpetual-protocol/perp-monorepo/pull/1505/files by adding a RestrictionMode which would be enabled when any position got liquidated or insurance fund has to pay for the liquidation losses.

(This fix is as reported by the developer team, but has not been verified by Diligence).

Description

Whenever an underwater position is closed, there is a potential need for a settlement based on the insurance fund. The clearinghouse includes a flaw that allows an attacker to profit from these settlements. Using this flaw, an attacker can drain the insurance fund.

Closing a position is beneficial for each actor that holds the inverse of the closed position (long -> short, short -> long). Each position has some collateral in the clearinghouse vault. When a position is closed at a loss, it will be covered using the collateral that was available for that position. Sometimes the collateral is insufficient to cover the loss, in which case the insurance fund will kick in to cover the remaining debt.

There is a profit opportunity for an attacker, as it is possible to open and close positions freely even when positions are closed at a net loss. So, an attacker can perform the following actions within a single transaction:

  1. An attacker can open a position.
  2. Open a second inverse position putting the first position underwater.
  3. Close the underwater position, resulting in the use of insurance funds.
  4. Close the second position

By performing all transactions in a single transaction, an attacker can ensure that they recuperate the complete loss of the underwater position (including the portion of the loss paid for by the insurance fund). Furthermore, an attacker could use a flash loan since they can execute the attack in a single transaction.

Examples

The following sequence of events describes an attack scenario where the attacker extracts funds from the insurance fund.

  1. The attacker gains temporary funds through a flash loan
  2. The attacker opens a short position using contract A
  3. The attacker opens a long position using contract B putting the short position of contract A underwater
  4. The attacker closes short position using contract A, resolving the remaining debt with the insurance fund
  5. The attacker closes a long position using contract B
  6. The attacker returns the flash loan

/Note/ The allocation of assets to the short & long position depends on the k (constant product) and current pool balance for the virtual AMM.

Recommendation

Limit the frequency at which an actor can change their position. Such a mitigation exists for liquidations already:

code/packages/contract/src/ClearingHouse.sol:L384

requireOneActionOnlyDuringLiquidationBlock(_amm);

Here, the code ensures that the attacker would not be able to update their position after liquidation. Similarly, one could consider not allowing position updates after the closing of an underwater position.

4.3 Unbounded array iteration in ClearingHouse::getPosition() may result in out of gas lockups Major

Description

In ClearingHouse.sol, the getPosition() function is within the callstack of several critical external functions, including closePosition(), settlePosition(), liquidate() and removeMargin().

code/packages/contract/src/ClearingHouse.sol:L572-L585

function getPosition(Amm _amm, address _trader) public view returns (Position memory) {
    Position memory pos = getUnadjustedPosition(_amm, _trader);
    uint256 liquidityChangedHistoryLength = ammMgr.getLiquidityChangedHistoryLength(_amm);
    if (pos.liquidityChangedIndex == liquidityChangedHistoryLength) {
        return pos;
    }

    for (uint256 i = pos.liquidityChangedIndex; i < liquidityChangedHistoryLength; i++) {
        Decimal.decimal memory expansionRatio = ammMgr.getLiquidityChangedHistoryItem(_amm, i).expansionRatio;
        pos.size = pos.size.mulD(expansionRatio);
    }
    pos.liquidityChangedIndex = liquidityChangedHistoryLength;
    return pos;
}

The starting index of the array iteration is determined by the ‘epoch’ during which a trader’s position was last updated. A new epoch is created each time the AmmMgr’s migrateLiquidity() function is called.

In the event that migrateLiquidity() is called frequently to adjust the constant product invariant of an AMM, traders with older position may be unable to close their positions.

Recommendation

See Be aware of the risks with loops above.

4.4 Safety check in token transfer logic may fail when transferred amounts are rounded Major

Description

In functions transfer and transferFrom there is a safety check that is supposed to prevent transfers of assets for deflationary tokens. This check may fail unintentionally if there is rounding; the code transfers a rounded value, but the check uses the original value.

code/packages/contract/src/utils/DecimalERC20.sol:L27

require(balanceAfter.cmp(balanceBefore.addD(_value)) == 0, "DecimalERC20: transfer value inconsistent");

Recommendation

Update the check to use the rounded value.

4.5 A miner can frontrun closePosition Major

Description

Finding issue 4.13 describes how miners might profit from maximizing slippage by front running openPosition calls. There, the existence of slippage protection limited the impact of the miner’s behaviour.

Such protection is not available for closePosition. As a result, a miner can arbitrarily profit from front running transactions that execute closePosition.

Examples

During regular trading, a miner might notice that a transaction for closePosition enters the mempool. At that point, the miner would attempt to construct a block that includes the following transaction sequence:

Miner Transaction: Open short position
Victim Transaction: Close long position
Miner Transaction: Close short position

Because there is no slippage protection, the attacker can open an arbitrarily large position and affect the price of the vAMM. The victim’s transaction will evaluate at a very unfavourable price, as closePosition does not have any slippage protection:

code/packages/contract/src/ClearingHouse.sol:L381

function closePosition(Amm _amm) external whenNotPaused() nonReentrant() {

The miner can close their position at a profit after the evaluation of the victim transaction.

Note that actors other than miners are also able to front run closePosition calls. We use miners as the primary actor because they can execute the attack without risk.

Recommendation

4.6 ClearingHouse’s payFunding() may be uncallable Major

Description

The payFunding() function in ClearingHouse.sol:

code/packages/contract/src/ClearingHouse.sol:L507-L515

function payFunding(Amm _amm) external {
    requireAmm(_amm, true);

    // must copy the baseAssetDeltaThisFundingPeriod
    SignedDecimal.signedDecimal memory baseAssetDeltaThisFundingPeriod = SignedDecimal.signedDecimal(
        _amm.baseAssetDeltaThisFundingPeriod()
    );

    SignedDecimal.signedDecimal memory premiumFraction = _amm.settleFunding();

Makes an external call to amm.settleFunding():

code/packages/contract/src/Amm.sol:L236-L237

function settleFunding() external onlyOpen returns (SignedDecimal.signedDecimal memory) {
    require(_blockTimestamp() >= nextFundingTime, "settle funding too early");

However settleFunding() funding can only be called once per hour, and will revert if the nextFundingTime has not yet been reached. This will also cause payFunding() to revert.

If payFunding() cannot be called, then funding payments are not kept up, and funds cannot be sent to or withdrawn from the Insurance Fund contract.

Recommendation

A potential solution is to prevent anyone from calling settleFunding() directly, and require it to be called by the Clearing House contract.

Description

Contract ChainlinkPriceFeed invokes several Chainlink functions without checking the return value properly. For instance, it uses the latestTimestamp function:

code/packages/contract/src/ChainlinkPriceFeed.sol:L108

uint256 latestTimestamp = aggregator.latestTimestamp();

which may simply return 0 if the round wasn’t complete: (see https://github.com/smartcontractkit/chainlink/blob/develop/evm-contracts/src/v0.6/AggregatorProxy.sol#L51-L54). It is not quite clear to us when exactly this may happen, but Chainlink’s documentation (see https://docs.chain.link/docs/historical-price-data) does check for non-zero timestamps. The documentation also points out the following:

Price Feeds are updated in rounds. Rounds are identified by their roundId, which increases with each new round (please note that the increase may not be monotonic).

We assume that this means that even if round N exists round N-1 might not. In such a case, querying round N-1 could result in 0 for the timestamp.

This non-monotonicity may impact function getTwapPrice where simply decrementing the round by 1 may result in terminating the computation too early.

code/packages/contract/src/ChainlinkPriceFeed.sol:L126

round = round.sub(1);

Recommendation

Please make sure to check return values properly and make sure that the code does not rely on the non-monotonicity assumption from above.

4.8 Integer underflows in RewardsDistribution Major

Description

There are two possible underflows in RewardsDistribution:

  • in removeRewardsDistribution :

code/packages/contract/src/RewardsDistribution.sol:L121

uint256 distributionsLatestIndex = distributions.length - 1;
  • in editRewardsDistribution:

code/packages/contract/src/RewardsDistribution.sol:L128

function editRewardsDistribution(uint256 _index, address _destination, Decimal.decimal memory _amount)

These can be triggered if distributions.length is 0. The first one will lead to a subsequent underflow of distributions.length, thereby essentially disabling all out-of-bounds checks. This may even allow the owner to override arbitrary storage slots (e.g., rewardsController and defaultRecipient) that should not be mutable.

The second one alone would allow editing any index.

Recommendation

The require-statements should be changed to avoid the subtraction. For instance, by changing

code/packages/contract/src/RewardsDistribution.sol:L133

require(_index <= distributions.length - 1, "index out of bounds");

to:

require(_index < distributions.length, "index out of bounds");

4.9 DecimalERC20 transfer check fails when the sender is the same as the receiver Major

Description

The DecimalERC20 checks the balance of a recipient address in a given token by calling token.balanceOf(_to) before and after calling token.transfer(_to, _amount). The return values are cached in memory variables named balanceBefore and balanceAfter.

A check is then performed to ensure that the balance has changed by the expected amount. However this check does not account for the case where the recipient is the same as the sender, which is a valid ERC20 operation, and would result in the call reverting. If used incorrectly, this could cause a contract to lock up.

Examples

code/packages/contract/src/utils/DecimalERC20.sol:L15-L27

function transfer(IERC20 _token, address _to, Decimal.decimal memory _value) internal {
    Decimal.decimal memory balanceBefore = balanceOf(_token, _to);

    // bytes4(keccak256(bytes('transfer(address,uint256)')));
    // solhint-disable avoid-low-level-calls
    (bool success, bytes memory data) = address(_token).call(
        abi.encodeWithSelector(0xa9059cbb, _to, toUint(_token, _value))
    );
    require(success && (data.length == 0 || abi.decode(data, (bool))), "DecimalERC20: transfer failed");

    // To prevent from deflationary token, check receiver's balance is as expectation.
    Decimal.decimal memory balanceAfter = balanceOf(_token, _to);
    require(balanceAfter.cmp(balanceBefore.addD(_value)) == 0, "DecimalERC20: transfer value inconsistent");

Recommendation

A potential fix would be to add a check if _to == _from and skip the other checks if this true.

Another option is to simply cast the Decimal type and call transferFrom() without the additional complexity of using the DecimalERC20 library, as it adds considerable complexity and cost.

4.10 Failures due to missing token approvals Medium

Description

In the exchange wrapper one can set the Balancer pool. This will also approve all its current tokens:

code/packages/contract/src/exchangeWrapper/ExchangeWrapper.sol:L122

address[] memory tokens = balancerPool.getCurrentTokens();

It seems that this set of tokens can change with time (for instance, new tokens could be added or removed). The exchange wrapper does not account for that. More specifically, if a token would be added to the pool after it was set certain operations may fail due to the missing approvals.

Similarly, if a token was removed it would be good to remove the approval.

Recommendation

Consider approving tokens on demand instead of upfront and maybe tracking which tokens have been approved. Alternatively, it might be possible to only permit “finalized” Balancer pools (see https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol#L101).

4.11 Function getTokenDecimals defaults to 18 in case of failure Medium

Description

In function DecimalERC20.getTokenDecimals the return value defaults to 18 in case the call to the token’s decimals function fails:

code/packages/contract/src/utils/DecimalERC20.sol:L97

return 18;

A failure may happen if the token does not expose this function. It could, however, also happen if the call runs out of gas. It seems more conservative to fail fast by requiring the call to succeed.

In general, we recommend to be careful with what tokens are allowed in the system by performing a thorough review of the token’s contract (incl. after potential upgrades). For instance, such a review should check that the token exposes the decimals function and that that function always return the same value (non-constant decimals could potentially break the system).

Recommendation

Consider requiring the call to the token to suceeed.

4.12 Cascading liquidations introduce undesirable market effects Medium

Description

Liquidating a position will unduly affect the price of the base asset in the vAMM. Such slippage creates an arbitrage opportunity. An arbitrageur might execute an inverse of the liquidation trade, restoring the price of the vAMM to the market price.

Permitting chain liquidations has three negative consequences:

First, the slippage introduced by the liquidations performed in a chain will compound, resulting in an exponentially profitable arbitrage opportunity. The liquidator can execute on this opportunity by opening a position directly after the evaluation of the liquidation chain.

Second, the price of an asset might change due to the liquidation of an underwater position. Such a price change does not necessarily represent the actual market price (i.e., the price that actors are willing to pay for the asset). Typically, a single liquidation would result in an arbitrage opportunity, which would return the price to match the market’s demand.
However, if chain liquidations are allowed, a liquidator can liquidate further positions based on the altered vAMM asset price. That introduces increased market volatility and increased risk for a trader.

Third, in chain liquidations, there is an increased risk of liquidation with bad debt. Bad debt will result in losses for the insurance fund, and potentially the value of the Perp token.

Recommendation

It is necessary to constrain how frequently positions can be liquidated.

4.13 Miners are incentivised to maximise slippage and to perform strategic block composition Medium

Description

Miners can arbitrarily determine which transactions will be part of a basic block. This means that they have an unfair advantage relative to regular traders:

  • First, they can open a position at the beginning of a block and close at the end without any associated risks.
  • Second, they can know and alter the market fluctuation throughout the trades included in a block.

These aspects affect how a miner might behave to maximize reward.

Front running openPosition to alter prices

The primary way that an attacker might leverage their abilities is by introspecting the transaction pool and see if there are any perp positions being opened/adjusted. When a transaction results in a price change on the vAMM, the miner will construct a block which does the following:

Miner Transaction: Open long position
...
Victim Transaction: Open long position
...
Miner Transaction: Close long position

In this scenario, the miner increases the price of the asset in the vAMM before the victim trader’s transaction is evaluated. Essentially, the attacking miner would frontrun to introduce arbitrary slippage on the victim’s trade.

Fortunately, there is some protection against slippage; openPosition allows the victim to pass minBaseAssetAmount:

code/packages/contract/src/ClearingHouse.sol:L326

Decimal.decimal calldata _minBaseAssetAmount

By setting the minimum amount of tokens, the victim can ensure that their trade is not executed at a price which is too unfavorable. As a result, the miner can not inflate the market arbitrarily. Instead, the attacker can only inflate the market to a point where the Victim’s trade results in at least minBaseAssetAmout.

This protection limits the profitability of front-running openPosition calls, but an attacker is still incentivized to maximize slippage on these trades.

4.14 Unnecessarily high complexity in function mintedAmountDuringMintThresholdPeriod Medium

Description

Function mintedAmountDuringMintThresholdPeriod (see) in contract InflationMonitor could be simplified significantly by not storing the cumulative amount per entry in the mintedTokenHistory array. The loop could just sum up the minted amounts within the threshold period.

code/packages/contract/src/InflationMonitor.sol:L73-L97

 function mintedAmountDuringMintThresholdPeriod() public view returns (Decimal.decimal memory) {
     uint256 len = mintedTokenHistory.length;
     if (len == 0) {
         return Decimal.zero();
     }

     uint256 durationSinceLastMinted = _blockTimestamp().sub(mintedTokenHistory[len - 1].timestamp);
     if (durationSinceLastMinted > MINT_THRESHOLD_PERIOD) {
         return Decimal.zero();
     }

     Decimal.decimal memory minted;
     for (uint256 i = len - 1; i > 0; i--) {
         Decimal.decimal memory amount = mintedTokenHistory[i].cumulativeAmount.subD(
             mintedTokenHistory[i - 1].cumulativeAmount
         );
         minted = minted.addD(amount);

         durationSinceLastMinted += mintedTokenHistory[i].timestamp.sub(mintedTokenHistory[i - 1].timestamp);
         if (durationSinceLastMinted > MINT_THRESHOLD_PERIOD) {
             break;
         }
     }
     return minted;
 }

Recommendation

Consider changing MintedTokenEntry to store the minted amount instead of the cumulative amount and adapting function mintedAmountDuringMintThresholdPeriod accordingly.

Description

Contract ChainlinkPriceFeed is using a deprecated Chainlink API (see https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference). As a result, there are many external calls (for instance, in function getTwapPrice) that drive up gas usage.

Recommendation

Consider using the the new API. This should simplify the code significantly and reduce gas usage. In particular, many calls can be replaced by a single call to latestRoundData.

In addition, calling the decimals function in the new API may eliminate the need for the hard-coded USD_PAIR_DIGIT constant.

4.16 Attacker can circumvent price manipulation mitigation Medium

Description

The liquidate function in ClearingHouse attempts to mitigate price manipulation. This mitigation should prevent an attacker from opening a position to put another position underwater, and consecutively liquidating that other position.

The current mitigation ensures that the msg.sender executing the liquidate(…) function is not allowed to have changed their position before liquidating another position in the same AMM.

code/packages/contract/src/ClearingHouse.sol:L434-L438

 address liquidator = _msgSender();
 require(
     getUnadjustedPosition(_amm, liquidator).blockNumber != _blockNumber(),
     "can't open and liquidate in the same block"
 );

However, it is possible to circumvent this mitigation. An attacker can use different smart contracts to send transactions from separate addresses, resulting in different values for msg.sender. Doing so allows an attacker to open a position and liquidate an underwater position in the same block.

The following check, in closePosition(), limits the impact of this attack:

code/packages/contract/src/ClearingHouse.sol:L384

 requireOneActionOnlyDuringLiquidationBlock(_amm);

This check ensures that the attacker is not able to open, liquidate and close his position in the same transaction.

Recommendation

A possible mitigation is the introduction of a time constraint on liquidation. Making sure that a position can not be put underwater and liquidated in the same block prevents the attack scenario as described above.

4.17 Ineffective front-running protection in DecimalERC20.approve Medium

Description

In DecimalERC20 the approve function attempts to prevent front-running:

code/packages/contract/src/utils/DecimalERC20.sol:L47-L52

// to prevent from front-run, set allowance to 0 first and then set what user wants
(bool success, bytes memory data) = address(_token).call(abi.encodeWithSelector(0x095ea7b3, _spender, 0));
require(success && (data.length == 0 || abi.decode(data, (bool))), "DecimalERC20: approve(0) failed");

(success, data) = address(_token).call(abi.encodeWithSelector(0x095ea7b3, _spender, toUint(_token, _value)));
require(success && (data.length == 0 || abi.decode(data, (bool))), "DecimalERC20: approve failed");

It uses two approve calls to the underlying ERC20 token where the first sets the approval to 0. However, since both calls happen within the same transaction they will both execute either before or after a potential front-runner. Therefore, this itself doesn’t prevent front-running.

Recommendation

Consider other ways to protect against front-running or review the need for such a protection (for instance, this shouldn’t be necessary if the approved user is trusted).

4.18 getSpotPrice returns incorrect price if both input and output are USDT Minor

Description

Function getSpotPrice in contract ExchangeWrapper returns an incorrect price if both input and output token are USDT:

code/packages/contract/src/exchangeWrapper/ExchangeWrapper.sol:L412-L416

if (isUSDT(_inputToken)) {
    return price.mulD(compoundUnderlyingAmount(Decimal.one()));
} else if (isUSDT(_outputToken)) {
    return price.divD(compoundUnderlyingAmount(Decimal.one()));
}

Recommendation

Consider changing the code to correctly handle this special case.

4.19 Use of uninitialized variable timeWeightedLocked Minor

Description

The following two lines use the variable timeWeightedLocked

code/packages/contract/src/StakingReserve.sol:L150-L151

totalEffectiveStakeMap[nextEpochIndex()] = totalEffectiveStakeMap[nextEpochIndex()].addD(timeWeightedLocked);
totalPendingStakeBalance = totalPendingStakeBalance.addD(_amount).subD(timeWeightedLocked);

However, if nextEndEpochTimestamp <= _blockTimestamp() then this variable will not be initialized.

Recommendation

Initialize timeWeightedLocked to Decimal.zero().

code/packages/contract/src/StakingReserve.sol:L125

Decimal.decimal memory timeWeightedLocked;

4.20 Unnecessary if statement Minor

Description

The following if statement is not required:

code/packages/contract/src/StakingReserve.sol:L233

if (fees.length > 0) {

This is the case, because the loop guarded by the ifstatement would not execute it’s body if the length of the array is 0.

code/packages/contract/src/StakingReserve.sol:L234-L238

for (uint256 i = 0; i < fees.length; i++) {
    if (fees[i].balance.toUint() != 0) {
        DecimalERC20.transfer(IERC20(fees[i].token), staker, fees[i].balance);
    }
}

4.21 Unused field priceFeedKeys in ChainlinkPriceFeed Minor

Description

In contract ChainlinkPriceFeed the priceFeedKeys field is populated and maintained, but it seems like no clients are using that field. If so, removing the field could simplify the code and reduce gas cost.

Recommendation

Consider removing the priceFeedKeys field unless it is needed in the future.

4.22 Assertions can be violated Minor

Description

There are three assertions that can potentially be violated:

code/packages/contract/src/Amm.sol:L685

assert(quoteAssetAfter.toUint() != 0);

code/packages/contract/src/Amm.sol:L724

assert(baseAssetAfter.toUint() != 0);

code/packages/contract/src/ClearingHouse.sol:L755

assert(remainOpenNotional.toInt() > 0);

MythX was able to detect violations of the first two and the developers confirmed that all three could potentially be violated.

Recommendation

Consider changing the assert-statements to require-statements. We recommend to only use assert-statements for conditions that should always hold. In addition, unlike a failing assert-statement, a failing require-statement does not consume all the remaining gas.

4.23 Reward distribution may fail Minor

Description

Function distributeRewards in contract RewardsDistribution distributes a given amount of rewards _amount by paying fixed amounts based on the added distributions. This may fail if the _amount is too small:

code/packages/contract/src/RewardsDistribution.sol:L81

remainder = remainder.subD(distributions[i].amount);

Recommendation

Consider distributing rewards based on a fraction instead of an absolute value that is distributed to each destination.

4.24 Missing input validation in RewardsDistribution.addRewardsDistribution Minor

Description

The addRewardsDistribution function makes it possible to add a distribution for a destination that has already been added:

code/packages/contract/src/RewardsDistribution.sol:L111-L112

require(_destination != address(0), "Cant add a zero address");
require(_amount.toUint() != 0, "Cant add a zero amount");

This seems undesirable given that the array may grow large and is iterated over in function distributeRewards. Besides, one can always edit the element via the editRewardsDistribution function.

Recommendation

Consider adding additional parameter validation to avoid multiple elements in the distributions array with the same destination.

4.25 Potentially misleading return value of inflationRate field in SupplySchedule contract Minor

Description

A user that calls the getter for the inflationRate field in the SupplySchedule contract may observe a decaying inflation rate even after supplyDecayEndTime has been reached:

code/packages/contract/src/SupplySchedule.sol:L80

inflationRate = inflationRate.mulD(Decimal.one().subD(decayRate));

However, at this point the inflation rate will be fixed (TERMINAL_SUPPLY_EPOCH_RATE).

Recommendation

Consider making the field private (_inflationRate) and adding a getter (inflationRate()) instead that returns the private field before supplyDecayEndTime and returns TERMINAL_SUPPLY_EPOCH_RATE afterwards. This should also simplify the logic of funtion mintableSupply.

4.26 Outdated Solidity version Minor

Description

The codebase is using an outdated version of the Solidity compiler (0.6.4).

Recommendation

Consider using an up-to-date version (ideally 0.7.1 or at least 0.6.12).

4.27 Unused return values in RewardsDistribution contract Minor

Description

In the RewardsDistribution contract, there are two function that always return true:

  1. function addRewardsDistribution:

code/packages/contract/src/RewardsDistribution.sol:L106-L118

function addRewardsDistribution(address _destination, Decimal.decimal memory _amount)
    public
    onlyOwner
    returns (bool)
{
    require(_destination != address(0), "Cant add a zero address");
    require(_amount.toUint() != 0, "Cant add a zero amount");

    DistributionData memory rewardsDistribution = DistributionData(address(_destination), _amount);
    distributions.push(rewardsDistribution);

    return true;
}
  1. function editRewardsDistribution:

code/packages/contract/src/RewardsDistribution.sol:L128-L139

function editRewardsDistribution(uint256 _index, address _destination, Decimal.decimal memory _amount)
    public
    onlyOwner
    returns (bool)
{
    require(_index <= distributions.length - 1, "index out of bounds");

    distributions[_index].destination = _destination;
    distributions[_index].amount = _amount;

    return true;
}

Recommendation

Consider dropping the return value if it isn’t used anywhere and is always true.

4.28 Integer underflows in Amm contract Minor

Description

There are two potential underflows in the Amm contract: - in function addReserveSnapshot:

code/packages/contract/src/Amm.sol:L458

ReserveSnapshot storage latestSnapshot = reserveSnapshots[reserveSnapshots.length - 1];
  • in function isOverPriceFluctuation:

code/packages/contract/src/Amm.sol:L631

ReserveSnapshot memory latestSnapshot = reserveSnapshots[len - 1];

Both of them don’t seem exploitable and they could only be triggered if the initialize function had not been called yet.

Recommendation

Consider adding protection to prevent those overflows.

Appendix 1 - Files in Scope

This audit covered the following files:

File SHA-1 hash
Amm.sol b5b770278aa859e8980070bbe824a1973600f855
AmmMgr.sol b28cc827bd17551eab2fd86070e56a1b9a0fa1fc
AmmReader.sol 8ed805f8bbc972e0ad627f638b63f256654ee90a
ChainlinkPriceFeed.sol 3fec2760931dcf3587f7e80981cdef13dd56fb6f
ClearingHouse.sol 66ab2bcc809403f1c2354bd1cff3cdc5bd394e59
ClearingHouseVault.sol 7b1e823fec4718dabd54523b60292b3b6da8f733
ClearingHouseViewer.sol 8c1236c6d73210535f92b92155f9c04cfc7707ba
InflationMonitor.sol fb16361c9ada5c59ea555723db8a99244739d3b3
InsuranceFund.sol 598e138bc850761de166267a6e64b0b490f5c542
Minter.sol 481bc8f6f7ca4038b221bec209800178c4497373
OwnerPausable.sol 01d8439fd81e50a862a2085c794e27d3c6a55f26
PerpToken.sol e29b2600796b9c71d27e3b3e45a925d7ac91b4e4
RewardsDistribution.sol f2a29353c99b5a540e0bc131e51854715b664c69
RewardsDistributionRecipient.sol dfe53619a73214e818839c3151c60e9bdf8126c6
StakingReserve.sol 7caeb10bcb71b28f313cb431d5df0ed240b20e60
SupplySchedule.sol 37b71b0abe5f5687e313a4b4570a28128269b735
exchangeWrapper/Balancer/BPool.sol bb44b1be88e4649af2e716b800eb55e80cb1cc58
exchangeWrapper/Compound/CTokenInterface.sol 8f3e2cf9c18e5c049cb19606fe873e3152e6b4a2
exchangeWrapper/ExchangeWrapper.sol 1892c4c6abb252c22929e35b91e3b87600b31cdd
interface/IExchangeWrapper.sol 67bbe4778b782ef89e91e9c684860780b8175d53
interface/IPriceFeed.sol d7eb6c4a9ad17147a7286a76dae20896253c7232
interface/IRewardRecipient.sol 255ccb5e1b43e84d03bb7d4adc32e3ece5e326f0
interface/IRewardsDistribution.sol 9d7876e15fc353b8d37fe6287c155d0a4a209cae
utils/BlockContext.sol 3b30b3a31b6ffaa80eff608b6b67430504d9634e
utils/Decimal.sol 7d48cb5230de9671b2c5370f66be081004ca63c8
utils/DecimalERC20.sol 0baecf174f4839c6023576a854b46eed07fc29df
utils/DecimalMath.sol 4b5e87abea20f4b4d6817add70fa28e53c82ec7c
utils/MixedDecimal.sol 61342ad53d3fb1c2271ade5e5e6c003347798f3c
utils/PerpFiOwnable.sol 7844489606fd8d6510010e2f9c9e57b091ffe286
utils/PerpFiOwnableUpgrade.sol 30f3a4159153b7aebb3a2c71b29e78e552f2ed49
utils/SignedDecimal.sol 89fcadc33806c571d99697beedb1cf5b36b34515
utils/SignedDecimalMath.sol 8db6badba850a09631f76b2c6913334e9f2a1175

Appendix 2 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.