c4-bot-3 commented 4 months ago

Lines of code

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

Vulnerability details

Description

When liquidating a user, _withdrawOrAllocateSharesLiquidation() can be called which checks if a pool is large enough to pay out the liquidator, and if not, then the liquidator is allocated shares that can be used to withdraw at a later time when the pool is large enough. There are two scenarios in which the pool will not be large enough to pay out the liquidator:

The obvious scenario where the pool simply doesn't contain enough tokens to cover the withdraw amount
So many tokens are borrowed out of the pool that there isn't enough available to pay out the liquidator. Note: if there are some tokens available but not enough to cover the entire withdraw amount, then those tokens are transferred, dropping the total pool to 0, and the rest are allocated as shares

However, if these scenarios were ever to arise, the _withdrawOrAllocateSharesLiquidation() function would not work as expected. This is due to how it calculates the lending shares (totalPoolInShares) of the total pool: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L493

When liquidating, it will calculate the totalPoolInShares like this (with amount being the total pool):

   lendingShares = (totalDepositShares * amount) / pseudoTotalPool - 1;

However, calculating the lending shares this way will cause the _compareSharePrices() to revert on the syncPool modifier. Specifically the check on

    _validateParameter(
        _lendSharePriceBefore,
        lendSharePriceAfter
    );

This is because _maxSharePrice is passed in as false to calculateLendingShares(), which subtracts one.

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

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L497

However, because we are withdrawing, it should be set to true so that it adds one. In all other areas where calculateLendingShares() is being used, the functions that focus on withdrawing have _maxSharePrice: true and the depositing functions have _maxSharePrice: false: withdrawOnBehalfExactAmount on WiseLending.sol: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLending.sol#L874 _preparationsWithdraw on MainHelper.sol: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/MainHelper.sol#L154 _handleDeposit on WiseCore.sol: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L119

When _maxSharePrice is set to true, the _validateParameter() check will pass.

Even if the _validateParameter() check did not exist, if _maxSharePrice is left as-is (set to false), any subsequent liquidations through _withdrawOrAllocateSharesLiquidation() would revert with panic due to an underflow. This is because (as stated above in bold), liquidating when the pool is not large enough to pay out the liquidator will drop the total pool to 0 and issue the remainder as shares. Therefore, any subsequent calls to liquidate when the total pool is 0 will underflow:

    //amount is totalPoolToken which in this scenario = 0;
    shares = amount * totalDepositShares / pseudoTotalPool - 1

Impact

When a particular receiving token is desired and _withdrawOrAllocateSharesLiquidation() is called, the liquidation will always revert if the total pool is not large enough to cover the withdraw amount. This defeats the purpose of _withdrawOrAllocateSharesLiquidation(), as stated in the NatSpec:

/**
     * @dev Internal math function for liquidation logic
     * which checks if pool has enough token to pay out
     * liquidator. If not, liquidator get corresponding
     * shares for later withdraw.
     */

This will lead to frustrated users who desire a particular receiving token. The level of frustration will be even higher if the reason this function reverts is because the total pool is borrowed out. This would lead to a very high APY, thus a much higher desire to receive the particular token. Additionally, the borrowers in these particular situations will not be liquidated even though they should be. Ultimately, this will lead to loss of faith in the protocol.

However, this can be easily avoided by just passing in a different receiving token that is large enough to payout the withdraw amount, which is why this is a medium level issue.

Proof of Concept

Copy and paste this foundry test into the /contracts folder and run forge test --fork-url mainnet --match-path ./contracts/WoolCentaurLiquidationTest.t.sol --match-test testLiquidateMockPool -vvvv. As the code is, the test will fail. To have it pass, change the bool here: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseCore.sol#L497 to true.

// SPDX-License-Identifier: -- WISE --

pragma solidity =0.8.24;

import "forge-std/Test.sol";
import "forge-std/StdUtils.sol";
import "./InterfaceHub/IBalancerFlashloan.sol";
import "./InterfaceHub/IWiseLending.sol";
import "./InterfaceHub/ICurve.sol";
import "./InterfaceHub/IAaveHub.sol";
import "./InterfaceHub/IAave.sol";
import "./InterfaceHub/IPositionNFTs.sol";
import "./WiseLendingBaseDeployment.t.sol";
import "./PoolManager.sol";

contract WoolCentaur is BaseDeploymentTest {

        function testLiquidateMockPool() public {
            /////////////////////////////
            // Setup new fork and deal tokens
            _useBlock(
                NEW_BLOCK
            );

            _deployNewWiseLending(
                {
                    _mainnetFork: false
                }
            );

            vm.deal(address(1), 2000 ether);
            vm.deal(WISE_DEPLOYER, 1000 ether);
            address paybackToken = address(MOCK_AAVE_ATOKEN_3);
            address receivingToken = address(MOCK_AAVE_ATOKEN_4);
            address mockWETH = address(MOCK_WETH);
            deal(paybackToken, WISE_DEPLOYER, 11000e6);
            deal(receivingToken, address(1), 200000e6);
            deal(mockWETH, WISE_DEPLOYER, 1000e18);
            deal(mockWETH, address(1), 1000e18);

            //Mint position nfts to address 1
            _startPrank(
                address(1)
            );

            POSITION_NFTS_INSTANCE.mintPosition();

            uint256[] memory nftsOfOwner1 = POSITION_NFTS_INSTANCE.walletOfOwner(
                address(1)
            );

            uint256 nftIdFirst = nftsOfOwner1[0];

            _stopPrank();

            // Mint position nfts to WISE_DEPLOYER
            _startPrank(
                WISE_DEPLOYER
            );
            POSITION_NFTS_INSTANCE.mintPosition();

            uint256[] memory nftsOfOwner2 = POSITION_NFTS_INSTANCE.walletOfOwner(
                WISE_DEPLOYER
            );

            uint256 nftIdSecond = nftsOfOwner2[0];

            skip(10);

            //Approve and deposit the payback tokens
            IERC20(paybackToken).approve(
                address(LENDING_INSTANCE),
                10000e6
            );

            LENDING_INSTANCE.depositExactAmount(
                nftIdSecond,
                paybackToken,
                10000e6
            );

            //Approve and deposit the collateral tokens
            IERC20(mockWETH).approve(
                address(LENDING_INSTANCE),
                1000e18
            );

            LENDING_INSTANCE.depositExactAmount(
                nftIdSecond,
                mockWETH,
                1000e18
            );

            _stopPrank();

            _startPrank(
                address(1)
            );
            //Deposit collateral tokens
            LENDING_INSTANCE.depositExactAmountETH{
                value: 2000 ether
            }(nftIdFirst);

            //Set the value of the receiving token
            MOCK_CHAINLINK_4.setValue(
                0.00000000000000001 ether
            );
            //Approve and deposit the receiving token
            IERC20(receivingToken).approve(
                address(LENDING_INSTANCE),
                20000e6
            );

            LENDING_INSTANCE.depositExactAmount(
                nftIdFirst,
                receivingToken,
                20000e6
            );
            //Set the value of the payback token
            MOCK_CHAINLINK_3.setValue(
                0.00000000000000001 ether
            );
            //Liquidatee borrows 100% the payback token
            LENDING_INSTANCE.borrowExactAmount(
                nftIdFirst,
                paybackToken,
                10000e6
            );
            //Set the new value of payback token, pushing it into bad debt so it can be liquidated
            MOCK_CHAINLINK_3.setValue(
                0.00000001 ether
            );

            _stopPrank();

            _startPrank(
                WISE_DEPLOYER
            );
            //Liquidator borrows 100% of the receivingToken so shares must be issued
            LENDING_INSTANCE.borrowExactAmount(
                nftIdSecond,
                receivingToken,
                20000e6
            );

            skip(6000);
            (, uint256 liquidateeLendingSharesBefore) = LENDING_INSTANCE.userLendingData(nftIdFirst, (receivingToken));
            (, uint256 liquidatorLendingSharesBefore) = LENDING_INSTANCE.userLendingData(nftIdSecond, (receivingToken));
            //assert that the liquidator starts with 0 shares
            assertEq(liquidatorLendingSharesBefore, 0);
            emit log_named_uint("shares of liquidatee before", liquidateeLendingSharesBefore);
            emit log_named_uint("shares of liquidator before", liquidatorLendingSharesBefore);
            //Approve and liquidate the respective shares and tokens
            IERC20(paybackToken).approve(
                address(LENDING_INSTANCE),
                11
            );

            LENDING_INSTANCE.liquidatePartiallyFromTokens(
                nftIdFirst,
                nftIdSecond,
                paybackToken,
                receivingToken,
                10
            );
            _stopPrank();

            (, uint256 liquidateeLendingSharesAfter) = LENDING_INSTANCE.userLendingData(nftIdFirst, (receivingToken));
            (, uint256 liquidatorLendingSharesAfter) = LENDING_INSTANCE.userLendingData(nftIdSecond, (receivingToken));
            //asserts that the liquidator shares have increased
            assertNotEq(liquidatorLendingSharesAfter, 0);
            emit log_named_uint("shares of liquidatee after", liquidateeLendingSharesAfter);
            emit log_named_uint("shares of liquidator after", liquidatorLendingSharesAfter);
            //asserts that the liquidatee + the liquidator = the total shares
            assertEq(liquidateeLendingSharesAfter + liquidatorLendingSharesAfter, 19999999998);
        }
}

Tools Used

Foundry/Manual Analysis

Recommended Mitigation Steps

Change the _maxSharePrice under calculateLendingShares on _withdrawOrAllocateSharesLiquidation() to true;

Assessed type

Error

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as duplicate of #238

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as sufficient quality report

trust1995 commented 4 months ago

Selected as best because of good POC + well balanced severity rationalization.

c4-judge commented 4 months ago

trust1995 marked the issue as selected for report

c4-judge commented 4 months ago

trust1995 marked the issue as satisfactory

thebrittfactor commented 2 months ago

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.

code-423n4 / 2024-02-wise-lending-findings

Incorrect calculation of lending shares in `_withdrawOrAllocateSharesLiquidation` can lead to revert and failure to liquidate #116