moneyversed - Potential Overflow in _bubbleUp and _bubbleDown functions in Loans.sol

[sherlock-admin]

moneyversed

medium

Potential Overflow in _bubbleUp and _bubbleDown functions in Loans.sol

Summary

The _bubbleUp and _bubbleDown functions in the Loans.sol contract perform arithmetic operations without appropriate checks, which could potentially lead to integer overflow or underflow, and subsequent incorrect calculations or unexpected behavior.

Vulnerability Detail

The _bubbleUp and _bubbleDown functions in Loans.sol perform division on the index_ parameter without validating it, potentially leading to a division by zero error. The index_ variable is also used in multiplication, creating a potential overflow issue.

In the _bubbleUp function, index_ / 2 is calculated without validating that index_ is not zero, potentially leading to a division by zero error.

In the _bubbleDown function, index_ * 2 is calculated without overflow checks, potentially leading to integer overflow if index_ is sufficiently large.

Impact

In the case of the _bubbleUp function, if index_ were to be zero, the function could cause a division by zero error, causing a transaction to fail.

In the case of the _bubbleDown function, if index_ is sufficiently large, the function could cause integer overflow, potentially resulting in an incorrect index being calculated, leading to unexpected behavior, such as manipulation of unexpected memory locations.

Code Snippet

Here are the problematic lines from Loans.sol:

_bubbleUp:

function _bubbleUp(LoansState storage loans_, Loan memory loan_, uint index_) private {
    uint256 count = loans_.loans.length;
    if (index_ == ROOT_INDEX || loan_.thresholdPrice <= loans_.loans[index_ / 2].thresholdPrice){
      _insert(loans_, loan_, index_, count);
    } else {
      _insert(loans_, loans_.loans[index_ / 2], index_, count);
      _bubbleUp(loans_, loan_, index_ / 2);
    }
}

_bubbleDown:

function _bubbleDown(LoansState storage loans_, Loan memory loan_, uint index_) private {

    uint cIndex = index_ * 2;

    uint256 count = loans_.loans.length;
    if (count <= cIndex) {
        _insert(loans_, loan_, index_, count);
    } else {
        Loan memory largestChild = loans_.loans[cIndex];
        ...
    }
}

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/internal/Loans.sol#L134-L142

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/internal/Loans.sol#L150-L171

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, it is recommended to implement checks for these scenarios:

1. For _bubbleUp, check if index_ is not zero before performing division. In solidity, this can be done using require(index_ != 0, "Index cannot be zero");.

2. For _bubbleDown, use a safe multiplication function that checks for overflow before performing the multiplication. This could be a custom function or a library function such as SafeMath.mul.

Proof Of Concept

To reproduce the vulnerability, you can perform the following steps:

1. Deploy the contract with a valid initial setup.
2. Call the function that eventually calls _bubbleUp or _bubbleDown with an index_ of 0 (for _bubbleUp) or a sufficiently large index_ (for _bubbleDown).
3. Observe the division by zero error in _bubbleUp or integer overflow in _bubbleDown.