c4-bot-3 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/PrimaryLiquidationFacet.sol#L229

Vulnerability details

Impact

There is an issue within the min88 function. The problem lies within the conditional statement a < b. Since a is of type uint256 and b is of type uint88, the comparison between them will always be false due to the different integer ranges.

In Solidity, when performing operations between different integer types, the smaller type is implicitly converted to the larger type before the operation is performed. In this case, b (of type uint88) will be converted to a uint256 before being compared with a. However, the maximum value of uint88 (2^88 - 1) is still smaller than the minimum value of uint256 (0), resulting in the comparison a < b being always false.

Proof of Concept

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/PrimaryLiquidationFacet.sol#L229

function min88(uint256 a, uint88 b) private pure returns (uint88) {
        if (a > type(uint88).max) revert Errors.InvalidAmount();
        return a < b ? uint88(a) : b;
    }

Tools Used

Manual

Recommended Mitigation Steps

Cast a to uint88 before the comparison, like this:

function min88(uint256 a, uint88 b) private pure returns (uint88) {
    if (a > type(uint88).max) revert Errors.InvalidAmount();

    return uint88(a) < b ? uint88(a) : b;
}

By casting a to uint88 before the comparison, you ensure that both operands are of the same type, and the comparison will work correctly.

Alternatively, you could also compare a with the maximum value of uint88 before returning the minimum value:

function min88(uint256 a, uint88 b) private pure returns (uint88) {
    if (a > type(uint88).max) revert Errors.InvalidAmount();

    return a > type(uint88).max ? b : uint88(a);
}

In this version, if a is greater than the maximum value of uint88, it returns b. Otherwise, it returns a cast to uint88.

Assessed type

Other

c4-pre-sort commented 6 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

raymondfam marked the issue as duplicate of #22

raymondfam commented 6 months ago

Similar to #22.

c4-judge commented 5 months ago

hansfriese changed the severity to QA (Quality Assurance)