Algo Ssstablecoinsss
First Flight #30
Beginner FriendlyDeFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Incorrect "Health Factor" Check Allows Full Collateral Withdrawal

aayku

Summary

The protocol's health factor check is incorrectly placed during collateral redemption, allowing users to bypass critical insolvency protections by exploiting intermediate states where their debt is set to zero. This logic flaw enables full collateral withdrawal even after severe price drops, risking total protocol insolvency.

Vulnerability Details

The protocol uses a "health factor" mechanism to ensure that collateral remains sufficient, preventing users from redeeming collateral when prices drop significantly. This mechanism is enforced through the _revert_if_health_factor_is_broken function, which reverts transactions if the user's health factor falls below a safe threshold.

However, the correct placement of this health factor check depends on the specific process being executed:

  • Depositing Collateral
    When depositing collateral, the health factor should be checked after updating the user’s collateral balance.

  • Redeeming Collateral
    When redeeming collateral, the health factor check should be performed before any state changes, such as reducing collateral balances or burning tokens. This prevents the system from being manipulated by intermediate states where balances are reduced or set to zero. For this protocol, the redeeming process is implemented incorrectly because the health factor check happens after these critical state changes.

For example, in the redeem_collateral_for_dsc function, if a user withdraws all collateral and burns all tokens, the system will see total_dsc_minted = 0 when performing the health factor check, returning the maximum possible value due to this logic:

if total_dsc_minted == 0:
return max_value(uint256)

This logic flaw allows bypassing the critical health factor check, enabling users to redeem full collateral even after a severe price drop, putting the protocol at risk of insolvency.

Note: The current implementation might assume max_value(uint256) as a valid return for users without any DSC debt. The critical risk occurs only when users have non-zero DSC debt but can still withdraw collateral due to bypassing the health factor check.

Impact

This logic flaw allows users with outstanding DSC debt to potentially redeem more collateral than intended after a severe price drop, risking protocol insolvency due to miscalculated health factors.

Tools Used

Manual review

Proof of Concept

This can be proved by running the following unit test:

def test_redeem_collateral_for_dsc_works_if_eth_price_1usd(dsce, weth, dsc, some_user, eth_usd):
# Ensure ETH price is in place
eth_usd_updated_price = 2000 * 10**8 # 1 ETH = $2000
eth_usd.updateAnswer(eth_usd_updated_price)
​
with boa.env.prank(some_user):
# Checks to ensure user has no value in the dsc_engine
dsc_minted, collateral_in_value = dsce.get_account_information(some_user)
assert dsc_minted == 0
assert collateral_in_value == 0
​
# Deposit collateral and mint stablecoins
initial_user_weth_balance = weth.balanceOf(some_user)
weth.approve(dsce, COLLATERAL_AMOUNT)
dsc.approve(dsce, AMOUNT_TO_MINT)
dsce.deposit_collateral_and_mint_dsc(weth, COLLATERAL_AMOUNT, AMOUNT_TO_MINT)
assert weth.balanceOf(some_user) < initial_user_weth_balance
​
# Adjust ETH price to $1
eth_usd_updated_price = 1 * 10**8 # 1 ETH = $1
eth_usd.updateAnswer(eth_usd_updated_price)
​
# Redeem collateral for DSC - this should not be possible...
dsce.redeem_collateral_for_dsc(weth, COLLATERAL_AMOUNT, AMOUNT_TO_MINT)
user_dsc_balance = dsc.balanceOf(some_user)
assert user_dsc_balance == 0
user_weth_balance = weth.balanceOf(some_user)
assert user_weth_balance == initial_user_weth_balance

Recommendations

The health factor check should happen before calculations that are related with collateral redemption or token burning:

def redeem_collateral_for_dsc(
token_collateral_address: address,
amount_collateral: uint256,
amount_dsc_to_burn: uint256,
):
+ self._revert_if_health_factor_is_broken(msg.sender)
self._burn_dsc(amount_dsc_to_burn, msg.sender, msg.sender)
self._redeem_collateral(
token_collateral_address, amount_collateral, msg.sender, msg.sender
)
- self._revert_if_health_factor_is_broken(msg.sender)
​
...
@external
def redeem_collateral(
token_collateral_address: address, amount_collateral: uint256
):
+ self._revert_if_health_factor_is_broken(msg.sender)
self._redeem_collateral(
token_collateral_address, amount_collateral, msg.sender, msg.sender
)
- self._revert_if_health_factor_is_broken(msg.sender)
​
...
@external
def burn_dsc(amount_dsc_to_burn: uint256):
+ self._revert_if_health_factor_is_broken(msg.sender)
self._burn_dsc(amount_dsc_to_burn, msg.sender, msg.sender)
- self._revert_if_health_factor_is_broken(msg.sender)
​
Updates

Lead Judging Commences

bube Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.