Yuki - Wrong accounting of the storage balances results for the protocol to be in debt even when the bad debt is repaid.

[sherlock-admin]

Yuki

high

Wrong accounting of the storage balances results for the protocol to be in debt even when the bad debt is repaid.

Summary

Wrong accounting of the storage balances results for the protocol to be in debt even when the bad debt is repaid.

Vulnerability Detail

When a position is fully closed, it can result to accruing bad debt or being in profit and repaying the borrowed liquidity which all depends on the amount_out_received from the swap.

In a bad debt scenario the function calculates the bad debt and accrues it based on the difference between the position debt and the amount out received from the swap. And after that repays the liquidity providers with the same received amount from the swap.

    if amount_out_received >= position_debt_amount:
        # all good, LPs are paid back, remainder goes back to trader
        trader_pnl: uint256 = amount_out_received - position_debt_amount
        self.margin[position.account][position.debt_token] += trader_pnl
        self._repay(position.debt_token, position_debt_amount)
    else:
        # edge case: bad debt
        self.is_accepting_new_orders = False  # put protocol in defensive mode
        bad_debt: uint256 = position_debt_amount - amount_out_received
        self.bad_debt[position.debt_token] += bad_debt
        self._repay(
            position.debt_token, amount_out_received
        )  # repay LPs as much as possible
        log BadDebt(position.debt_token, bad_debt, position.uid)

The mapping total_debt_amount holds all debt borrowed from users, and this amount accrues interest with the time.

Prior to closing a position, the bad debt is calculated and accrued to the mapping bad_debt, but it isn't subtracted from the mapping total_debt_amount which holds all the debt even the bad one accrued through the time.

@internal
def _update_debt(_debt_token: address):
    """
    @notice
        Accounts for any accrued interest since the last update.
    """
    if block.timestamp == self.last_debt_update[_debt_token]:
        return  # already up to date, nothing to do

    self.last_debt_update[_debt_token] = block.timestamp
    
    if self.total_debt_amount[_debt_token] == 0:
        return # no debt, no interest

    self.total_debt_amount[_debt_token] += self._debt_interest_since_last_update(
        _debt_token
    )

As the bad debt isn't subtracted from the total_debt_amount when closing a position, even after repaying the bad debt. It will still be in the total_debt_amount, which will prevent the full withdrawing of the liquidity funds.

@nonreentrant("lock")
@external
def repay_bad_debt(_token: address, _amount: uint256):
    """
    @notice
        Allows to repay bad_debt in case it was accrued.
    """
    self.bad_debt[_token] -= _amount
    self._safe_transfer_from(_token, msg.sender, self, _amount)

@internal
@view
def _available_liquidity(_token: address) -> uint256:
    return self._total_liquidity(_token) - self.total_debt_amount[_token]

@internal
@view
def _total_liquidity(_token: address) -> uint256:
    return (
        self.base_lp_total_amount[_token]
        + self.safety_module_lp_total_amount[_token]
        - self.bad_debt[_token]
    )

Impact

The issue leads to liquidity providers unable to withdraw their full amount of funds, as even after repaying the bad debt it will still be in the total_debt_amount mapping.

Code Snippet

https://github.com/sherlock-audit/2023-06-unstoppable/blob/main/unstoppable-dex-audit/contracts/margin-dex/Vault.vy#L233

Tool used

Manual Review

Recommendation

The only way to fix this problem is to repay the position debt amount prior to closing a position and not only the received amount from the swap. Because total_debt_amount holds the bad debt as well.

    else:
        # edge case: bad debt
        self.is_accepting_new_orders = False  # put protocol in defensive mode
        bad_debt: uint256 = position_debt_amount - amount_out_received
        self.bad_debt[position.debt_token] += bad_debt
        self._repay(
            position.debt_token, position_debt_amount
        )  # repay LPs as much as possible
        log BadDebt(position.debt_token, bad_debt, position.uid)

[CodingNameKiki]

Escalate

This issue should be high, as it results to permanent loss of funds for liquidity providers.

What the issue leads to:

1. Double counting of the bad debt in the system, as it isn't subtracted from the mapping total_debt_amount.
2. When the bad debt is repaid, the system will still be in debt.
3. Liquidity providers will not be able to withdraw their full amount of funds, as a result the funds will be stuck in the system.

The referred issue describes the outcome of the issue well, but l am still going to provide an additional explanation in this escalation.

Detailed explanation

Part 1 - Showing how bad debt accrues.

The internal function _close_position is called when a position is fully closed.

There could be two scenarios when the function is called.

• if amount_out_received >= position_debt_amount: - the amount out received from the swap is bigger than the position debt, which indicates that the user is in profit and the whole debt amount is repaid and subtracted from the mapping total_debt_amount.
• else indicates that the amount out received from the swap is smaller than the position debt. As a result bad debt is accrued in the mapping bad_debt.

@internal
def _close_position(_position_uid: bytes32, _min_amount_out: uint256) -> uint256:
    """
    @notice
        Closes an existing position, repays the debt plus
        accrued interest and credits/debits the users margin
        with the remaining PnL.
    """
    # fetch the position from the positions-dict by uid
    position: Position = self.positions[_position_uid]

    # assign to local variable to make it editable
    min_amount_out: uint256 = _min_amount_out
    if min_amount_out == 0:
        # market order, add some slippage protection
        min_amount_out = self._market_order_min_amount_out(
            position.position_token, position.debt_token, position.position_amount
        )

    position_debt_amount: uint256 = self._debt(_position_uid)
    amount_out_received: uint256 = self._swap(
        position.position_token,
        position.debt_token,
        position.position_amount,
        min_amount_out,
    )

    if amount_out_received >= position_debt_amount:
        # all good, LPs are paid back, remainder goes back to trader
        trader_pnl: uint256 = amount_out_received - position_debt_amount
        self.margin[position.account][position.debt_token] += trader_pnl
        self._repay(position.debt_token, position_debt_amount)
    else:
        # edge case: bad debt
        self.is_accepting_new_orders = False  # put protocol in defensive mode
        bad_debt: uint256 = position_debt_amount - amount_out_received
        self.bad_debt[position.debt_token] += bad_debt
        self._repay(
            position.debt_token, amount_out_received
        )  # repay LPs as much as possible
        log BadDebt(position.debt_token, bad_debt, position.uid)

    # cleanup position
    self.positions[_position_uid] = empty(Position)

    log PositionClosed(position.account, position.uid, position, amount_out_received)

    return amount_out_received

Conclusion:

• When bad debt is accrued, the system adds the amount of debt to the mapping bad_debt, but doesn't subtract the same amount from the mapping total_debt_amount.
• The mapping total_debt_amount holds all the debt borrowed from users and the extra amount of debt accrued through the time in terms of interest.
• As a result the bad debt is double counted in the system.

The system provides a way for users to repay the bad debt accrued in the mapping bad_debt.

• However as double counted, once repaid from the mapping bad_debt, the other same half will still persist in the mapping total_debt_amount.

@nonreentrant("lock")
@external
def repay_bad_debt(_token: address, _amount: uint256):
    """
    @notice
        Allows to repay bad_debt in case it was accrued.
    """
    self.bad_debt[_token] -= _amount
    self._safe_transfer_from(_token, msg.sender, self, _amount)

Part 2 - Liquidity providers not able to withdraw their full amount of funds

So far we've explained the main part of the issue, but lets see the impact on the liquidity providers.

• When the function withdraw_liquidity is called, it checks that the system has the available amount of liquidity in order to perform the withdraw.

@nonreentrant("lock")
@external
def withdraw_liquidity(_token: address, _amount: uint256, _is_safety_module: bool):
    """
    @notice
        Allows LPs to withdraw their _token liquidity.
        Only liquidity that is currently not lent out can be withdrawn.
    """
    assert (self.account_withdraw_liquidity_cooldown[msg.sender] <= block.timestamp), "cooldown"

    assert _amount <= self._available_liquidity(_token), "liquidity not available"

    self._account_for_withdraw_liquidity(_token, _amount, _is_safety_module)

    self._safe_transfer(_token, msg.sender, _amount)
    log WithdrawLiquidity(msg.sender, _token, _amount)

What we know so far:

• When the system accrues bad debt, it forgets to remove the particular amount of bad debt from the mapping total_debt_amount, which holds all the debt accrued through the time in terms of interest (debt + extra debt), as a result after bad debt is accrued the same amount of bad debt persist both in the mappings bad_debt and total_debt_amount.

What happens when calculating the available liquidity:

1. First we can see that the total liquidity amount is calculated based on the provided liquidity from the liquidity providers. Then the bad debt which the system holds is subtracted from it.
2. After that the system subtracts the total_debt_amount from the total liquidity.
3. As a result the same amount of bad debt accrued is subtracted twice from the total liquidity as it persist both in the mappings bad_debt and total_debt_amount.

@internal
@view
def _available_liquidity(_token: address) -> uint256:
    return self._total_liquidity(_token) - self.total_debt_amount[_token]

@internal
@view
def _total_liquidity(_token: address) -> uint256:
    return (
        self.base_lp_total_amount[_token]
        + self.safety_module_lp_total_amount[_token]
        - self.bad_debt[_token]
    )

Conclusion:

• The conclusion is that even if the bad debt is repaid from the mapping bad_debt, it will still persist in total_debt_amount.
• This amount of bad debt can't be removed from the mapping total_debt_amount and will be accrued in terms of interest through time and persist in it forever.
• Duo to this issue the liquidity providers will not be able to withdraw the full amount of funds they deposited, and the same amount of funds will be permanently locked in the system.

[sherlock-admin2]

Escalate

This issue should be high, as it results to permanent loss of funds for liquidity providers.

What the issue leads to:

1. Double counting of the bad debt in the system, as it isn't subtracted from the mapping total_debt_amount.
2. When the bad debt is repaid, the system will still be in debt.
3. Liquidity providers will not be able to withdraw their full amount of funds, as a result the funds will be stuck in the system.

The referred issue describes the outcome of the issue well, but l am still going to provide an additional explanation in this escalation.

Detailed explanation

Part 1 - Showing how bad debt accrues.

The internal function _close_position is called when a position is fully closed.

There could be two scenarios when the function is called.

• if amount_out_received >= position_debt_amount: - the amount out received from the swap is bigger than the position debt, which indicates that the user is in profit and the whole debt amount is repaid and subtracted from the mapping total_debt_amount.
• else indicates that the amount out received from the swap is smaller than the position debt. As a result bad debt is accrued in the mapping bad_debt.

@internal
def _close_position(_position_uid: bytes32, _min_amount_out: uint256) -> uint256:
    """
    @notice
        Closes an existing position, repays the debt plus
        accrued interest and credits/debits the users margin
        with the remaining PnL.
    """
    # fetch the position from the positions-dict by uid
    position: Position = self.positions[_position_uid]

    # assign to local variable to make it editable
    min_amount_out: uint256 = _min_amount_out
    if min_amount_out == 0:
        # market order, add some slippage protection
        min_amount_out = self._market_order_min_amount_out(
            position.position_token, position.debt_token, position.position_amount
        )

    position_debt_amount: uint256 = self._debt(_position_uid)
    amount_out_received: uint256 = self._swap(
        position.position_token,
        position.debt_token,
        position.position_amount,
        min_amount_out,
    )

    if amount_out_received >= position_debt_amount:
        # all good, LPs are paid back, remainder goes back to trader
        trader_pnl: uint256 = amount_out_received - position_debt_amount
        self.margin[position.account][position.debt_token] += trader_pnl
        self._repay(position.debt_token, position_debt_amount)
    else:
        # edge case: bad debt
        self.is_accepting_new_orders = False  # put protocol in defensive mode
        bad_debt: uint256 = position_debt_amount - amount_out_received
        self.bad_debt[position.debt_token] += bad_debt
        self._repay(
            position.debt_token, amount_out_received
        )  # repay LPs as much as possible
        log BadDebt(position.debt_token, bad_debt, position.uid)

    # cleanup position
    self.positions[_position_uid] = empty(Position)

    log PositionClosed(position.account, position.uid, position, amount_out_received)

    return amount_out_received

Conclusion:

• When bad debt is accrued, the system adds the amount of debt to the mapping bad_debt, but doesn't subtract the same amount from the mapping total_debt_amount.
• The mapping total_debt_amount holds all the debt borrowed from users and the extra amount of debt accrued through the time in terms of interest.
• As a result the bad debt is double counted in the system.

The system provides a way for users to repay the bad debt accrued in the mapping bad_debt.

• However as double counted, once repaid from the mapping bad_debt, the other same half will still persist in the mapping total_debt_amount.

@nonreentrant("lock")
@external
def repay_bad_debt(_token: address, _amount: uint256):
    """
    @notice
        Allows to repay bad_debt in case it was accrued.
    """
    self.bad_debt[_token] -= _amount
    self._safe_transfer_from(_token, msg.sender, self, _amount)

Part 2 - Liquidity providers not able to withdraw their full amount of funds

So far we've explained the main part of the issue, but lets see the impact on the liquidity providers.

• When the function withdraw_liquidity is called, it checks that the system has the available amount of liquidity in order to perform the withdraw.

@nonreentrant("lock")
@external
def withdraw_liquidity(_token: address, _amount: uint256, _is_safety_module: bool):
    """
    @notice
        Allows LPs to withdraw their _token liquidity.
        Only liquidity that is currently not lent out can be withdrawn.
    """
    assert (self.account_withdraw_liquidity_cooldown[msg.sender] <= block.timestamp), "cooldown"

    assert _amount <= self._available_liquidity(_token), "liquidity not available"

    self._account_for_withdraw_liquidity(_token, _amount, _is_safety_module)

    self._safe_transfer(_token, msg.sender, _amount)
    log WithdrawLiquidity(msg.sender, _token, _amount)

What we know so far:

• When the system accrues bad debt, it forgets to remove the particular amount of bad debt from the mapping total_debt_amount, which holds all the debt accrued through the time in terms of interest (debt + extra debt), as a result after bad debt is accrued the same amount of bad debt persist both in the mappings bad_debt and total_debt_amount.

What happens when calculating the available liquidity:

1. First we can see that the total liquidity amount is calculated based on the provided liquidity from the liquidity providers. Then the bad debt which the system holds is subtracted from it.
2. After that the system subtracts the total_debt_amount from the total liquidity.
3. As a result the same amount of bad debt accrued is subtracted twice from the total liquidity as it persist both in the mappings bad_debt and total_debt_amount.

@internal
@view
def _available_liquidity(_token: address) -> uint256:
    return self._total_liquidity(_token) - self.total_debt_amount[_token]

@internal
@view
def _total_liquidity(_token: address) -> uint256:
    return (
        self.base_lp_total_amount[_token]
        + self.safety_module_lp_total_amount[_token]
        - self.bad_debt[_token]
    )

Conclusion:

• The conclusion is that even if the bad debt is repaid from the mapping bad_debt, it will still persist in total_debt_amount.
• This amount of bad debt can't be removed from the mapping total_debt_amount and will be accrued in terms of interest through time and persist in it forever.
• Duo to this issue the liquidity providers will not be able to withdraw the full amount of funds they deposited, and the same amount of funds will be permanently locked in the system.

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.

[SyntheticYuki]

The escalation is correct, the issue should be high severity.

[141345]

Recommendation:
change the original judging, to high severity.

Escalate

This issue should be high, as it results to permanent loss of funds for liquidity providers.

What the issue leads to:

1. Double counting of the bad debt in the system, as it isn't subtracted from the mapping total_debt_amount.
2. When the bad debt is repaid, the system will still be in debt.
3. Liquidity providers will not be able to withdraw their full amount of funds, as a result the funds will be stuck in the system.

The referred issue describes the outcome of the issue well, but l am still going to provide an additional explanation in this escalation.

Detailed explanation

Part 1 - Showing how bad debt accrues.

The internal function _close_position is called when a position is fully closed.

There could be two scenarios when the function is called.

• if amount_out_received >= position_debt_amount: - the amount out received from the swap is bigger than the position debt, which indicates that the user is in profit and the whole debt amount is repaid and subtracted from the mapping total_debt_amount.
• else indicates that the amount out received from the swap is smaller than the position debt. As a result bad debt is accrued in the mapping bad_debt.

@internal
def _close_position(_position_uid: bytes32, _min_amount_out: uint256) -> uint256:
    """
    @notice
        Closes an existing position, repays the debt plus
        accrued interest and credits/debits the users margin
        with the remaining PnL.
    """
    # fetch the position from the positions-dict by uid
    position: Position = self.positions[_position_uid]

    # assign to local variable to make it editable
    min_amount_out: uint256 = _min_amount_out
    if min_amount_out == 0:
        # market order, add some slippage protection
        min_amount_out = self._market_order_min_amount_out(
            position.position_token, position.debt_token, position.position_amount
        )

    position_debt_amount: uint256 = self._debt(_position_uid)
    amount_out_received: uint256 = self._swap(
        position.position_token,
        position.debt_token,
        position.position_amount,
        min_amount_out,
    )

    if amount_out_received >= position_debt_amount:
        # all good, LPs are paid back, remainder goes back to trader
        trader_pnl: uint256 = amount_out_received - position_debt_amount
        self.margin[position.account][position.debt_token] += trader_pnl
        self._repay(position.debt_token, position_debt_amount)
    else:
        # edge case: bad debt
        self.is_accepting_new_orders = False  # put protocol in defensive mode
        bad_debt: uint256 = position_debt_amount - amount_out_received
        self.bad_debt[position.debt_token] += bad_debt
        self._repay(
            position.debt_token, amount_out_received
        )  # repay LPs as much as possible
        log BadDebt(position.debt_token, bad_debt, position.uid)

    # cleanup position
    self.positions[_position_uid] = empty(Position)

    log PositionClosed(position.account, position.uid, position, amount_out_received)

    return amount_out_received

Conclusion:

• When bad debt is accrued, the system adds the amount of debt to the mapping bad_debt, but doesn't subtract the same amount from the mapping total_debt_amount.
• The mapping total_debt_amount holds all the debt borrowed from users and the extra amount of debt accrued through the time in terms of interest.
• As a result the bad debt is double counted in the system.

The system provides a way for users to repay the bad debt accrued in the mapping bad_debt.

• However as double counted, once repaid from the mapping bad_debt, the other same half will still persist in the mapping total_debt_amount.

@nonreentrant("lock")
@external
def repay_bad_debt(_token: address, _amount: uint256):
    """
    @notice
        Allows to repay bad_debt in case it was accrued.
    """
    self.bad_debt[_token] -= _amount
    self._safe_transfer_from(_token, msg.sender, self, _amount)

Part 2 - Liquidity providers not able to withdraw their full amount of funds

So far we've explained the main part of the issue, but lets see the impact on the liquidity providers.

• When the function withdraw_liquidity is called, it checks that the system has the available amount of liquidity in order to perform the withdraw.

@nonreentrant("lock")
@external
def withdraw_liquidity(_token: address, _amount: uint256, _is_safety_module: bool):
    """
    @notice
        Allows LPs to withdraw their _token liquidity.
        Only liquidity that is currently not lent out can be withdrawn.
    """
    assert (self.account_withdraw_liquidity_cooldown[msg.sender] <= block.timestamp), "cooldown"

    assert _amount <= self._available_liquidity(_token), "liquidity not available"

    self._account_for_withdraw_liquidity(_token, _amount, _is_safety_module)

    self._safe_transfer(_token, msg.sender, _amount)
    log WithdrawLiquidity(msg.sender, _token, _amount)

What we know so far:

• When the system accrues bad debt, it forgets to remove the particular amount of bad debt from the mapping total_debt_amount, which holds all the debt accrued through the time in terms of interest (debt + extra debt), as a result after bad debt is accrued the same amount of bad debt persist both in the mappings bad_debt and total_debt_amount.

What happens when calculating the available liquidity:

1. First we can see that the total liquidity amount is calculated based on the provided liquidity from the liquidity providers. Then the bad debt which the system holds is subtracted from it.
2. After that the system subtracts the total_debt_amount from the total liquidity.
3. As a result the same amount of bad debt accrued is subtracted twice from the total liquidity as it persist both in the mappings bad_debt and total_debt_amount.

@internal
@view
def _available_liquidity(_token: address) -> uint256:
    return self._total_liquidity(_token) - self.total_debt_amount[_token]

@internal
@view
def _total_liquidity(_token: address) -> uint256:
    return (
        self.base_lp_total_amount[_token]
        + self.safety_module_lp_total_amount[_token]
        - self.bad_debt[_token]
    )

Conclusion:

• The conclusion is that even if the bad debt is repaid from the mapping bad_debt, it will still persist in total_debt_amount.
• This amount of bad debt can't be removed from the mapping total_debt_amount and will be accrued in terms of interest through time and persist in it forever.
• Duo to this issue the liquidity providers will not be able to withdraw the full amount of funds they deposited, and the same amount of funds will be permanently locked in the system.

The missed updated total debt will:

• continuing accruing interest
• reduce the available liquidity to use, so reduce income from normal operation
• gradually lock LP's fund

Based on the above, the long term impact could be deemed as "material loss of funds", high is appropriate.

[hrishibhat]

Result:
High
Unique
Agree with Lead judge's comment. Considering this issue a high

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CodingNameKiki: accepted

[Unstoppable-DeFi]

Unstoppable-DeFi/unstoppable-dex-audit#19

[maarcweiss]

Fixed by moving the repay() position logic to before the position is closed