GFX Interest Protocol

While the tokens that will be used as collateral are added via governance and are thus expected to be compatible, the protocol does not support tokens deviating from the ERC20 standard by not returning a boolean success value using returndata (USDT is the most popular such token). We suggest the use of the OZ SafeERC20 wrapper library to support both standard and non-standard token implementations.

In VaultController::repayAllUSDi, the following computation takes place to calculate the USDi liability:

function repayAllUSDi(uint96 id) external override paysInterest whenNotPaused {
    // grab the vault by id if part of our system. revert if not
    IVault vault = getVault(id);
    // get the total usdi liability, equal to the interest factor * vault's base liabilty
    // Dedaub: Truncation should happen before the safeu192 call, to prevent any edge-case overflows.
    uint256 usdi_liability = truncate(safeu192(_interest.factor * vault.baseLiability()));
    ...
}

However, in this case, truncation takes place after the safeu192 conversion, which could cause this computation to revert in some edge cases. It’s recommended that the operation order is flipped.

In VaultController::liquidateAccount the asset to be liquidated is not explicitly required to be registered, though this is implied when requesting its current price from oracleMaster:

/// VaultController::_liquidationMath
uint256 price = _oracleMaster.getLivePrice(asset_address);

/// OracleMaster::getLivePrice
require(_relays[token_address] != address(0x0), "token not enabled");

It is expected that only relays for the registered tokens are deployed, so the above instruction should typically fail in case of an unregistered asset_address. However, there is no guarantee that the available relays are always in sync with Vaultcontroller’s registered tokens.

There is an edge case where the above omission could allow an attacker to drain any amount of unregistered token from a vault (probably transferred to the vault by mistake): \

• Assume an underwater vault V that has a non-zero balance of token A. \
• Assume that A is unregistered but there are relays deployed so that oracleMaster does not fail and returns A’s price \
• The vault’s minter cannot withdraw A because V is underwater \
• Now an attacker liquidator can choose to liquidate V as for asset A. The attacker can drain the whole vault’s balance of A, since it is not contributing to the vault’s borrowing power.

We suggest checking for registered asset_address in liquidateAccount() as a best practice. We also suggest considering allowing withdrawals of unregistered tokens even if the vault is underwater.

In the edge case where a registered asset’s price falls to zero, oracleMaster::getLivePrice reverts:

function getLivePrice(address token_address) external view override returns (uint256) {
  require(_paused[token_address] == false, "relay paused");
  require(_relays[token_address] != address(0x0), "token not enabled");
  IOracleRelay relay = IOracleRelay(_relays[token_address]);
  uint256 value = relay.currentValue();
  require(value != 0, "value is 0"); //Dedaub: reverts on 0 price
  return value;
}

However, VaultController::get_vault_borrowing_power is not consistent to this design since it would simply omit a token in case of zero price:

function get_vault_borrowing_power(IVault vault) private view returns (uint192) {
  uint192 total_liquidity_value = 0;
  for (uint192 i = 1; i <= _tokensRegistered; i++) {
    // ...
    uint256 balance = vault.tokenBalance(token_address);
      if (balance == 0) {
        continue;
      }

    uint192 raw_price = safeu192(_oracleMaster.getLivePrice(token_address));
    // Dedaub: zero prive would revert in oracleMaster
    if (raw_price == 0) {
      continue;
    }
    // ...
  }
  return total_liquidity_value;
}

Therefore, in such an extreme case, vaults that involve a problematic asset A, would practically paralyze since a vault’s functionality relies on the calculation of its borrowing power. What is more, minters whose vaults do not hold amounts of A but are close to underwater, have incentives to transfer an amount of A to their vault so as to avoid getting liquidated.

One possible solution would be to remove the problematic asset from the registered assets list, however that whitelist is designed to be append-only (_tokenAddress_tokenId). The team informed us that in practice, governance can implicitly remove an asset from the platform by setting its collateral status parameter to 0. VaultController::get_vault_borrowing_power should check upon this parameter’s value in order to overcome such problematic situations.

The core contract of the protocol, VaultController, maintains a list of all tokens that can be used as collateral. While this list contains all currently enabled tokens, it’s append only, meaning that if governance decides in the future that a token will no longer be supported as collateral (by setting its LTV, liquidation incentive and oracle address to zero), the code will still loop over such tokens, leading to wasted gas.

This issue is prominent in most interactions with a Vault, because a loop over all registered collateral tokens is required (leading to some storage accesses for each) in order to calculate its borrowing power:

function get_vault_borrowing_power(IVault vault) private view returns (uint192) {
    uint192 total_liquidity_value = 0;

  //Dedaub: if tokensRegistered grows long, gas cost would increase significantly
  //Dedaub:consider depositing through vault and keep track of each vault's assets
    for (uint192 i = 1; i <= _tokensRegistered; i++) {
      address token_address = _enabledTokens[i - 1];
      uint256 balance = vault.tokenBalance(token_address);
      if (balance == 0) {
        continue;
      }
      // ...
    }
    return total_liquidity_value;
}

Each Vault will most probably hold amounts of only a small subset of the registered tokens. Consequently, the gas costs for interacting with a Vault increases as more tokens are being registered. From a security standpoint, this could even lead to a DoS issue in case the list of registered tokens grows significantly. The fact that registered tokens cannot be completely removed from the list, as per the current design, makes this issue even more concerning.

The following two items are highly recommended and should be considered by the protocol developers, as it would allow for a much more scalable design system (in terms of gas cost):

• Prune the enabledTokens list whenever a token is deprecated
• Maintain a per-account token list, instead of a global one – the average user will only deposit a subset of the supported tokens as collateral.

In UFragments::permit(owner, ...) one can issue arbitrary permits if owner is the null address. This is because the precompiled ecrecover returns zero if the message signature is invalid:

function permit(owner, ...) {
    // Dedaub: This check is trivial to pass with owner == address(0x0)
    require(owner == ecrecover(digest, v, r, s));
    ...
}

While this behavior is really inconsequential to the rest of the protocol (as it is impossible for the null-address to hold any USDi in the current implementation), it is definitely weird and could become an issue in future versions of the protocol.

We highly recommend that an additional require is added to verify that ecrecover terminated successfully, by simply checking that its return value is non-zero.

In TokenDelegate.sol there is a wrong event emitted upon changing the token’s symbol:

 function changeSymbol(string calldata symbol_) external override onlyOwner {
    require(bytes(symbol_).length > 0, "changeSymbol: length invaild");
    // Dedaub: wrong event emitted, should be "ChangedSymbol"
    emit ChangedName(symbol, symbol_);
    symbol = symbol_;
  }

In GovernorDelegate.sol functions add256/sub256 are used for protection of overflows/undeflows. However, the solidity compiler of versions ^0.8.0 takes care of these cases automatically. We suggest removing these two functions.

In GovernorDelegate.sol there are a number of functions that are accessible only via the contract itself as a result of governance proposal execution. The access restriction is guaranteed by the following requirement:

require(_msgSender() == address(this), "must come from the gov.");

We suggest turning this to a modifier to avoid code duplication but also improve readability.

1. In GovernorDelegate.sol, there is an outdated comment:

  /**
   * @notice Used to initialize the contract during delegator contructor
   * @param ipt_ The address of the COMP token // Dedaub: should be IPT token
   * @param votingPeriod_ The initial voting period
   * @param votingDelay_ The initial voting delay
   * @param proposalThreshold_ The initial proposal threshold
   * @param proposalTimelockDelay_ The initial proposal holding period
   */
  function initialize(...) {...}

2. In VaultController.sol, there are two typos in two comments referring to LTV:

function checkAccount(...) {
   ...
   // if the TLV >= liability, the vault is solvent // Dedaub: should be LTV
   return (total_liquidity_value >= usdi_liability);
}

function get_vault_borrowing_power(...) {
   ...
      // increase the TLV of the vault by the token value // Dedaub: should be LTV
      total_liquidity_value = total_liquidity_value + token_value;
   ...
}

We recommend updating these comments for readability.

The current implementation of CurveMaster supports the registration of multiple interest rate curves, as it maintains a mapping from token addresses to curves. However, this generality is unnecessary as the protocol always invokes the CurveMaster functions with the zero address as an argument, which makes sense as only it only needs a single global interest curve.

It is recommended that the code be simplified so that it only keeps track of a single curve, in order to improve readability and reduce gas consumption.

In the whitepaper, it is mentioned that the interest curve is decreasing. However, the no checks are performed in the curve constructor to ensure that these properties hold:

constructor(
    int256 r0,
    int256 r1,
    int256 r2,
    int256 s1,
    int256 s2
) {
    // Dedaub: No sanity checks on the curve parameters
    _r0 = r0;
    _r1 = r1;
    _r2 = r2;
    _s1 = s1;
    _s2 = s2;
}

It is recommended that some basic sanity checks are done to ensure that the parameters of the curve satisfy the property. The checks could include:

• Checking that 0 < r2 < r1 < r0
• Checking that 0 < s1 < s2 < 1e18

In Vault::withdrawErc20, there a transferFrom operation takes place:

  function withdrawErc20(address token_address, uint256 amount) external override onlyMinter {
	// transfer the token to the owner
	// Dedaub: Can be simply IERC20(...).transfer(...)
	IERC20(token_address).transferFrom(address(this), _msgSender(), amount);
	//  check if the account is solvent
	bool solvency = _master.checkAccount(_vaultInfo.id);
	require(solvency, "over-withdrawal");

	emit Withdraw(token_address, amount);
  }

However, since from=address(this), the operation can be replaced by the simpler transfer.

In VaultController::repayUSDi constant variable ‘expScale’ could be used instead of hardcoding value 1e18:

function repayUSDi(uint96 id, uint192 amount) external override paysInterest whenNotPaused {
    // ...
    // Dedaub: use 'expScale' instead of 1e18 for consistency
    uint192 base_amount = safeu192((amount * 1e18) / _interest.factor);
   // ...
}

Function GovernorDelegate::executeTransaction returns the data of a proposal’s transaction execution but is never used.

In Vault.sol variables _vaultInfo and _master are set during the contract’s construction and cannot be altered, thus they can be declared immutable for readability and gas savings.

In VaultControllers the following fields are redundantly declared public, as getter functions are defined explicitly in the code:

// Dedaub: No need to be public - getters defined explicitly in the code
uint256 public _tokensRegistered;
uint192 public _totalBaseLiability;
uint192 public _protocolFee;

In UFragments.sol variables rebasePausedDeprecated and tokenPausedDeprecated are declared and initialized but never used.

In VaultController’s updateRegisteredErc20 and registerErc20 functions, the parameters of the specified collateral token are written to storage verbatim, including its LTV and liquidation incentive. However, not all (LTV, liquidation incentive) pairs are compatible. More specifically, the following calculation takes place during liquidations:

function _liquidationMath(...) {
    uint256 badFillPrice = truncate(price * (1e18 - _tokenAddress_liquidationIncentive[asset_address]));

    // the ltv discount is the amount of collateral value that one token provides
    uint256 ltvDiscount = truncate(price * _tokenId_tokenLTV[_tokenAddress_tokenId[asset_address]]);
    // this number is the denominator when calculating the max_tokens_to_liquidate
    // it is simply the badFillPrice - ltvDiscount
    // Dedaub: This implies that the LTV must be < (1 - liquidation incentive) .
    //         Otherwise, the calculation would underflow.
    uint256 denominator = badFillPrice - ltvDiscount;
    // [...]
}

It is recommended that a check be added in both updateRegisteredErc20 and registerErc20 to verify that LTV < (1 - Liquidation Incentive).

In VaultController::borrowUsdi, the following calculation takes place:

function borrowUsdi(uint96 id, uint192 amount){
    uint192 base_amount = safeu192(uint256(amount * expScale) / uint256(_interest.factor));
    // Dedaub: Redundant -- this is simply 'amount'
    uint256 usdi_amount =  truncate(uint256(_interest.factor) * base_amount);
    // [...]
}

The calculation of usdi_amount is redundant and its uses can be replaced with amount.

In VaultController::changeProtocolFee, an event is emitted

function changeProtocolFee(uint192 new_protocol_fee) external override onlyOwner {
    require(new_protocol_fee < 1e18, "fee is too large");
    _protocolFee = new_protocol_fee;
    // Dedaub: Emit new_protocol_fee to save some gas
    emit NewProtocolFee(_protocolFee);
}

Gas savings can be achieved by emitting new_protocol_fee, instead of _protocolFee which is a storage field.

In both ChainlinkOracleRelay and UniswapV3OracleRelay, all storage fields can be declared as immutable, as they are only initialized in the constructor and never written to again.

Furthermore the following fields are unused and can be removed:

• ChainlinkOracleRelay::_feedAddress
• UniswapV3OracleRelay::_poolAddress

The floating pragma ^0.8.0 is used in the contracts, allowing them to be compiled with any version 0.8.x of the Solidity compiler. Although the differences between these versions are small, floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment.

Solidity compiler version v0.8.9 has, at the time of writing, some known bugs. We inspected the code and found that it is not affected by these bugs.