AMO2::setRebalanceUpThreshold and AMO2::setRebalanceDownThreshold should check that invariant REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD holds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L203-L225
https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L495-L504
https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/AMO2.sol#L512-L521

Vulnerability details

Description

Functions AMO2::setRebalanceUpThreshold and AMO2::setRebalanceDownThreshold allows setting any value to REBALANCE_DOWN_THRESHOLD and REBALANCE_UP_THRESHOLD. This allows to break REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD invariant

Impact

Invariant REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD can be broken leading to unwanted protocol state which can DOS AMO2::rebalanceUp or AMO2::rebalanceDown, depending on the new values.

POC

Current functions AMO2::setRebalanceUpThreshold and AMO2::setRebalanceDownThreshold allow setting any value to REBALANCE_DOWN_THRESHOLD and REBALANCE_UP_THRESHOLD

    function setRebalanceUpThreshold(
        uint256 newRebalanceUpThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceUpThreshold(
            REBALANCE_UP_THRESHOLD,
            newRebalanceUpThreshold
        );
        // @audit there is no check that REBALANCE_DOWN_THRESHOLD < newRebalanceUpThreshold
        REBALANCE_UP_THRESHOLD = newRebalanceUpThreshold;
    }

    function setRebalanceDownThreshold(
        uint256 newRebalanceDownThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceDownThreshold(
            REBALANCE_DOWN_THRESHOLD,
            newRebalanceDownThreshold
        );
        // @audit there is no check that newRebalanceDownThreshold < REBALANCE_UP_THRESHOLD
        REBALANCE_DOWN_THRESHOLD = newRebalanceDownThreshold;
    }

The lack of limitation to hold invariant REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD would break AMO2::preRebalanceCheck behavior in case of bad assignment of values:

// AMO2::preRebalanceCheck
// @audit If REBALANCE_DOWN_THRESHOLD > REBALANCE_UP_THRESHOLD in case that the admin decide to change first REBALANCE_DOWN_THRESHOLD and later REBALANCE_UP_THRESHOLD would force an unwanted rebalance up
if (xEthPct > REBALANCE_UP_THRESHOLD) {
    isRebalanceUp = true;
}
/// @notice if the ratio is below the lower threshold, rebalanceDown() will be called
/// @notice possible gas optimization here.
else if (xEthPct < REBALANCE_DOWN_THRESHOLD) {
    isRebalanceUp = false;
}

It must be remarked tha the rebalance, according to xETH documentation is handle by a bot: The amounts of lpBurn and xETH mint comes from an offchain bot called defender.

This can eventually lead to next scenario:

1. Context:
   • Currently REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD
   • Currently REBALANCE_UP_THRESHOLD < xEthPct, for some reason this situation is not changing (market conditions) so the admin wants to increase REBALANCE_DOWN_THRESHOLD as well as REBALANCE_UP_THRESHOLD to actually rebalance down: This would mean that $REBALANCE_ UP_ THRESHOLD_{now} &lt; REBALANCE_ DOWN_ THRESHOLD_{new}$
2. The admin want to increase REBALANCE_UP_THRESHOLD and REBALANCE_DOWN_THRESHOLD, for this reason he decide to call AMO2::setRebalanceDownThreshold and AMO2::setRebalanceUpThreshold. Consider that the new REBALANCE_DOWN_THRESHOLD is greater than the current REBALANCE_UP_THRESHOLD
3. While AMO2::setRebalanceDownThreshold is correctly executed, AMO2::setRebalanceUpThreshold is reverted. The admin for some reason do not notice this
4. Until the admin successfully calls AMO2::setRebalanceUpThreshold will still be called by the defender (offchain bot), leading to an unwanted state (only rebalance up can occur)

Mitigation steps

Add the corresponding checks to only allow valid values. This means:

    function setRebalanceDownThreshold(
        uint256 newRebalanceDownThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceDownThreshold(
            REBALANCE_DOWN_THRESHOLD,
            newRebalanceDownThreshold
        );

+       if(newRebalanceDownThreshold >= REBALANCE_UP_THRESHOLD){
+           revert ERROR_REBALANCE_THRESHOLD_INVARIANT_WILL_BREAK_WITH_THIS_ASSIGNMENT();
+       }
        REBALANCE_DOWN_THRESHOLD = newRebalanceDownThreshold;
    }

    function setRebalanceUpThreshold(
        uint256 newRebalanceUpThreshold
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetRebalanceUpThreshold(
            REBALANCE_UP_THRESHOLD,
            newRebalanceUpThreshold
        );
+       if(newRebalanceUpThreshold <= REBALANCE_DOWN_THRESHOLD){
+           revert ERROR_REBALANCE_THRESHOLD_INVARIANT_WILL_BREAK_WITH_THIS_ASSIGNMENT();
+       }
        REBALANCE_UP_THRESHOLD = newRebalanceUpThreshold;
    }

Assessed type

Invalid Validation

[c4-judge]

kirk-baird marked the issue as primary issue

[d3e4]

Since only xEthPct > REBALANCE_UP_THRESHOLD is checked if it holds, this effectively caps REBALANCE_DOWN_THRESHOLD at REBALANCE_UP_THRESHOLD in how isRebalanceUp is determined.

Note that the same issue has been reported as a QA in #11 and #36.

[carlitox477]

#36 cannot be a dup given that the invariant is clearly REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD

[d3e4]

#36 cannot be a dup given that the invariant is clearly REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD

That’s precisely what it says in #36. Or what do you mean?

[carlitox477]

#36 cannot be a dup given that the invariant is clearly REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD

That’s precisely what it says in #36. Or what do you mean?

No, it says REBALANCE_DOWN_THRESHOLD <= REBALANCE_UP_THRESHOLD

[d3e4]

#36 cannot be a dup given that the invariant is clearly REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD

That’s precisely what it says in #36. Or what do you mean?

No, it says REBALANCE_DOWN_THRESHOLD <= REBALANCE_UP_THRESHOLD

It also mentions that < is a possibility. But that distinction makes little practical difference.

[c4-sponsor]

vaporkane marked the issue as disagree with severity

[c4-sponsor]

vaporkane marked the issue as sponsor confirmed

[kirk-baird]

While this is a valid issue, I do not believe it meets the requirements for a medium severity for the following reasons.

• The function has access control restrictions to only allow the admin role
• The admin role can recover anytime by calling the function again
• The admin role cannot drain funds from the protocol through this bug

As such I see this as a non-critical issue and will downgrade to QA.

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-b