dForce Lending Protocol Review

Description

Traditionally, collateral refers to: “an asset pledged as security for the repayment of a loan, to be forfeited in the event of a default.”

Using dForce, users may deposit assets into the system and borrow assets from the system. When borrowing assets, the system calculates the amount that can be borrowed as a function of the value of the user’s collateral. Users must explicitly mark deposited assets as collateral via the Controller.enterMarkets method, which adds the asset to the user’s “collaterals” list:

code/contracts/Controller.sol:L1397-L1409

function _enterMarket(address _iToken, address _account)
    internal
    returns (bool)
{
    // Market not listed, skip it
    if (!iTokens.contains(_iToken)) {
        return false;
    }

    // add() will return false if iToken is in account's market list
    if (accountsData[_account].collaterals.add(_iToken)) {
        emit MarketEntered(_iToken, _account);
    }

Users may deposit and hold assets without marking them as collateral via this method. Using the traditional definition of “collateral,” a user may expect that assets not marked as collateral cannot be seized in the event they default on a loan. However, this is not the case: ANY asset deposited by the user can be seized during liquidation, regardless of whether it was marked collateral or not.

Recommendation

Given this subversion of expectations, we recommend ensuring that dForce’s users have a clear understanding of their risks and responsibilities when they deposit assets into the lending platform.

• Consider revisiting the term “collateral” to apply to all assets deposited into the system.

• Consider creating user-facing documentation that clearly outlines the meaning of the term.

Description

Base._updateInterest is executed before most operations in the dForce system. The method accumulates interest from borrows since the last time the method was called, and adds a portion to the contract’s reserves. It then updates these values in contract state, ensuring the action being taken is using the most up-to-date values:

code/contracts/TokenBase/Base.sol:L140-L144

// Writes the previously calculated values into storage.
accrualBlockNumber = _vars.currentBlockNumber;
borrowIndex = _vars.newBorrowIndex;
totalBorrows = _vars.newTotalBorrows;
totalReserves = _vars.newTotalReserves;

_updateInterest is relatively long, and will likely be called several times per block. In this case, the blockDelta used to calculate accumulated interest will result in a calculated value of “0” interest accumulated:

code/contracts/TokenBase/Base.sol:L109-L126

// Records the current block number.
_vars.currentBlockNumber = block.number;

// Calculates the number of blocks elapsed since the last accrual.
_vars.blockDelta = _vars.currentBlockNumber.sub(accrualBlockNumber);

/**
 * Calculates the interest accumulated into borrows and reserves and the new index:
 *  simpleInterestFactor = borrowRate * blockDelta
 *  interestAccumulated = simpleInterestFactor * totalBorrows
 *  newTotalBorrows = interestAccumulated + totalBorrows
 *  newTotalReserves = interestAccumulated * reserveFactor + totalReserves
 *  newBorrowIndex = simpleInterestFactor * borrowIndex + borrowIndex
 */
_vars.simpleInterestFactor = _vars.borrowRate.mul(_vars.blockDelta);
_vars.interestAccumulated = _vars.simpleInterestFactor.rmul(
    _vars.totalBorrows
);

When no interest has been accumulated, the method’s state changes have no net effect.

Recommendation

In order to save gas on repeated calls in the same block, _updateInterest should return early if accrualBlockNumber == block.number.
Note that the function currently emits an UpdateInterest event even if this is not the first call in this block. It might be worth mentioning that returning early if accrualBlockNumber == block.number will change that behavior — which is probably a good thing since nothing has been updated anyway.

Description

issue 4.5 describes a requirement in RewardDistributor that conflicts with dForce’s plans to transition the Owner role to a smart contract. This finding suggests that this eventual transition has not been sufficiently planned, and is wholly untested.

Given that this is an important milestone for the protocol, it is important to plan for its execution well in advance. Whether the Owner role will be held by an EOA, a multisig, or a DAO, the capabilities of the Owner are very important to the system as a whole.

Examples

The following examples outline some basic design considerations dForce should plan for now, ahead of this eventual transition:

• The Owner needs to be able to execute multiple actions atomically. This is primarily important because there will be situations where multiple function calls are required to safely carry out a change.

  • For example, in the event the MSDController needs to be updated, calls to iMSD._setMSDController and MSDS._setMSDController need to happen atomically. Without atomic execution of both methods, a window of time exists where transactions may interact with one or both of these contracts in a half-configured state.
  • As another example, if an InterestRateModel is found to be faulty, a complete upgrade requires calling each iToken individually (iToken._setInterestRateModel).

• Owner actions should be timelocked, in order to remove unpredictability for users. As explained in 2.2 Risks, instant configuration changes mean users cannot be sure what the behavior of a function call will be, as this behavior can change at any time. Transitioning the Owner role to a smart contract enables the possibility of implementing time-locked actions, where a mandatory delay ensures users are able to react to pending Owner actions in time.

Recommendation

• Extend current testing of the Owner role with tests where the Owner is replaced by a smart contract, preferably one capable of batching actions.

• Come up with real-world scenarios where Owner actions are needed, then test these scenarios. An easy way to come up with these scenarios is to consider cases where a bug is discovered in one or more of the system’s components. What actions should the Owner take in these scenarios? Can those actions be taken safely using both an EOA and a smart contract?

Description

There are several instances of duplicated code throughout the codebase. This should generally be avoided as it reduces maintainability and readability of the source code, increases source code length, and might increase bytecode length.

Examples

1. There are four interest rate models; three of them employ the asset’s utilization rate and define the exact same function for its computation. In issue 4.3, we suggest modifications to this function; currently, they would have to be applied to all three instances of this code.

2. All four models contain the following function, which —although trivial and unlikely to change — would better be placed in a base contract all interest rate models inherit from:

code/contracts/InterestRateModel/InterestRateModel.sol:L65-L70

/**
 * @notice Ensure this is an interest rate model contract.
 */
function isInterestRateModel() external pure returns (bool) {
    return true;
}

Recommendation

Well-known techniques like inheritance and use of libraries help avoid code duplication.

Description

iETH.exchangeRateStored returns the exchange rate of the contract as a function of the current cash of the contract. In the case of iETH, current cash is calculated as the contract’s ETH balance minus msg.value:

code/contracts/iETH.sol:L54-L59

/**
 * @dev Gets balance of this contract in terms of the underlying
 */
function _getCurrentCash() internal view override returns (uint256) {
    return address(this).balance.sub(msg.value);
}

msg.value is subtracted because the majority of iETH methods are payable, and msg.value is implicitly added to a contract’s balance before execution begins. If msg.value were not subtracted, the value sent with a call could be used to inflate the contract’s exchange rate artificially.

As part of execution, iETH makes calls to the Controller, which performs important checks using (among other things) the stored exchange rate. When exchangeRateStored is invoked from the Controller, the call context has a msg.value of 0. However, the msg.value sent by the initial iETH execution is still included in the contract’s balance. This means that the Controller receives an exchange rate inflated by the initial call’s msg.value.

Examples

This problem occurs in multiple locations in the Controller:

• beforeMint uses the exchange rate to ensure the supply capacity of the market is not reached. In this case, inflation would prevent the entire supply capacity of the market from being utilized:

code/contracts/Controller.sol:L670-L678

// Check the iToken's supply capacity, -1 means no limit
uint256 _totalSupplyUnderlying =
    IERC20Upgradeable(_iToken).totalSupply().rmul(
        IiToken(_iToken).exchangeRateStored()
    );
require(
    _totalSupplyUnderlying.add(_mintAmount) <= _market.supplyCapacity,
    "Token supply capacity reached"
);

• beforeLiquidateBorrow uses the exchange rate via calcAccountEquity to calculate the value of the borrower’s collateral. In this case, inflation would increase the account’s equity, which could prevent the liquidator from liquidating:

code/contracts/Controller.sol:L917-L919

(, uint256 _shortfall, , ) = calcAccountEquity(_borrower);

require(_shortfall > 0, "Account does not have shortfall");

Recommendation

• Rather than having the Controller query the iETH.exchangeRateStored, the exchange rate could be passed-in to Controller methods as a parameter.

• Ensure no other components in the system rely on iETH.exchangeRateStored after being called from iETH.

Description

Controller.calcAccountEquity calculates the relative value of a user’s supplied collateral and their active borrow positions. Users may mark an arbitrary number of assets as collateral, and may borrow from an arbitrary number of assets. In order to calculate the value of both of these positions, this method performs two loops.

First, to calculate the sum of the value of a user’s collateral:

code/contracts/Controller.sol:L1227-L1233

// Calculate value of all collaterals
// collateralValuePerToken = underlyingPrice * exchangeRate * collateralFactor
// collateralValue = balance * collateralValuePerToken
// sumCollateral += collateralValue
uint256 _len = _accountData.collaterals.length();
for (uint256 i = 0; i < _len; i++) {
    IiToken _token = IiToken(_accountData.collaterals.at(i));

Second, to calculate the sum of the value of a user’s borrow positions:

code/contracts/Controller.sol:L1263-L1268

// Calculate all borrowed value
// borrowValue = underlyingPrice * underlyingBorrowed / borrowFactor
// sumBorrowed += borrowValue
_len = _accountData.borrowed.length();
for (uint256 i = 0; i < _len; i++) {
    IiToken _token = IiToken(_accountData.borrowed.at(i));

From dForce, we learned that 200 or more assets would be supported by the Controller. This means that a user with active collateral and borrow positions on all 200 supported assets could force any calcAccountEquity action to perform some 400 iterations of these loops, each with several expensive external calls.

Examples

By modifying dForce’s unit test suite, we showed that an attacker could force the cost of calcAccountEquity above the block gas limit. This would prevent all of the following actions, as each relies on calcAccountEquity:

• iToken.transfer and iToken.transferFrom
• iToken.redeem and iToken.redeemUnderlying
• iToken.borrow
• iToken.liquidateBorrow and iToken.seize

The following actions would still be possible:

• iToken.mint
• iToken.repayBorrow and iToken.repayBorrowBehalf

As a result, an attacker may abuse the unbounded looping in calcAccountEquity to prevent the liquidation of underwater positions. We provided dForce with a PoC here: gist.

Recommendation

There are many possible ways to address this issue. Some ideas have been outlined below, and it may be that a combination of these ideas is the best approach:

In general, cap the number of markets and borrowed assets a user may have: The primary cause of the DoS is that the number of collateral and borrow positions held by a user is only restricted by the number of supported assets. The PoC provided above showed that somewhere around 150 collateral positions and 150 borrow positions, the gas costs of calcAccountEquity use most of the gas in a block. Given that gas prices often spike along with turbulent market conditions and that liquidations are far more likely in turbulent market conditions, a cap on active markets / borrows should be much lower than 150 each so as to keep the cost of liquidations as low as possible.

dForce should perform their own gas cost estimates to determine a cap, and choose a safe, low value. Estimates should be performed on the high-level liquidateBorrow method, so as to simulate an actual liquidation event. Additionally, estimates should factor in a changing block gas limit, and the possibility of opcode gas costs changing in future forks. It may be wise to make this cap configurable, so that the limits may be adjusted for future conditions.

Description

The utilization rate UR of an asset forms the basis for interest calculations and is defined as borrows / ( borrows + cash - reserves).

code/contracts/InterestRateModel/InterestRateModel.sol:L72-L88

/**
 * @notice Calculate the utilization rate: `_borrows / (_cash + _borrows - _reserves)`
 * @param _cash Asset balance
 * @param _borrows Asset borrows
 * @param _reserves Asset reserves
 * @return Asset utilization [0, 1e18]
 */
function utilizationRate(
    uint256 _cash,
    uint256 _borrows,
    uint256 _reserves
) internal pure returns (uint256) {
    // Utilization rate is 0 when there are no borrows
    if (_borrows == 0) return 0;

    return _borrows.mul(BASE).div(_cash.add(_borrows).sub(_reserves));
}

The implicit assumption here is that reserves <= cash; in this case — and if we define UR as 0 for borrows == 0 — we have 0 <= UR <=1. We can view cash - reserves as “available cash”. However, the system does not guarantee that reserves never exceeds cash. If reserves > cash (and borrows + cash - reserves > 0), the formula for UR above gives a utilization rate above 1. This doesn’t make much sense conceptually and has undesirable technical consequences; an especially severe one is analyzed in issue 4.4.

Recommendation

If reserves > cash — or, in other words, available cash is negative — this means part of the reserves have been borrowed, which ideally shouldn’t happen in the first place. However, the reserves grow automatically over time, so it might be difficult to avoid this entirely. We recommend (1) avoiding this situation whenever it is possible and (2) fixing the UR computation such that it deals more gracefully with this scenario. More specifically:

1. Loan amounts should not be checked to be smaller than or equal to cash but cash - reserves (which might be negative). Note that the current check against cash happens more or less implicitly because the transfer just fails for insufficient cash.
2. Make the utilization rate computation return 1 if reserves > cash (unless borrows == 0, in which case return 0 as is already the case).

Remark

Internally, the utilization rate and other fractional values are scaled by 1e18. The discussion above has a more conceptual than technical perspective, so we used unscaled numbers. When making changes to the code, care must be taken to apply the scaling.

Description

Before executing most methods, the iETH and iToken contracts update interest accumulated on borrows via the method Base._updateInterest. This method uses the contract’s interest rate model to calculate the borrow interest rate. If the calculated value is above maxBorrowRate (0.001e18), the method will revert:

code/contracts/TokenBase/Base.sol:L92-L107

function _updateInterest() internal virtual override {
    InterestLocalVars memory _vars;
    _vars.currentCash = _getCurrentCash();
    _vars.totalBorrows = totalBorrows;
    _vars.totalReserves = totalReserves;

    // Gets the current borrow interest rate.
    _vars.borrowRate = interestRateModel.getBorrowRate(
        _vars.currentCash,
        _vars.totalBorrows,
        _vars.totalReserves
    );
    require(
        _vars.borrowRate <= maxBorrowRate,
        "_updateInterest: Borrow rate is too high!"
    );

If this method reverts, the entire contract may halt and be unrecoverable. The only ways to change the values used to calculate this interest rate lie in methods that must first call Base._updateInterest. In this case, those methods would fail.

One other potential avenue for recovery exists: the Owner role may update the interest rate calculation contract via TokenAdmin._setInterestRateModel:

code/contracts/TokenBase/TokenAdmin.sol:L46-L63

/**
 * @dev Sets a new interest rate model.
 * @param _newInterestRateModel The new interest rate model.
 */
function _setInterestRateModel(
    IInterestRateModelInterface _newInterestRateModel
) external virtual onlyOwner settleInterest {
    // Gets current interest rate model.
    IInterestRateModelInterface _oldInterestRateModel = interestRateModel;

    // Ensures the input address is the interest model contract.
    require(
        _newInterestRateModel.isInterestRateModel(),
        "_setInterestRateModel: This is not the rate model contract!"
    );

    // Set to the new interest rate model.
    interestRateModel = _newInterestRateModel;

However, this method also calls Base._updateInterest before completing the upgrade, so it would fail as well.

Examples

We used interest rate parameters taken from dForce’s unit tests to determine whether any of the interest rate models could return a borrow rate that would cause this failure. The default InterestRateModel is deployed using these values:

baseInterestPerBlock: 0
interestPerBlock: 5.074e10
highInterestPerBlock: 4.756e11
high: 0.75e18

Plugging these values in to their borrow rate calculations, we determined that the utilization rate of the contract would need to be 2103e18 in order to reach the max borrow rate and trigger a failure. Plugging this in to the formula for utilization rate, we derived the following ratio:

reserves >= (2102/2103)*borrows + cash

With the given interest rate parameters, if token reserves, total borrows, and underlying cash meet the above ratio, the interest rate model would return a borrow rate above the maximum, leading to the failure conditions described above.

Recommendation

Note that the examples above depend on the specific interest rate parameters configured by dForce. In general, with reasonable interest rate parameters and a reasonable reserve ratio, it seems unlikely that the maximum borrow rate will be reached. Consider implementing the following changes as a precaution:

• As utilization rate should be between 0 and 1 (scaled by 1e18), prevent utilization rate calculations from returning anything above 1e18. See issue 4.3 for a more thorough discussion of this topic.

• Remove the settleInterest modifier from TokenAdmin._setInterestRateModel: In a worst case scenario, this will allow the Owner role to update the interest rate model without triggering the failure in Base._updateInterest.

Description

From dForce, we learned that the eventual plan for the system Owner role is to use a smart contract (a multisig or DAO). However, a requirement in RewardDistributor would prevent the onlyOwner method _setDistributionFactors from working in this case.

_setDistributionFactors calls updateDistributionSpeed, which requires that the caller is an EOA:

code/contracts/RewardDistributor.sol:L179-L189

/**
 * @notice Update each iToken's distribution speed according to current global speed
 * @dev Only EOA can call this function
 */
function updateDistributionSpeed() public override {
    require(msg.sender == tx.origin, "only EOA can update speeds");
    require(!paused, "Can not update speeds when paused");

    // Do the actual update
    _updateDistributionSpeed();
}

In the event the Owner role is a smart contract, this statement would necessitate a complicated upgrade to restore full functionality.

Recommendation

Rather than invoking updateDistributionSpeed, have _setDistributionFactors directly call the internal helper _updateDistributionSpeed, which does not require the caller is an EOA.

Description

MSDController._withdrawReserves allows the Owner to mint the difference between an MSD asset’s accumulated debt and earnings:

code/contracts/msd/MSDController.sol:L182-L195

function _withdrawReserves(address _token, uint256 _amount)
    external
    onlyOwner
    onlyMSD(_token)
{
    (uint256 _equity, ) = calcEquity(_token);

    require(_equity >= _amount, "Token do not have enough reserve");

    // Increase the token debt
    msdTokenData[_token].debt = msdTokenData[_token].debt.add(_amount);

    // Directly mint the token to owner
    MSD(_token).mint(owner, _amount);

Debt and earnings are updated each time the asset’s iMSD and MSDS contracts are used for the first time in a given block. Because _withdrawReserves does not force an update to these values, it is possible for the withdrawal amount to be calculated using stale values.

Recommendation

Ensure _withdrawReserves invokes iMSD.updateInterest() and MSDS.updateInterest().

Description

The contracts Base, MSD, and MSDS each have an EIP-2612-style permit function that supports approvals with EIP-712 signatures. We focus this discussion on the Base contract, but the same applies to MSD and MSDS.
When the contract is initialized, the chain ID is queried (with the CHAINID opcode) and becomes part of the DOMAIN_SEPARATOR — a hash of several values which (presumably) don’t change over the lifetime of the contract and that can therefore be computed only once, when the contract is deployed.

code/contracts/TokenBase/Base.sol:L23-L56

function _initialize(
    string memory _name,
    string memory _symbol,
    uint8 _decimals,
    IControllerInterface _controller,
    IInterestRateModelInterface _interestRateModel
) internal virtual {
    controller = _controller;
    interestRateModel = _interestRateModel;
    accrualBlockNumber = block.number;
    borrowIndex = BASE;
    flashloanFeeRatio = 0.0008e18;
    protocolFeeRatio = 0.25e18;
    __Ownable_init();
    __ERC20_init(_name, _symbol, _decimals);
    __ReentrancyGuard_init();

    uint256 chainId;

    assembly {
        chainId := chainid()
    }
    DOMAIN_SEPARATOR = keccak256(
        abi.encode(
            keccak256(
                "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
            ),
            keccak256(bytes(_name)),
            keccak256(bytes("1")),
            chainId,
            address(this)
        )
    );
}

The DOMAIN_SEPARATOR is supposed to prevent replay attacks by providing context for the signature; it is hashed into the digest to be signed.

code/contracts/TokenBase/Base.sol:L589-L610

bytes32 _digest =
    keccak256(
        abi.encodePacked(
            "\x19\x01",
            DOMAIN_SEPARATOR,
            keccak256(
                abi.encode(
                    PERMIT_TYPEHASH,
                    _owner,
                    _spender,
                    _value,
                    _currentNonce,
                    _deadline
                )
            )
        )
    );
address _recoveredAddress = ecrecover(_digest, _v, _r, _s);
require(
    _recoveredAddress != address(0) && _recoveredAddress == _owner,
    "permit: INVALID_SIGNATURE!"
);

The chain ID is not necessarily constant, though. In the event of a chain split, only one of the resulting chains gets to keep the original chain ID and the other will have to use a new one. With the current pattern, a signature will be valid on both chains; if the DOMAIN_SEPARATOR is recomputed for every verification, a signature will only be valid on the chain that keeps the original ID — which is probably the intended behavior.

Remark

The reason why the not necessarily constant chain ID is part of the supposedly constant DOMAIN_SEPARATOR is that EIP-712 predates the introduction of the CHAINID opcode. Originally, it was not possible to query the chain ID via opcode, so it had to be supplied to the constructor of a contract by the deployment script.

Recommendation

An obvious fix is to compute the DOMAIN_SEPARATOR dynamically in permit. However, since a chain split is a relatively unlikely event, it makes sense to compute the DOMAIN_SEPARATOR at deployment/initialization time and then check in permit whether the current chain ID equals the one that went into the DOMAIN_SEPARATOR. If that is true, we proceed as before. If the chain ID has changed, we could (1) just revert, or (2) recompute the DOMAIN_SEPARATOR with the new chain ID. Solution (1) is probably the easiest and most straightforward to implement, but it should be noted that it makes the permit functionality of this contract completely unusable on the new chain.

Description

iETH.receive() requires that the caller is a contract:

code/contracts/iETH.sol:L187-L195

/**
 * @notice receive ETH, used for flashloan repay.
 */
receive() external payable {
    require(
        msg.sender.isContract(),
        "receive: Only can call from a contract!"
    );
}

This method uses the extcodesize of an account to check that the account belongs to a contract. However, contracts currently executing their constructor will have an extcodesize of 0, and will not be able to use this method.

This is unlikely to cause significant issues, but dForce may want to consider supporting this edge case.

Recommendation

Use msg.sender != tx.origin as a more reliable method to detect use by a contract.