Closed code423n4 closed 1 year ago
kirk-baird marked the issue as primary issue
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.
REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD
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?
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
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.
vaporkane marked the issue as disagree with severity
vaporkane marked the issue as sponsor confirmed
While this is a valid issue, I do not believe it meets the requirements for a medium severity for the following reasons.
As such I see this as a non-critical issue and will downgrade to QA.
kirk-baird changed the severity to QA (Quality Assurance)
kirk-baird marked the issue as grade-b
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
andAMO2::setRebalanceDownThreshold
allows setting any value toREBALANCE_DOWN_THRESHOLD
andREBALANCE_UP_THRESHOLD
. This allows to breakREBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD
invariantImpact
Invariant
REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD
can be broken leading to unwanted protocol state which can DOSAMO2::rebalanceUp
orAMO2::rebalanceDown
, depending on the new values.POC
Current functions
AMO2::setRebalanceUpThreshold
andAMO2::setRebalanceDownThreshold
allow setting any value toREBALANCE_DOWN_THRESHOLD
andREBALANCE_UP_THRESHOLD
The lack of limitation to hold invariant
REBALANCE_DOWN_THRESHOLD < REBALANCE_UP_THRESHOLD
would breakAMO2::preRebalanceCheck
behavior in case of bad assignment of values: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:
REBALANCE_UP_THRESHOLD
andREBALANCE_DOWN_THRESHOLD
, for this reason he decide to callAMO2::setRebalanceDownThreshold
andAMO2::setRebalanceUpThreshold
. Consider that the newREBALANCE_DOWN_THRESHOLD
is greater than the currentREBALANCE_UP_THRESHOLD
AMO2::setRebalanceDownThreshold
is correctly executed,AMO2::setRebalanceUpThreshold
is reverted. The admin for some reason do not notice thisAMO2::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:
Assessed type
Invalid Validation