Finance Contracts - Bonzo

During the _executeBorrow function in the LendingPool contract, there is a potential issue when releaseUnderlying is set to true. If the underlying asset is the wrapped HBAR ( _whbarToken ), the unwrapping process should be carefully managed. Currently, the code unwraps and sends the HBAR to vars.onBehalfOf, which can lead to incorrect handling and unnecessary complexity.

The unwrapping should only occur when releaseUnderlying is true, and the unwrapped HBAR should be sent to vars.user, who is the one receiving the underlying asset through transferUnderlyingTo. This ensures that the correct user receives the unwrapped HBAR and avoids any confusion or double payments.

Modify the _executeBorrow function to handle the unwrapping of _whbarToken properly. Ensure that the unwrapping only occurs when releaseUnderlying is true and that the unwrapped HBAR is sent to vars.user.

if (vars.releaseUnderlying) {
    if (vars.asset == address(_whbarToken)) {
        IAToken(vars.aTokenAddress).transferUnderlyingTo(address(this), vars.amount);
        IWHBAR(_whbarContract).withdraw(vars.user, vars.amount);
    } else {
        IAToken(vars.aTokenAddress).transferUnderlyingTo(vars.user, vars.amount);
    }
}

By implementing this change, the borrowing process will correctly handle the unwrapping of _whbarToken, ensuring that the appropriate user receives the unwrapped HBAR and preventing any potential issues related to asset handling during borrowing.

SOLVED: The suggestion was applied by the Bonzo team.

A critical bug related to the stable borrow rate was reported on November 4, 2023. This bug affects certain assets due to their configuration and poses a significant risk. Given the nature of the bug and the progressive deprecation of the stable rate in Aave v2 and v3, it was decided to fully deprecate the stable rate mode. The fix involved halting new borrowings in stable mode, as well as stopping rebalancing and swapping from variable to stable. Although we are confident that there is no further vector for the bug, the current situation creates significant technical overhead and complicates security evaluations. Furthermore, disclosing the attack vector is not safe until all user stable debt positions are closed.

https://governance.aave.com/t/bgd-full-deprecation-of-stable-rate/16473

To mitigate this issue, it is recommended to update the swapBorrowRateMode function in the Aave Pool smart contract on both Aave v2 and v3 to make the swap from stable to variable rate permission-less. This will allow any address to trigger the rate swap, enabling the Aave DAO to automate the process of moving all current stable debt positions to variable without penalizing users. Once all positions are in variable mode, the stable rate mode can be fully deprecated.

SOLVED: The suggestion was applied by the Bonzo team.

The AToken contract is susceptible to price inflation due to liquidity index manipulation and rounding issues in the mint and burn functions. This vulnerability can be exploited to extract profit, particularly in scenarios where the initial pool balance is not properly initialized. While Aave v2 deployment scripts typically initialize the pool with an initial balance to prevent the First Deposit Bug, this is not always guaranteed. Similar issues have been observed in other protocols, such as Compound and Radiant Lending Pool.

1. Initialize Pool with Initial Balance:

Ensure that the pool is initialized with a small balance to prevent the First Deposit Bug. This can be achieved by minting a small number of tokens to an unused address (e.g., the zero address).

Reference:

- Compound Initial Mint Fix

- Uniswap V2 Mint Fix

Code Snippet:

    if (totalSupply == 0) {
        totalSupply = 1000;
        accountTokens[address(0)] = 1000;
        mintTokens -= 1000;
    }

2. Ensure Non-zero Shares Minted:

When minting tokens, ensure that the number of shares minted is non-zero to prevent rounding issues.

Code Snippet:

   require(_shares != 0, "zero shares minted");

3. Create Periphery Contract for Initialization:

Create a periphery contract that contains a wrapper function to atomically call initialize and deposit. This ensures that the initialization and first deposit are handled in a single transaction.

By implementing these changes, the risk of price inflation and associated exploits can be significantly reduced, enhancing the overall security and stability of the AToken contract.

References:

- Hopelend Incident Analysis

- Radiant Lending Pool Hack Analysis

- Compound Initial Mint Fix

- Uniswap V2 Mint Fix

SOLVED: A periphery contract was added, named LendingPoolPeriphery that initializes the pool as soon as it is deployed.

In the LendingPoolConfigurator contract, the setDecimals function allows changing the number of decimals for a reserve asset. This can lead to critical issues if the new decimals do not match the underlying asset's actual decimals. The core functions getAmountInEth and _executeBorrow assume the decimals correspond to the underlying asset, which impacts calculations involving collateral and borrowing.

Assume:

• Original decimals of the underlying asset: D1

• New decimals set via setDecimals: D2

• Amount of asset held by a user: A

• Price of the asset in ETH: P

Value in ETH with original decimals:

V1 = (A*P)/10^{D1}

Value in ETH with new decimals:

V2 = (A*P)/10^{D2}

Impact:

• If D2 > D1, V2 < V1: The collateral value in ETH is underestimated, leading to potential under-collateralization and allowing excessive borrowing.

• If D2 < D1, V2 > V1: The collateral value in ETH is overestimated, restricting borrowing more than necessary.

Changing the decimals without ensuring they match the underlying asset's decimals can cause:

• Underestimation or overestimation of collateral value.

• Discrepancies in borrowing calculations.

• Potential liquidation or over-leverage scenarios for users.

This demonstrates the critical issues that arise from mismatched decimals, emphasizing the need for consistent handling of asset decimals across the smart contract system.

1. Remove the setDecimals Functionality: Consider removing the ability to change the decimals of a reserve asset to maintain consistency and avoid critical issues related to collateral and borrowing calculations.

2. Restrict Decimals Change: If retaining the functionality, restrict changes to only when there are no supplies or borrows in the pool. Ensure that the new decimals match the underlying asset's actual decimals by performing a validation check before applying the change.

These measures will help maintain the integrity of the system and prevent discrepancies in collateral and borrowing values.

SOLVED: The setDecimals function was removed.

In the Ownable contracts, such as SupraOracle and others, the current ownership transfer mechanism might be insecure. A two-step ownership transfer process is recommended to prevent accidental transfer or loss of contract control. Additionally, the renounceOwnership function should be overridden to revert the transaction, avoiding the possibility of leaving the contract in an unusable state without an owner.

Implement a two-step ownership transfer process by introducing a pendingOwner state. Also, override the renounceOwnership function to revert the transaction.

SOLVED: The issue was solved.

The LendingPool contract currently uses two storage variables, _whbarContract and _whbarToken, to manage wrapped HBAR (WHBAR). These variables could be optimized by storing the WHBAR contract address as an immutable variable and using address(0) to represent native HBAR. This would reduce storage usage and simplify the code by eliminating the need for redundant variables.

Additionally, the current implementation uses testnet addresses for _whbarContract and _whbarToken. Using immutable variables set during contract deployment can help avoid errors and make the contract more secure and efficient.

1. Use an immutable variable for _whbarContract.

2. Use address(0) to represent the WHBAR token, removing the need for _whbarToken.

3. Update the contract deployment process to set the immutable _whbarContract variable.

This approach enhances the contract's efficiency and security by leveraging Solidity's immutable variables and simplifying the handling of wrapped HBAR.

ACKNOWLEDGED: Ignoring, because ValidationLogic.sol functions need to have an actual reserve for the asset address

There are multiple instances of the HederaTokenService contract within the codebase, one in the misc directory and the other in the tokenWrapper directory. These contracts contain duplicated functions such as createFungibleToken, mintToken, burnToken, associateTokens, and transferToken. Additionally, the misc directory contains a unique function, transferTokenRouter, which is essentially the same as transferToken but uses delegatecall.

Moreover, the HederaResponseCodes contract is duplicated under both the misc/HBAR and tokenWrapper directories. The one in the misc directory only defines the UNKNOWN and SUCCESS error codes, while the tokenWrapper directory one defines 330 errors.

Consider merging the functionalities of the HederaTokenService contracts into a single contract that encompasses all required functionalities. If certain functionalities are not being used, consider removing them to reduce complexity and potential security risks.

For HederaResponseCodes, consolidate the error codes into a single contract to avoid redundancy and potential inconsistencies. Use the more comprehensive set of error codes defined in the tokenWrapper directory.

SOLVED: The issue was solved.

The redirectForToken function in the HederaTokenService contract has inconsistent return handling between its original version and the version in scope. The original version returns (HederaResponseCodes.SUCCESS, result), while the in-scope version decodes the result as (int32, bytes). This inconsistency can lead to unexpected behavior and potential security vulnerabilities due to different interpretations of the return values.

Standardize the return handling of the redirectForToken function to ensure consistent behavior. Update the in-scope version to match the original version's return format if that is the intended standard. Ensure thorough testing to verify that all functions correctly handle the return values.

SOLVED: The issue was solved by removing the unused HederaTokenService.

The getDecimals function, which retrieves the decimal configuration for a given asset, is currently implemented in the LendingPoolConfigurator contract. This design requires unnecessary contract interaction and complicates the retrieval process. Implementing this function directly in the LendingPool contract would streamline the process, making it more efficient and intuitive.

Move the getDecimals function to the LendingPool contract to enhance the design and improve efficiency.

// In the LendingPool contract
function getDecimals(address asset) external view returns (uint8) {
    DataTypes.ReserveData storage reserve = _reserves[asset];
    return reserve.configuration.getDecimals();
}

This change ensures that the decimal information for a given asset can be accessed directly from the LendingPool contract, simplifying the codebase and reducing the need for cross-contract calls.

SOLVED: The issue was solved.

The current implementation of the AToken contract does not leverage the Hedera Token Service (HTS) for token transfers, which can lead to inefficiencies and additional complexity in the code. By extending from the IHRC as specified in HIP-719, the AToken can use the HTS transfer mechanism. This approach allows users to associate themselves with the AToken, enabling more efficient and secure transfers, including mint and burn operations.

Users will need to call the LendingPool's AToken association function before receiving tokens. This change requires comprehensive testing and validation to ensure all liquidity rate updates and AToken balances are correctly handled.

Extend the AToken contract from IHRC as specified in HIP-719. Update all internal transfers, including mint and burn, to use the HTS transfer mechanism. Ensure that users associate themselves with the AToken before receiving tokens. All transfers should use the "safe" variant to check for Hedera success codes.

ACKNOWLEDGED: Ignoring it because we have taken a conscious business decision NOT to make the aTokens as HTS tokens in order to avoid their trading on DEXes.