Prepared by:
HALBORN
Last Updated 06/04/2024
Date of Engagement by: February 26th, 2024 - April 3rd, 2024
88% of all REPORTED Findings have been addressed
All findings
8
Critical
0
High
0
Medium
1
Low
5
Informational
2
GoldLink engaged Halborn to conduct a security assessment on their smart contracts beginning on February 26th, 2024 and ending on April 3rd, 2024. The security assessment was scoped to the smart contracts provided to the Halborn team.
The team at Halborn assigned a full-time security engineer to verify the security of the smart contracts. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.
The purpose of this assessment is to:
Ensure that smart contract functions operate as intended.
Identify potential security issues with the smart contracts.
In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the GoldLink team. The main ones were the following:
Only execute the transfer logic if the amount of assets to be transferred is different to zero.
Ensure a minimum executor premium for liquidators or make use of liquidation bots.
Set upper / lower boundaries for the health score parameters.
Verify that the premium rates and the optimal utilization parameter are less than 100%.
Verify if the attempt to fetch the asset decimals is successful or not before further processing.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:
Research into architecture and purpose.
Smart contract manual code review and walkthrough.
Graphing out functionality and contract logic/connectivity/functions (solgraph).
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.
Manual testing by custom scripts.
Static Analysis of security for scoped contract, and imported functions (slither).
Testnet deployment (Foundry).
| EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
| Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
| Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
| IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
| Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
| Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
| SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
|---|---|---|
| Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
| Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
| Severity | Score Value Range |
|---|---|
| Critical | 9 - 10 |
| High | 7 - 8.9 |
| Medium | 4.5 - 6.9 |
| Low | 2 - 4.4 |
| Informational | 0 - 1.9 |
Critical
0
High
0
Medium
1
Low
5
Informational
2
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| Improper handling of zero transfers for some ERC20 tokens | Medium | Solved - 05/14/2024 |
| Lack of incentives to liquidate unprofitable strategy accounts | Low | Future Release - 03/12/2024 |
| Health score parameters do not have boundaries | Low | Solved - 04/22/2024 |
| Unchecked premiums | Low | Solved - 04/19/2024 |
| Unchecked optimal utilization | Low | Solved - 05/17/2024 |
| Improper handling of failed attempts to fetch asset decimals | Low | Solved - 04/23/2024 |
| Repaying loans could revert without an accurate error message | Informational | Solved - 05/14/2024 |
| Lack of zero address check | Informational | Solved - 04/19/2024 |
// Medium
Some functions don't verify if the amount of assets to be transferred are different from zero. Because there are some ERC20 tokens that reverts when trying to transfer zero tokens (e.g. LEND), it could have three different and independent consequences in the protocol:
repay: The vulnerability could revert a liquidation if returnedLoan is zero (e.g.: in case of a loss) and as a consequence, the liquidation will get stuck indefinitely.
syncAndAccrue: The vulnerability affects all the operations in the StrategyReserve contract that relies on the syncAndAccrue modifier and makes them unusable every time that the interest accrued is zero: updateModel, borrowAssets, repay, settleInterest, deposit, mint, withdraw and redeem. Although the unavailability in this case is temporal, it could affect multiple users and multiple transactions every time the interest accrued is zero, which happens in two different scenarios:
Two or more transactions are executed by a user in the same block.
A function calls other internal functions and the synchronization is executed more than once in the same transaction.
Finally, for StrategyReserve.borrowAssets and StrategyAccount.executeWithdrawErc20Assets, the impact is minor because the vulnerability will impact the users who try to borrow / withdraw zero tokens.
The syncAndAccrue modifier in the StrategyReserve contract doesn't verify if the amount of assets to be transferred is different from zero:
modifier syncAndAccrue() {
// Get the used and total asset amounts.
uint256 used = utilizedAssets_;
uint256 total = used + reserveBalance_; // equal to totalAssets()
// Settle interest that has accrued since the last settlement,
// and get the new amount owed on the utilized asset balance.
uint256 interestOwed = _accrueInterest(used, total);
// Take interest from `StrategyBank`.
//
// Note that in rare cases it is possible for the bank to underpay,
// if it has insufficient collateral available to satisfy the payment.
// In this case, the reserve will simply receive less interest than
// expected.
uint256 interestToPay = STRATEGY_BANK.getInterestAndTakeInsurance(
interestOwed
);
// Update reserve balance with interest to be paid.
reserveBalance_ += interestToPay;
// Transfer interest from the strategy bank. We use the amount
// returned by getInterestAndTakeInsurance() which is guaranteed to
// be less than or equal to the bank's ERC-20 asset balance.
STRATEGY_ASSET.safeTransferFrom(
address(STRATEGY_BANK),
address(this),
interestToPay
);
// Run the function.
_;
}The borrowAssets function in the StrategyReserve contract doesn't verify if the amount of assets to be transferred is different from zero:
function borrowAssets(
address borrower,
uint256 borrowAmount
) external override onlyStrategyBank syncAndAccrue {
// Verify that the amount is available to be borrowed.
require(
availableToBorrow() >= borrowAmount,
Errors.STRATEGY_RESERVE_INSUFFICIENT_AVAILABLE_TO_BORROW
);
// Increase utilized assets and decrease reserve balance.
utilizedAssets_ += borrowAmount;
reserveBalance_ -= borrowAmount;
// Transfer borrowed assets to the borrower.
STRATEGY_ASSET.safeTransfer(borrower, borrowAmount);
emit BorrowAssets(borrowAmount);
}The repay function in the StrategyReserve contract doesn't verify if the amount of assets to be transferred is different from zero:
function repay(
uint256 initialLoan,
uint256 returnedLoan
) external onlyStrategyBank syncAndAccrue {
// Reduce utilized assets by assets no longer borrowed and increase
// reserve balance by the amount being returned, net of loan loss.
utilizedAssets_ -= initialLoan;
reserveBalance_ += returnedLoan;
// Effectuate the transfer of the returned amount.
STRATEGY_ASSET.safeTransferFrom(
address(STRATEGY_BANK),
address(this),
returnedLoan
);
emit Repay(initialLoan, returnedLoan);
}The executeWithdrawErc20Assets function in the StrategyAccount contract doesn't verify if the amount of assets to be transferred is different from zero:
function executeWithdrawErc20Assets(
address receiever,
IERC20[] calldata tokens,
uint256[] calldata amounts
) external onlyOwner strategyNonReentrant whenNotLiquidating noActiveLoan {
uint256 n = tokens.length;
require(
amounts.length == n,
Errors.STRATEGY_ACCOUNT_PARAMETERS_LENGTH_MISMATCH
);
for (uint256 i; i < n; ++i) {
tokens[i].safeTransfer(receiever, amounts[i]);
emit WithdrawErc20Asset(receiever, tokens[i], amounts[i]);
}
}Foundry test that shows 2 different scenarios:
Depositing 500 DAI tokens in the StrategyReserve contract will be a successful operation, as expected.
Depositing 500 LEND tokens in the StrategyReserve contract won't be a successful operation because this kind of token reverts when transferring 0 tokens at some point of the whole transaction.
function testDepositingTokens() public {
IERC20 dai = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);
IERC20 lend = IERC20(0x80fB784B7eD66730e8b1DBd9820aFD29931aab03);
IStrategyReserve.ReserveParameters memory reserveParameters = TestConstants.defaultReserveParameters();
StrategyAccountDeployerMock strategyAccountDeployerMock = new StrategyAccountDeployerMock();
IStrategyBank.BankParameters memory bankParameters = TestUtilities.defaultBankParameters(
IStrategyAccountDeployer(address(strategyAccountDeployerMock))
);
// Operations with DAI token
StrategyController strControllerDai = new StrategyController(msg.sender, dai, reserveParameters, bankParameters);
IStrategyReserve strReserveDai = strControllerDai.STRATEGY_RESERVE();
deal(address(dai), msg.sender, 5*1e6*1e18, true);
vm.prank(msg.sender);
dai.approve(address(strReserveDai), 500);
vm.prank(msg.sender);
strReserveDai.deposit(500, msg.sender); // Depositing 500 DAI
// Operations with LEND token
StrategyController strControllerLend = new StrategyController(msg.sender, lend, reserveParameters, bankParameters);
IStrategyReserve strReserveLend = strControllerLend.STRATEGY_RESERVE();
deal(address(lend), msg.sender, 5*1e6*1e18, true);
vm.prank(msg.sender);
lend.approve(address(strReserveLend), 500);
vm.expectRevert();
vm.prank(msg.sender);
strReserveLend.deposit(500, msg.sender); // Depositing 500 LEND
}The result of the test is the following:
Verify the amount of assets to be transferred and only execute the transfer logic if this amount is different from zero.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Low
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.
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.
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."
// Low
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
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Low
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.
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Low
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.
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Low
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.
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Informational
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.
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
// Informational
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
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.
SOLVED: The GoldLink team solved the issue in the specified commit id.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed