oxcm - [H] Users can lose already-accrued claimable amounts during liquidation

[github-actions[bot]]

oxcm

high

[H] Users can lose already-accrued claimable amounts during liquidation

Summary

In the current implementation, the claim() function does not support the withdrawal of the claimable amount for positions that have been liquidated. However, during liquidation, the code does not check if the current position's claimable amount is greater than zero before transferring it to the user.

Vulnerability Detail

The current implementation of the Fair Funding Alchemix Vault has a vulnerability that can result in the loss of unclaimed earnings that are rightfully owed to the token owner. The issue arises because the liquidate() function marks the position as liquidated without first calling the _claimable_for_token() function to settle the claimable amount of WETH that is owed to the token owner.

This claimable amount is the past earnings that have been generated by the protocol but not yet claimed by the token owner. This amount is owed to the token owner and represents a debt owed by the protocol to the owner.

The current implementation does not settle the claimable amount before marking the position as liquidated. As a result, after liquidation, the token owner loses the right to claim any of the past earnings that have been generated but not yet claimed, as the _claimable_for_token() function always returns 0 after a position has been marked as liquidated.

Impact

During liquidation, any unwithdrawn claimable amount for the position will be lost, causing a loss for the user.

Code Snippet

https://github.com/sherlock-audit/2023-02-fair-funding/blob/main/fair-funding/contracts/Vault.vy#L428-L463

@view
@internal
def _claimable_for_token(_token_id: uint256) -> uint256:
    """
    @notice
        Calculates the pending WETH for a given token_id.
    """
    position: Position = self.positions[_token_id]
    if position.is_liquidated:
        return 0

    total_claimable_for_position: uint256 = position.shares_owned * self.amount_claimable_per_share / PRECISION
    return total_claimable_for_position - position.amount_claimed

@external
def claim(_token_id: uint256) -> uint256:
    """
    @notice
        Allows a token holder to claim his share of pending WETH.
    """
    token_owner: address = ERC721(NFT).ownerOf(_token_id)
    assert msg.sender == token_owner, "only token owner can claim"

    amount: uint256 = self._claimable_for_token(_token_id)
    assert amount > 0, "nothing to claim"

    position: Position = self.positions[_token_id]
    position.amount_claimed += amount
    self.positions[_token_id] = position

    ERC20(WETH).transfer(token_owner, amount)

    log Claimed(_token_id, token_owner, amount)
    return amount

https://github.com/sherlock-audit/2023-02-fair-funding/blob/main/fair-funding/contracts/Vault.vy#L318-L353

@nonreentrant("lock")
@external
def liquidate(_token_id: uint256, _min_weth_out: uint256) -> uint256:
    """
    @notice
        Liquidates the underlying debt of position[_token_id] by burning
        a corresponding amount of shares.
        Withdraws remaining value of shares as WETH to token_owner.
        Reverts if owner would receive less than _min_weth_out.
    """
    token_owner: address = ERC721(NFT).ownerOf(_token_id)
    assert token_owner == msg.sender, "only token owner can liquidate"

    position: Position = self.positions[_token_id]
    assert position.is_liquidated == False, "position already liquidated"

    position.is_liquidated = True
    self.positions[_token_id] = position
    self.total_shares -= position.shares_owned

    collateralisation: uint256 = self._latest_collateralisation()
    shares_to_liquidate: uint256 = position.shares_owned * DECIMALS / collateralisation

    amount_shares_liquidated: uint256 = IAlchemist(self.alchemist).liquidate(
        ALCX_YVWETH,                 # _yield_token: address,
        shares_to_liquidate,         # _shares: uint256,
        1                            # _min_amount_out: uint256 -> covered by _min_weth_out
    )

    amount_to_withdraw: uint256 = position.shares_owned - amount_shares_liquidated
    # _withdraw_underlying_from_alchemix reverts on < _min_weth_out
    amount_withdrawn: uint256 = self._withdraw_underlying_from_alchemix(amount_to_withdraw, token_owner, _min_weth_out)

    log Liquidated(_token_id, token_owner, amount_withdrawn)
    return amount_withdrawn

Tool used

Manual Review / ChatGPT PLUS

Recommendation

Consider change to:

@nonreentrant("lock")
@external
def liquidate(_token_id: uint256, _min_weth_out: uint256) -> uint256:
    """
    @notice
        Liquidates the underlying debt of position[_token_id] by burning
        a corresponding amount of shares.
        Withdraws remaining value of shares as WETH to token_owner.
        Reverts if owner would receive less than _min_weth_out.
    """
    token_owner: address = ERC721(NFT).ownerOf(_token_id)
    assert token_owner == msg.sender, "only token owner can liquidate"

    position: Position = self.positions[_token_id]
    assert position.is_liquidated == False, "position already liquidated"

    position.is_liquidated = True
    self.positions[_token_id] = position
    self.total_shares -= position.shares_owned

    # Check if there are any pending WETH claimable for the position
    claimable_amount: uint256 = self._claimable_for_token(_token_id)
    if claimable_amount > 0:
        position.amount_claimed += claimable_amount
        ERC20(WETH).transfer(token_owner, claimable_amount)

    collateralisation: uint256 = self._latest_collateralisation()
    shares_to_liquidate: uint256 = position.shares_owned * DECIMALS / collateralisation

    amount_shares_liquidated: uint256 = IAlchemist(self.alchemist).liquidate(
        ALCX_YVWETH,                 # _yield_token: address,
        shares_to_liquidate,         # _shares: uint256,
        1                            # _min_amount_out: uint256 -> covered by _min_weth_out
    )

    amount_to_withdraw: uint256 = position.shares_owned - amount_shares_liquidated
    # _withdraw_underlying_from_alchemix reverts on < _min_weth_out
    amount_withdrawn: uint256 = self._withdraw_underlying_from_alchemix(amount_to_withdraw, token_owner, _min_weth_out)

    log Liquidated(_token_id, token_owner, amount_withdrawn)
    return amount_withdrawn

Duplicate of #123