lil.eth - Precision Loss Due to Integer Division in _getReserveStatus Function

[sherlock-admin]

lil.eth

medium

Precision Loss Due to Integer Division in _getReserveStatus Function

Summary

The function _getReserveStatus might suffer from precision loss due to the order of arithmetic operations. The integer division of allReserves * valueBase / liabilities is executed before the result is passed to the scaleByBases() function. In Solidity, integer division always rounds down to the nearest whole number, which can lead to a precision loss.

Vulnerability Detail

In the _getReserveStatus function, the division operation allReserves * valueBase / liabilities is executed before the scaleByBases()function is called. This can lead to precision loss due to the fact that Solidity rounds down the result of integer division to the nearest whole number.

This precision loss could be huge in cases where liabilities is much larger than allReserves * valueBase. It may lead to situations where the calculated reserveRatio is lower than it should be, which impact the logic of your contract.

Impact

The _getReserveStatus() is used in functionnalities like swapping tokens when the _checkReserveRatio() so this rounding error could become dangerous for users. Considering that RESERVE_RATIO_BASE and value_Base are expressed in 18 decimals I labelled this as Medium

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/Unitas.sol#L476-L496

    function _getReserveStatus(uint256 allReserves, uint256 liabilities)
        internal
        view
        returns (ReserveStatus reserveStatus, uint256 reserveRatio)
    {
        if (liabilities == 0) {
            reserveStatus = allReserves == 0 ? ReserveStatus.Undefined : ReserveStatus.Infinite;
        } else {
            reserveStatus = ReserveStatus.Finite;

            // All decimals of parameters are the same as USD1
            uint256 valueBase = 10 ** tokenManager.usd1().decimals();
           // @audit-issue rounding error
            reserveRatio = ScalingUtils.scaleByBases(
                allReserves * valueBase / liabilities,
                valueBase,
                tokenManager.RESERVE_RATIO_BASE()
            );
        }
    }

Tool used

Manual Review

Recommendation

To mitigate this issue, the order of operations could be rearranged to ensure that the division operation is performed last. For example, instead of:

reserveRatio = ScalingUtils.scaleByBases(
    allReserves * valueBase / liabilities,
    valueBase,
    tokenManager.RESERVE_RATIO_BASE()
);

Consider using:

reserveRatio = ScalingUtils.scaleByBases(
    allReserves * valueBase,
    valueBase * liabilities,
    tokenManager.RESERVE_RATIO_BASE()
);

This ensures that the division is performed inside the scaleByBases() function, potentially reducing the precision loss.

Duplicate of #152