sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

OxZ00mer - The omission of Uniswap position fees from a user's assets can result in a premature liquidation #127

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

OxZ00mer

high

The omission of Uniswap position fees from a user's assets can result in a premature liquidation

Summary

Users with inadequate collateral in their Borrower contract to cover their loans will face premature liquidation due to the omission of their accrued Uniswap fees from consideration.

Vulnerability Detail

Users must have sufficient collateral, which can be in the form of token reserves or positions in the Uniswap pool market, to cover their loans when using their Borrower contract.

The problem arises because accrued fees are not considered in the asset calculation of users. Specifically, when assessing assets, only the provided liquidity in the pool is added, while the fees are overlooked. This oversight is reflected in the following code snippet:

function _getAssets(uint256 slot0_, Prices memory prices, bool withdraw) private returns (Assets memory assets) {
    // ...
    unchecked {
        for (uint256 i; i < count; i += 2) {
        // ...
        int24 l = positions[i];
        int24 u = positions[i + 1];

        // @audit only the provided liquidity in the pool gets added as a part of the assets of the user, and not the fees as well.
        (uint128 liquidity, , , , ) = UNISWAP_POOL.positions(keccak256(abi.encodePacked(address(this), l, u)));

        (uint256 amount0, uint256 amount1) = LiquidityAmounts.getAmountsForLiquidity(prices.c, L, U, liquidity);

        // ...
    }
    // ...
}

Furthermore, when a user is liquidated, their Uniswap positions are burned, and their fees are collected but not considered in the liquidation process. As a result, these fees effectively go to the liquidator, resulting in their misappropriation.

Impact

Users who lack sufficient assets in their Borrower contract but have accumulated ample fees on their Uniswap positions to maintain financial health will face premature liquidation, resulting in the loss of their collateral.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L194

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L490-L525

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L515-L517

Tool used

Manual Review

Recommendation

Consider claiming and accounting for the user's fees before evaluating their liquidation eligibility.

function _getAssets(uint256 slot0_, Prices memory prices, bool withdraw) private returns (Assets memory assets) {
    // ...
    unchecked {
        for (uint256 i; i < count; i += 2) {
        // ...
        int24 l = positions[i];
        int24 u = positions[i + 1];

        (uint128 liquidity, , , , ) = UNISWAP_POOL.positions(keccak256(abi.encodePacked(address(this), l, u)));

        (uint256 amount0, uint256 amount1) = LiquidityAmounts.getAmountsForLiquidity(prices.c, L, U, liquidity);

        // @audit to re-calculate the fees owed to the user:
        (burned0, burned1) = UNISWAP_POOL.burn(l, u, 0);
                // @audit collecting the fees
        (collected0, collected1) = UNISWAP_POOL.collect(recipient, lower, upper, type(uint128).max, type(uint128).max);

        // @audit adding the fees to the asset balance of the user, which might prevent them from being prematurely liquidated:
        assets.fluid0C += amount0 + collected0;
                assets.fluid1C += amount1 + collected1;
        // ... 
    }
    // ...
}

Duplicate of #71

sherlock-admin2 commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

panprog commented:

low, because this is known issue and expected behavior (due to gas optimization) as described in the docs

MohammedRizwan commented:

valid