Division by Zero Vulnerability in Migration Contract's `convertFromV1ToDemurrage` Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xb7a4471705bda0fd626c68ad3782aa902cc1442e214e8d3e1d401b72bbee6da2 Severity: low

Description: Description

The convertFromV1ToDemurrage function in the Migration contract contains a potential division by zero vulnerability. This function is responsible for converting v1 Circles to demurrage Circles, which is a critical operation in the migration process from v1 to v2 of the Circles protocol.

If the rP variable in the function calculates to zero, it would cause a division by zero error in the final return statement. This would result in the transaction reverting, potentially blocking the migration process for users and disrupting the functionality of the contract. In a worst-case scenario, this could render the migration feature unusable, preventing users from transferring their tokens from v1 to v2 of the protocol.

Proof of Concept and Revised Code

// Vulnerable version
function convertFromV1ToDemurrage(uint256 _amount) public view returns (uint256) {
    uint256 currentPeriod = hubV1.periods();
    uint256 nextPeriod = currentPeriod + 1;

    uint256 startOfPeriod = inflationDayZero + currentPeriod * period;
    uint256 secondsIntoCurrentPeriod = block.timestamp - startOfPeriod;

    uint256 factorCurrentPeriod = hubV1.inflate(ACCURACY, currentPeriod);
    uint256 factorNextPeriod = hubV1.inflate(ACCURACY, nextPeriod);

    uint256 rP =
        factorCurrentPeriod * (period - secondsIntoCurrentPeriod) + factorNextPeriod * secondsIntoCurrentPeriod;

    // Potential division by zero if rP == 0
    return (_amount * 3 * ACCURACY * period) / rP;
}

// Revised version
function convertFromV1ToDemurrage(uint256 _amount) public view returns (uint256) {
    uint256 currentPeriod = hubV1.periods();
    uint256 nextPeriod = currentPeriod + 1;

    uint256 startOfPeriod = inflationDayZero + currentPeriod * period;
    uint256 secondsIntoCurrentPeriod = block.timestamp - startOfPeriod;

    uint256 factorCurrentPeriod = hubV1.inflate(ACCURACY, currentPeriod);
    uint256 factorNextPeriod = hubV1.inflate(ACCURACY, nextPeriod);

    uint256 rP =
        factorCurrentPeriod * (period - secondsIntoCurrentPeriod) + factorNextPeriod * secondsIntoCurrentPeriod;

    // Add a check to prevent division by zero
    require(rP != 0, "Division by zero error: rP is zero");

    return (_amount * 3 * ACCURACY * period) / rP;
}

Explanation of the vulnerability and proposed fix:

1. Vulnerability: In the original function, there's no check to ensure that rP is not zero before performing the division operation. If rP were to be zero, it would cause a division by zero error, reverting the transaction.

2. Fix: Added a require statement to check if rP is zero before performing the division. This will cause the function to revert with a clear error message if a division by zero occurs.

These changes address the immediate vulnerability.

[benjaminbollen]

Thank you for your report on the potential division by zero vulnerability in the Migration Contract's convertFromV1ToDemurrage function. After careful review, we've determined that this is not an issue.

The concern you've raised about the division by the factor 'rP' is understandable. However, 'rP' represents a linear interpolation between two inflation factors from Hub v1. These factors are known to be greater than one and always increasing. As a result, 'rP' can never be zero at any point in the future.

We appreciate your attention to potential vulnerabilities in our mathematical operations. Your diligence in examining our code for edge cases contributes to the overall robustness of our system. Thank you for your efforts in helping ensure the security and stability of our platform.