019.md

Kind Aqua Ostrich

Medium

Unchecked increment will cause an overflow in PredictDotLoan.sol

Summary

Unchecked increment of nextLoanId of contract PredictDotLoan.sol could cause an overflow. In Solidity, unchecked arithmetic (i.e., not checking for overflow or underflow) was historically a vulnerability before Solidity 0.8.x, where arithmetic operations would automatically revert in case of overflow/underflow. However, unchecked arithmetic can still be a risk in specific cases or deliberately used for gas optimization.

Root Cause

In the contract, the following piece of code contains an unchecked increment of nextLoanId: https://github.com/sherlock-audit/2024-09-predict-fun/blob/main/predict-dot-loan/contracts/PredictDotLoan.sol#L312-L314 The goal of an unchecked increment is typically to save gas, as checking for overflow costs extra gas. However, if nextLoanId were to reach the maximum value for uint256 (which is very large), the next increment would cause it to wrap around to zero, resulting in a loan ID collision or unexpected behavior. Once nextLoanId wraps around to zero (i.e., it overflows), the next loan issued will have an ID of 0, which could potentially overwrite an existing loan with ID 0 or introduce an incorrect state in the protocol. If nextLoanId is used in mappings or arrays as a unique identifier for loans, an overflow could cause loans to share the same ID, leading to broken logic and potential financial risk. In systems that rely on unique IDs for tracking, an overflow could corrupt data, making it difficult to track or manage loans properly.

Internal pre-conditions

Let`s reffer to first PoC:

1. We initialize nextLoanId to the maximum value (type(uint256).max) to demonstrate the overflow condition in the PoC.
2. The createLoan function increments nextLoanId unchecked, meaning that after the first call, nextLoanId will wrap around to zero.
3. This leads to a collision of loan IDs, which could result in a loan overwriting an existing loan.

External pre-conditions

No response

Attack Path

Let`s reffer to second PoC:

1. After the first createLoan() call, a loan is created with ID 2^256 - 1 (the maximum value for uint256).
2. The second createLoan() call causes the nextLoanId to overflow to 0, and this new loan will overwrite the existing loan with ID 0.
3. When you try to retrieve the loan with ID 0, it will return the details of the second loan, which means that the first loan (with ID 0) has been overwritten or mismanaged.

Impact

1. The primary impact of this vulnerability is that loan IDs could collide due to overflow, leading to existing loans being overwritten. In financial systems, this can cause significant issues in tracking debt obligations, repayments, and liquidations.
2. When loans are overwritten, users might lose their loan data, or the system might incorrectly assume certain loans exist or do not exist. This can result in borrowers losing their collateral or lenders being unable to collect repayments.
3. If an attacker can manipulate the system to force loan ID collisions, they could disrupt the proper functioning of the lending protocol, creating DoS (Denial of Service) attacks or draining funds through incorrect loan operations.
4. As loans are overwritten or mismanaged, the overall state of the protocol becomes corrupt. Auditors and users will find it difficult to rely on the integrity of the system, reducing trust in the protocol.

PoC

FIRST PoC

// Simplified contract snippet showing unchecked increment
contract LoanContract {
    uint256 public nextLoanId = type(uint256).max;  // Start at the max value to force overflow

    mapping(uint256 => Loan) public loans;

    struct Loan {
        address borrower;
        uint256 amount;
        bool active;
    }

    function createLoan(uint256 amount) public {
        unchecked {
            ++nextLoanId;  // Unchecked increment that could overflow
        }

        loans[nextLoanId] = Loan({
            borrower: msg.sender,
            amount: amount,
            active: true
        });
    }

    function getLoan(uint256 loanId) public view returns (Loan memory) {
        return loans[loanId];
    }
}

SECOND PoC

// Deploy the contract and interact with it in a test or using a Solidity script

// Step 1: Create a loan with max loanId
loanContract.createLoan(100);  // Creates loan with ID 2^256 - 1

// Step 2: Call the function again, causing the nextLoanId to overflow to 0
loanContract.createLoan(200);  // Creates loan with ID 0 (overwrites existing loan)

// Step 3: Retrieve the loan with ID 0
Loan memory loan = loanContract.getLoan(0);
console.log("Loan Borrower:", loan.borrower);
console.log("Loan Amount:", loan.amount);  // Displays incorrect or overwritten loan data

Mitigation

The simplest way to prevent this vulnerability is to remove the unchecked block around the nextLoanId increment. Solidity’s default behavior in version 0.8.x will automatically revert if an overflow occurs.

function createLoan(uint256 amount) public {
    nextLoanId++;
    loans[nextLoanId] = Loan({
        borrower: msg.sender,
        amount: amount,
        active: true
    });
}

Although Solidity 0.8.x already checks for overflow by default, explicitly using SafeMath can make the code clearer and ensure that future versions of the compiler are handled safely. Ensure that nextLoanId starts at a reasonable value (e.g., 0 or 1), and avoid setting it to an extremely high value that would approach the uint256 limit. Additionally, perform checks to ensure that nextLoanId does not overflow before each increment. If loan ID uniqueness is critical, consider using a more sophisticated identifier scheme, such as combining the loan ID with a timestamp or address to ensure uniqueness, even if overflow occurs.