Upgraded Q -> 3 from #48 [1721695957843]

[c4-judge]

Judge has assessed an item in Issue #48 as 3 risk. The relevant finding follows:

[QA-02] False comparison logic in _doCheckValueType leading to incorrect post-operation checks

Impact

The incorrect comparison logic in the _doCheckValueType function can lead to incorrect validation of post-operation checks. This will result in the ptotocol not properly verifying the expected conditions, thereby, allowing operations to proceed when they should not, or failing valid operations.

Proof of Concept

The _doCheckValueType function is intended to check if a value meets certain conditions based on the operator provided. However, there is a mistake in the comparison logic for the gte and lte operators.

Look at this part of the code: https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-protocol/packages/contracts/contracts/LeverageMacroBase.sol#L277-L290

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

The problem here is with the gte and lte checks. The gte (greater than or equal to) check should ensure that valueToCheck is greater than or equal to check.value, and the lte (less than or equal to) check should ensure that valueToCheck is less than or equal to check.value. However, the current implementation does the opposite.

Recommended Mitigation Steps

The comparison logic should be corrected to ensure that the gte and lte checks are performed correctly:

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

[c4-judge]

alex-ppg marked the issue as duplicate of #18

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-75

[c4-judge]

alex-ppg marked the issue as partial-50

[alex-ppg]

A penalty of 25% has been applied due to a misjudged severity of low-risk, and a further penalty of 25% has been applied due to not properly elaborating on the impact related to post-operation checks.

[c4-judge]

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