howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

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 thus 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");
    }
}

Assessed type

Error

wtj2021 commented 4 months ago

confirmed, fixing in this PR https://github.com/ebtc-protocol/ebtc-zap-router/pull/18

alex-ppg commented 3 months ago

This submission has been identified as very similar to #23, and the contents of both submissions appear highly generic and fail to properly articulate the ramifications of the vulnerability described.

Given that they were generated using the assistance of ChatGPT (or similar generative tools) and did not articulate why the code flaw is a vulnerability, I am inclined to invalidate both submissions as lacking sufficient proof.

c4-judge commented 3 months ago

alex-ppg marked the issue as unsatisfactory: Insufficient proof

code-423n4 / 2024-06-badger-findings