twicek - Leverage calculation is wrong
Opened this issue · 12 comments
twicek
high
Leverage calculation is wrong
Summary
Leverage calculation is wrong which will lead to unfair liquidations or over leveraged positions depending on price movements.
Vulnerability Detail
_calculate_leverage miscalculate the leverage by using _debt_value + _margin_value as numerator instead of _position_value :
def _calculate_leverage(
_position_value: uint256, _debt_value: uint256, _margin_value: uint256
) -> uint256:
if _position_value <= _debt_value:
# bad debt
return max_value(uint256)
return (
PRECISION
* (_debt_value + _margin_value)
/ (_position_value - _debt_value)
/ PRECISION
)The three inputs of the function _position_value, _debt_value and _margin_value are all determined by a chainlink oracle price feed.
_debt_value represents the value of the position's debt share converted to debt amount in USD.
_margin_value represents the current value of the position's initial margin amount in USD.
_position_value represents the current value of the position's initial position amount in USD.
The problem with the above calculation is that _debt_value + _margin_value does not represent the value of the position. The leverage is the ratio between the current value of the position and the current margin value. _position_value - _debt_value is correct and is the current margin value, but _debt_value + _margin_value doesn't represent the current value of the position since there is no guarantee that the debt token and the position token have correlated price movements.
Example: debt token: ETH, position token: BTC.
- Alice uses 1 ETH of margin to borrow 14 ETH (2k USD/ETH) and get 1 BTC (30k USD/BTC) of position token. Leverage is 14.
- The next day, the price of ETH in USD is still 2k USD/ETH but BTC price in USD went down from 30k to 29k USD/BTC. Leverage is now (
_position_value == 29k) / (_position_value == 29k-_debt_value == 28k) = 29, instead of what is calculated in the contract: (_debt_value == 28k + _margin_value == 2k) / (_position_value == 29k-_debt_value == 28k) = 30.
Impact
Leverage calculation is wrong which will lead to unfair liquidations or over leveraged positions depending on price movements.
Code Snippet
Tool used
Manual Review
Recommendation
Consider modifying the code like this:
def _calculate_leverage(
_position_value: uint256, _debt_value: uint256, _margin_value: uint256
) -> uint256:
if _position_value <= _debt_value:
# bad debt
return max_value(uint256)
return (
PRECISION
- * (_debt_value + _margin_value)
+ * (_position_value)
/ (_position_value - _debt_value)
/ PRECISION
)While our implementation uses a slightly different definition of leverage and the initial margin + debt as base, we agree that the above implementation is cleaner and more intuitive for users.
Both formulas would work similarly though and liquidate positions timely to protect the protocol.
agree, maybe medium is more appropriate
The dup #100 gives an example why the current formula could lead to unexpected result.
Escalate for 10 USDC.
My report shows why the current used formula is wrong as it does not take into account that debt tokens and position tokens are not necessarily tokens with correlated prices.
The duplicate #100 shows in another way that the formula fail to calculate the leverage of a position correctly.
The impact is the same, but my report highlight _debt_value + _margin_value != _position_value, the same way that the debt against a house is not equal to the market value of this house (also described in another way in #156).
The definition of leverage used in the code is not correct and will lead to unfair liquidations or over leveraged positions, which is definitely high severity.
Escalate for 10 USDC.
My report shows why the current used formula is wrong as it does not take into account that debt tokens and position tokens are not necessarily tokens with correlated prices.
The duplicate #100 shows in another way that the formula fail to calculate the leverage of a position correctly.
The impact is the same, but my report highlight_debt_value + _margin_value != _position_value, the same way that the debt against a house is not equal to the market value of this house (also described in another way in #156).
The definition of leverage used in the code is not correct and will lead to unfair liquidations or over leveraged positions, which is definitely high severity.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
The duplicate report #100 shows how adding margin makes a position less healthy, and removing margin makes a position healthier. This is simply a backwards implementation that will lead to unfair liquidations or higher leverage than should be possible. The impact is clear loss of funds.
Recommendation:
change the original judging, to high severity.
Escalate for 10 USDC. My report shows why the current used formula is wrong as it does not take into account that debt tokens and position tokens are not necessarily tokens with correlated prices. The duplicate #100 shows in another way that the formula fail to calculate the leverage of a position correctly. The impact is the same, but my report highlight
_debt_value + _margin_value != _position_value, the same way that the debt against a house is not equal to the market value of this house (also described in another way in #156). The definition of leverage used in the code is not correct and will lead to unfair liquidations or over leveraged positions, which is definitely high severity.
Unexpected and unfair liquidation could cause loss to users. Since the issue roots from the formula, the loss could be long term, result in accumulated fund loss for users, can can be deemed as "material loss of funds".
Based on the above, high severity might be appropriate.
@Unstoppable-DeFi based on the above escalation it seems to be a high issue. Is there any other reason this should not be a high-severity issue?
Result:
High
Has duplicates
Considering this issue a valid high
Fixed by changing the leverage calculation formula from using * (_debt_value + _margin_value) to using * (_position_value) instead