Liquidations are not possible when the pool doesn't have enough tokens to payback the liquidator because of a rounding that will make the prices of lending shares to drop if the liquidation is executed.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L493-L499 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol#L229-L232

Vulnerability details

It is stated in the documentation of the contest from the c4 site that liquidations are rounded against the liquidator when it will evaluate the amount of tokens they need to payback related to the borrowshares they payoff. This becomes a problem when the amount that will be liquidated will be bigger then the amount of tokens that are available in the pool, when this happens the liquidation process logic will get in the function _withdrawOrAllocateSharesLiquidation which it's role is to cashout all the possible tokens from the pool and if that amount is not enough it will also give some shares to the liquidator. Now, the problem comes from the fact that totalPoolInShares will be rounded down because variable _maxSharePrice is selected as false, while totalPoolToken is not rounded down and represents the actual(real) amount of tokens that are in the pool. After that when the function _coreWithdrawBare is called and those 2 values will be used to update pseudoTotalPool and totalDepositShares ( using functions _decreasePseudoTotalPool and _decreaseTotalDepositShares ) the variables pseudoTotalPool and totalDepositShares will not be synced anymored as one is updated with a rounded down value and the other one with a real value. The variable totalDepositShares will be bigger then pseudoTotalPool and this becomes a problem in _syncPoolAfterCodeExecution because it will determine that lendingSharePriceAfter is smaller then lendingSharePriceBefore breaking the invariant and reverting the liquidation transaction.

Impact

Any liquidation over a pool that does not have enough tokens in it's balance to to pay the liquidator will be blocked

Proof of Concept

Run the test from below using command

forge test -vvvv --match-test "test_liq__create_shares_for_liquidator_correct_price_feeds_price_drop_20_perc" --fork-url mainnet

 function test_liq__create_shares_for_liquidator_correct_price_feeds_price_drop_20_perc() public {
        _useBlock(NEW_BLOCK);
        _deployNewWiseLending({_mainnetFork: true});
        //This is the bug, for this tests to work properly the _maxSharePrice of calculateLendingShares in _withdrawOrAllocateSharesLiquidation needs to be on false
        //Carol initialization
        address carol = vm.addr(0x17896);

        //Bob initialization
        uint256 bobPositionId;
        uint256 bobDepositSharesAfterFirstDeposit;
        address bob = vm.addr(0x45678);

        //Alice initialization
        uint256 alicePositionId;
        uint256 alicesDepositSharesAfterFirstDeposit;
        uint256 aliceBorrowSharesAfterFirstBorrow;
        address alice = vm.addr(0x12345);

        vm.deal(alice, 200 ether);
        vm.deal(bob, 200 ether);
        vm.deal(carol, 200 ether);
        deal(USDC_ADDRESS, alice, 20000e6);

        vm.warp(block.timestamp+1);

        (   ,
            int256 goodAnswer,
            ,
            ,
        ) = IPriceFeed(USDC_ETH_PRICE_FEED).latestRoundData();

        (   uint80 roundId,
            ,
            ,
            ,
        ) = IPriceFeed(USDC_USD_PRICE_FEED).latestRoundData();

        vm.mockCall(
            USDC_USD_PRICE_FEED, 
            abi.encodeWithSelector(IPriceFeed(USDC_USD_PRICE_FEED).latestRoundData.selector),
            abi.encode(roundId,goodAnswer,0,0,0)
        );

        vm.mockCall(
            USDC_USD_PRICE_FEED, 
            abi.encodeWithSelector(IPriceFeed(USDC_USD_PRICE_FEED).decimals.selector),
            abi.encode(18)
        );

        //Bob operations
        vm.startPrank(bob);
        bobPositionId = POSITION_NFTS_INSTANCE.mintPosition();
        bobDepositSharesAfterFirstDeposit = LENDING_INSTANCE.depositExactAmountETH{value: 10 ether}(bobPositionId);
        vm.stopPrank();
        console.log("Bob deposit shares: ", bobDepositSharesAfterFirstDeposit );

        //Alice operations
        vm.startPrank(alice);
        IERC20(USDC_ADDRESS).approve(address(LENDING_INSTANCE), 20000e6);
        alicePositionId = POSITION_NFTS_INSTANCE.mintPosition();
        alicesDepositSharesAfterFirstDeposit = LENDING_INSTANCE.depositExactAmount(alicePositionId, USDC_ADDRESS, 20000e6);
        aliceBorrowSharesAfterFirstBorrow = LENDING_INSTANCE.borrowExactAmountETH(alicePositionId, 4.1 ether);
        vm.stopPrank();

        //Carol operations
        vm.startPrank(carol);
        uint256 carolPositionId = POSITION_NFTS_INSTANCE.mintPosition();
        LENDING_INSTANCE.depositExactAmountETH{value: 10 ether}(carolPositionId);
        LENDING_INSTANCE.borrowExactAmount(carolPositionId, USDC_ADDRESS, 4000e6);
        vm.stopPrank();

        vm.mockCall(
            USDC_USD_PRICE_FEED, 
            abi.encodeWithSelector(IPriceFeed(USDC_USD_PRICE_FEED).latestRoundData.selector),
            abi.encode(roundId,goodAnswer/10000*8100,0,0,0)
        );
        deal(WETH_ADDRESS, bob, 200e18);
        vm.startPrank(bob);
        IERC20(WETH_ADDRESS).approve(address(LENDING_INSTANCE), type(uint256).max);
        LENDING_INSTANCE.liquidatePartiallyFromTokens(
            alicePositionId,
            bobPositionId,
            WETH_ADDRESS,
            USDC_ADDRESS,
            aliceBorrowSharesAfterFirstBorrow
        );
        vm.stopPrank();

        uint256 numbersOfBobPositions = LENDING_INSTANCE.getPositionLendingTokenLength(bobPositionId);

        console.log("Alice deposit shares: ", alicesDepositSharesAfterFirstDeposit );
        console.log("Alice borrow shares: ", aliceBorrowSharesAfterFirstBorrow );

        console.log("Alice ether balance: ", address(alice).balance);
        console.log("Bob USDC balance: ", IERC20(USDC_ADDRESS).balanceOf(bob));

        console.log("Bob lending positions: ", numbersOfBobPositions);

    }

The output of the test

├─ [0] console::log("lendSharePriceBefore: ", 1000000000049999997 [1e18]) [staticcall]
    │   │   └─ ← ()
    │   ├─ [0] console::log("lendSharePriceAfter: ", 999999999750000062 [9.999e17]) [staticcall]
    │   │   └─ ← ()
    │   └─ ← InvalidAction()
    └─ ← InvalidAction()

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.85s (1.77s CPU time)

Ran 1 test suite in 4.85s (2.85s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in contracts/FindingsTest.t.sol:FindingsTest
[FAIL. Reason: InvalidAction()] test_liq__create_shares_for_liquidator_correct_price_feeds_price_drop_20_perc() (gas: 34530857)

We can clearly observe that lendSharePriceAfter is smaller then lendSharePriceBefore and this is why the transaction is reverted with the error InvalidAction.

Tools Used

Manual Review

Recommended Mitigation Steps

When you are calculating the value of totalPoolInShares round up the value turning _maxSharePrice to true.

        uint256 totalPoolInShares = calculateLendingShares(
            {
                _poolToken: _poolToken,
                _amount: totalPoolToken,
                _maxSharePrice: true
            }
        );

Assessed type

DoS

[vm06007]

Any liquidation over a pool that does not have enough tokens in it's balance to to pay the liquidator will be blocked

that seems to be like a desired functionality by design and expected behavior. @vonMangoldt can confirm

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[vonMangoldt]

Any liquidation over a pool that does not have enough tokens in it's balance to to pay the liquidator will be blocked

that seems to be like a desired functionality by design and expected behavior. @vonMangoldt can confirm

no i think hes right from my first looking into it. Just curious why it didnt dos in our javascript tests. Probably the percentage roundings etc need to be aligned for that behaviour

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[vonMangoldt]

I would suggest downgrading to medium since as a liquidator you can always find a rounding where it still works which is annoying but it means the general funcitonality still works and no user funds at risk etc. Still a good find 👍 (For an example take a look at javascript test extra functionality2 : it("Liquidator get right amount if shares when pool has not enough token") @GalloDaSballo

[vm06007]

@GalloDaSballo please mark this as disagree with severity (add label)

[c4-judge]

trust1995 marked issue #116 as primary and marked this issue as a duplicate of 116

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)