Incorrect Comparison Logic in Post-Operation Checks

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-protocol/packages/contracts/contracts/LeverageMacroBase.sol#L277-L289

Vulnerability details

Impact

The _doCheckValueType function, which is crucial for post-operation validation, contains reversed comparison logic for the gte (greater than or equal to) and lte (less than or equal to) operators. This reversal causes the function to perform the opposite checks than intended, potentially allowing operations to pass validation when they should fail, or fail when they should pass.

This issue could lead to:

1. Incorrect validation of CDP (Collateralized Debt Position) states after operations.
2. Potential manipulation of the system by exploiting these misaligned checks.

Proof of Concept

The issue is located in the _doCheckValueType function of the LeverageMacroBase contract:

https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-protocol/packages/contracts/contracts/LeverageMacroBase.sol#L277-L289

Specifically, the problematic comparisons are:

} else if (check.operator == Operator.gte) {
    require(check.value >= valueToCheck, "!LeverageMacroReference: gte post check");
} else if (check.operator == Operator.lte) {
    require(check.value <= valueToCheck, "!LeverageMacroReference: let post check");
}

These comparisons are reversed and should be:

} else if (check.operator == Operator.gte) {
    require(valueToCheck >= check.value, "!LeverageMacroReference: gte post check");
} else if (check.operator == Operator.lte) {
    require(valueToCheck <= check.value, "!LeverageMacroReference: lte post check");
}

Tools Used

manual review

Recommended Mitigation Steps

To fix this issue:

1. Reverse the comparisons in the _doCheckValueType function for the gte and lte operators.
2. Update the function as follows:

function _doCheckValueType(CheckValueAndType memory check, uint256 valueToCheck) internal {
    if (check.operator == Operator.skip) {
        // Early return
        return;
    } else if (check.operator == Operator.gte) {
        require(valueToCheck >= check.value, "!LeverageMacroReference: gte post check");
    } else if (check.operator == Operator.lte) {
        require(valueToCheck <= check.value, "!LeverageMacroReference: lte post check");
    } else if (check.operator == Operator.equal) {
        require(check.value == valueToCheck, "!LeverageMacroReference: equal post check");
    } else {
        revert("Operator not found");
    }
}

Assessed type

Context

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #18

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[GalloDaSballo]

We disagree that the finding is of High Severity and believe it's logically equivalent to a lack of a slippage check, more commonly classified as Medium Severity

Additionally a slippage check is implicitly present when using 0x for swaps

The check is a important check to have but a lack of it working well doesn't open up to meaningful losses due to other safeguards

[alex-ppg]

Hey @GalloDaSballo, thank you for your feedback!

My initial judgment of this submission as high was based on the fact that the code performs an invalid operation and is a sensitive dependency with a high likelihood of vulnerability re-surfacing if left unpatched due to its presence in a utility library. In detail, the impact would be considered medium but the incidence itself merits a "high" rating as it is incorrectly executed every time the function is invoked.

Given that the function is present in a generic library, its impact would vary depending on the library's usage and it is not unreasonable to assume that, if left unpatched, it would lead to a higher severity vulnerability surfacing.

I believe a high-severity rating is justified based on the above analysis, however, I am keen to hear more feedback especially if you believe that any of my statements are incorrect.

[GalloDaSballo]

From our perspective the check is tightly coupled with these parameters: https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-protocol/packages/contracts/contracts/LeverageMacroBase.sol#L207-L229

And was meant to be a generic way to perform checks on those values not to be re-used in other contracts

[alex-ppg]

After closely reviewing the codebase, the function does not appear to be utilized anywhere else except for the LeverageMacroBase contract itself. Based on the fact that the demonstratable vulnerability would be capped at medium severity and the prospective vulnerabilities that can arise from this code flaw are not readily apparent or guaranteed, I will proceed with downgrading this submission to medium risk.

This decision is final and no further feedback is expected in relation to this submission.

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)