Core Contracts - GoldLink

The _getPremiums function in the StrategyBank contract calculates the executor premiums for the liquidators. In case the collateral is 0 (e.g.: after adjusting the value for the loan payment), the executorPremium will be 0 and liquidators won't have incentives to liquidate unprofitable strategy accounts.

As a consequence, the StrategyBank contract could have some temporary liquidity issues until those accounts are liquidated.

Code Location

The _getPremiums function doesn't ensure a minimum executorPremium for liquidators:

function _getPremiums(
  uint256 collateral,
  uint256 availableAssets
)
  internal
  view
  returns (uint256 updatedCollateral, uint256 executorPremium)
{
  // Apply the executor premium: the fee earned by the liquidator,
  // calculated as a portion of the liquidated account value.
  //
  // Note that the premiums cannot exceed the collateral that is
  // available in the account.
  executorPremium = Math.min(
    availableAssets.percentToFraction(EXECUTOR_PREMIUM),
    collateral
  );
  updatedCollateral = collateral - executorPremium;

  // Apply the insurance premium: the fee accrued to the insurance fund,
  // calculated as a portion of the liquidated account value.
  updatedCollateral -= Math.min(
    availableAssets.percentToFraction(LIQUIDATION_INSURANCE_PREMIUM),
    updatedCollateral
  );

  return (updatedCollateral, executorPremium);
}

It is recommended to ensure a minimum executor premium for liquidators or make use of liquidation bots, especially for accounts that are liquidatable and cannot repay completely their loans.

Remediation Plan

PENDING: The GoldLink team mentioned that they are working on a solution for this issue and stated the following:

"We are currently in the process of building an open sourced liquidation bot, and we as a company have pledged to liquidate any account regardless of whether or not it is profitable. Luckily, we are deploying on Arbitrum, so gas will be cheap in the worst case."

The health score parameters (LIQUIDATABLE_HEALTH_SCORE and minimumOpenHealthScore_) do not have upper / lower boundaries. As a consequence, if any of them is mistakenly set, it could generate unexpected situations for the borrowing and lending process, e.g.: setting minimumOpenHealthScore_ with a too low value could allow that loans' collateralizations are much less than expected, which would increase the risk of non-payment. The affected functions are the following:

• constructor

• updateMinimumOpenHealthScore

Code Location

The constructor doesn't have upper / lower boundaries for the LIQUIDATABLE_HEALTH_SCORE and minimumOpenHealthScore_ parameters:

constructor(
  address strategyOwner,
  IERC20 strategyAsset,
  IStrategyController strategyController,
  IStrategyReserve strategyReserve,
  BankParameters memory parameters
) Ownable(strategyOwner) ControllerHelpers(strategyController) {
  // Cannot set `minimumOpenHealthScore` at or below `liquidatableHealthScore`.
  require(
    parameters.minimumOpenHealthScore >
      parameters.liquidatableHealthScore,
    Errors
      .STRATEGY_BANK_MINIMUM_OPEN_HEALTH_SCORE_CANNOT_BE_AT_OR_BELOW_LIQUIDATABLE_HEALTH_SCORE
  );

  // Set immutable parameters.
  STRATEGY_ASSET = strategyAsset;
  STRATEGY_RESERVE = strategyReserve;
  INSURANCE_PREMIUM = parameters.insurancePremium;
  LIQUIDATION_INSURANCE_PREMIUM = parameters.liquidationInsurancePremium;
  EXECUTOR_PREMIUM = parameters.executorPremium;
  LIQUIDATABLE_HEALTH_SCORE = parameters.liquidatableHealthScore;
  MINIMUM_COLLATERAL_BALANCE = parameters.minimumCollateralBalance;
  STRATEGY_ACCOUNT_DEPLOYER = parameters.strategyAccountDeployer;

  // Set mutable parameters.
  minimumOpenHealthScore_ = parameters.minimumOpenHealthScore;

  // Set allowance for the `STRATEGY_RESERVE` to take allowed assets when repaying
  // loans.
  STRATEGY_ASSET.approve(address(STRATEGY_RESERVE), type(uint256).max);
}

The updateMinimumOpenHealthScore function doesn't have upper / lower boundaries for the minimumOpenHealthScore_ parameter:

function updateMinimumOpenHealthScore(
  uint256 newMinimumOpenHealthScore
) external onlyOwner {
  // Cannot set `minimumOpenHealthScore_` at or below the liquidation threshold.
  require(
    newMinimumOpenHealthScore > LIQUIDATABLE_HEALTH_SCORE,
    Errors
      .STRATEGY_BANK_MINIMUM_OPEN_HEALTH_SCORE_CANNOT_BE_AT_OR_BELOW_LIQUIDATABLE_HEALTH_SCORE
  );

  // Set new minimum open health score for this strategy bank.
  minimumOpenHealthScore_ = newMinimumOpenHealthScore;

  emit UpdateMinimumOpenHealthScore(newMinimumOpenHealthScore);
}

It is recommended to set upper / lower boundaries for the health score parameters.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The constructor in the StrategyBank contract does not verify if INSURANCE_PREMIUM, LIQUIDATION_INSURANCE_PREMIUM and EXECUTOR_PREMIUM are less than 100%. As a consequence, if any of them is mistakenly set, it could generate that the protocol stops working, e.g.: setting INSURANCE_PREMIUM with a value greater than 100% would make that the transactions revert every time that the balance and accrue interest are synchronized.

Code Location

The constructor does not verify if the premium rates are less than 100%:

constructor(
  address strategyOwner,
  IERC20 strategyAsset,
  IStrategyController strategyController,
  IStrategyReserve strategyReserve,
  BankParameters memory parameters
) Ownable(strategyOwner) ControllerHelpers(strategyController) {
  // Cannot set `minimumOpenHealthScore` at or below `liquidatableHealthScore`.
  require(
    parameters.minimumOpenHealthScore >
      parameters.liquidatableHealthScore,
    Errors
      .STRATEGY_BANK_MINIMUM_OPEN_HEALTH_SCORE_CANNOT_BE_AT_OR_BELOW_LIQUIDATABLE_HEALTH_SCORE
  );

  // Set immutable parameters.
  STRATEGY_ASSET = strategyAsset;
  STRATEGY_RESERVE = strategyReserve;
  INSURANCE_PREMIUM = parameters.insurancePremium;
  LIQUIDATION_INSURANCE_PREMIUM = parameters.liquidationInsurancePremium;
  EXECUTOR_PREMIUM = parameters.executorPremium;
  LIQUIDATABLE_HEALTH_SCORE = parameters.liquidatableHealthScore;
  MINIMUM_COLLATERAL_BALANCE = parameters.minimumCollateralBalance;
  STRATEGY_ACCOUNT_DEPLOYER = parameters.strategyAccountDeployer;

  // Set mutable parameters.
  minimumOpenHealthScore_ = parameters.minimumOpenHealthScore;

  // Set allowance for the `STRATEGY_RESERVE` to take allowed assets when repaying
  // loans.
  STRATEGY_ASSET.approve(address(STRATEGY_RESERVE), type(uint256).max);
}

It is recommended to verify that the premium rates are less than 100% before further processing.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The _updateModel function in the InterestRateModel contract does not verify if optimalUtilization is less than 100%. As a consequence, if this value is mistakenly set too high, the _getInterestRate function will calculate an incorrect interest rate without notifying about the mistaken value in optimalUtilization.

Code Location

The _updateModel function does not verify if optimalUtilization is less than 100%:

function _updateModel(InterestRateModelParameters memory model) internal {
  model_ = model;

  emit ModelUpdated(
      model.optimalUtilization,
      model.baseInterestRate,
      model.rateSlope1,
      model.rateSlope2
  );
}

It is recommended to verify that the optimal utilization parameter is less than 100% before further processing.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The constructor in the StrategyReserve contract does not verify if the attempt to fetch the asset decimals is successful or not, i.e.: boolean return value in the _tryGetAssetDecimals function inherited from the ERC4626 contract.

As a consequence, a failed attempt (e.g., using an asset that has not been created yet) will set the decimals as 18, which could not match the final value for the asset decimals and invalidate all the operations that rely on the decimals value. Furthermore, a failed attempt could constitute a warning about the use of an incorrect asset address.

Code Location

The constructor does not verify if the attempt to fetch the asset decimals is successful or not before further processing:

constructor(
  address strategyOwner,
  IERC20 strategyAsset,
  IStrategyController strategyController,
  IStrategyReserve.ReserveParameters memory reserveParameters,
  IStrategyBank.BankParameters memory bankParameters
)
  Ownable(strategyOwner)
  ERC20(reserveParameters.erc20Name, reserveParameters.erc20Symbol)
  ERC4626(strategyAsset)
  ControllerHelpers(strategyController)
  InterestRateModel(reserveParameters.interestRateModel)
{
  STRATEGY_ASSET = strategyAsset;

  // Create the strategy bank.
  STRATEGY_BANK = new StrategyBank(
    strategyOwner,
    strategyAsset,
    strategyController,
    this,
    bankParameters
  );

  // Set TVL cap for this reserve.
  tvlCap_ = reserveParameters.totalValueLockedCap;
}

It is recommended to verify if the attempt to fetch the asset decimals is successful or not before further processing.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

The repayLoan function in the StrategyBank contract tries to repay part of the loan using the assets in the strategy account. However, it could happen that when trying to repay, there is no enough liquidity in the account (e.g.: assets could have been used to open positions in GMX), so the transaction will revert without showing an accurate description of the root cause of the error.

Code Location

The repayLoan function will revert if there is no enough liquidity in the account without showing the root cause of the error:

// Reduce loan in holdings by total `repayAmount` as the portion of the strategy account
// and potentially a portion of collateral are being transferred to the strategy reserve.
// Since the account is not liquidatable, there is no concern that the `repayAmount`
// will not be fully paid.
holdings.loan -= repayAmount;

// Repay loan portion (`repayAmount`) to strategy reserve.
_repayAssets(
  repayAmount,
  repayAmount,
  strategyAccount,
  collateralRepayment
);

emit RepayLoan(strategyAccount, repayAmount, collateralRepayment);

It is recommended to show accurate descriptions of the root cause of errors, so users can understand them more easily.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.

Some functions in the codebase do not include a zero address check for their parameters. If one of those parameters is mistakenly set to zero, it could affect the correct operation of the protocol. The affected contracts are the following:

• ControllerHelpers

• StrategyReserve

Code Location

The constructor in the ControllerHelpers contract does not have a zero address check for the STRATEGY_CONTROLLER parameter:

constructor(IStrategyController controller) {
  STRATEGY_CONTROLLER = controller;
}

The constructor in the StrategyReserve contract does not have a zero address check for the STRATEGY_ASSET parameter:

constructor(
  address strategyOwner,
  IERC20 strategyAsset,
  IStrategyController strategyController,
  IStrategyReserve.ReserveParameters memory reserveParameters,
  IStrategyBank.BankParameters memory bankParameters
)
  Ownable(strategyOwner)
  ERC20(reserveParameters.erc20Name, reserveParameters.erc20Symbol)
  ERC4626(strategyAsset)
  ControllerHelpers(strategyController)
  InterestRateModel(reserveParameters.interestRateModel)
{
  STRATEGY_ASSET = strategyAsset;

  // Create the strategy bank.
  STRATEGY_BANK = new StrategyBank(
    strategyOwner,
    strategyAsset,
    strategyController,
    this,
    bankParameters
  );

  // Set TVL cap for this reserve.
  tvlCap_ = reserveParameters.totalValueLockedCap;
}

It is recommended to add a zero address check in the functions mentioned above.

Remediation Plan

SOLVED: The GoldLink team solved the issue in the specified commit id.