search

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

8 stars 6 forks source link

Liquidator will brick the entire pool market, also stealing from the subsequent depositors #281

Closed c4-bot-3 closed 4 months ago

c4-bot-3 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseCore.sol#L460-L536 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/MainHelper.sol#L752-L788

Vulnerability details

Impact

When the pool lacks sufficient funds to fully compensate the liquidator, it will reset both totalPool and totalLendingShares to 0, but will increase the lendingShares of the liquidator by the difference.

This enables the liquidator to retrieve it later when the pool liquidity rises once again, either by paybacks or lp deposits. Consequently, when someone deposits into the pool, the liquidator can claim the remaining portion of their liquidation reward, preventing the depositor from withdrawing their full amount. As we can observe this creates a weird scenario when every next liquidity provider tokens are “stolen” from the previous withdrawer.

This is extremely bad for the market because unlike borrowing, where you are sure that these funds will be back into the pool either by repaying or liquidation when the liquidator that has received surplus shares withdraws them it will lead to a domino effect and the last withdrawer for this market will bear all the cost risking up to 100% of his deposits to be stolen.

When pureCollateral of a user isn’t enough for the liquidator to seize, _withdrawOrAllocateSharesLiquidation is executed:

WiseCore.sol#L460-L536

function _withdrawOrAllocateSharesLiquidation(
    uint256 _nftId,
    uint256 _nftIdLiquidator,
    address _poolToken,
    uint256 _percentWishCollateral
)
    private
    returns (uint256)
{
 ...Code when withdrawAmount <= totalPoolToken

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

    uint256 shareDifference = cashoutShares
        - totalPoolInShares;

    _coreWithdrawBare(
        _nftId,
        _poolToken,
        totalPoolToken,
        totalPoolInShares
    ); 

    _decreaseLendingShares(
        _nftId,
        _poolToken,
        shareDifference
    );

    _increasePositionLendingDeposit(
        _nftIdLiquidator,
        _poolToken,
        shareDifference
    );

    _addPositionTokenData(
        _nftIdLiquidator,
        _poolToken,
        hashMapPositionLending,
        positionLendTokenData
    );

    _removeEmptyLendingData(
        _nftId,
        _poolToken
    );

    //@audit after: 
    //totalPool = 0
    //totalDepositShares = 0
    //shares of liquidator = diff of cashout - total

    return totalPoolToken;
}

When assets of a pool are less than the amount that liquidator receives we can see that both

are being reset to 0.

And when borrowers payback, mappings that get increased are:

MainHelper.sol#L752-L788

function _corePayback(
    uint256 _nftId,
    address _poolToken,
    uint256 _amount,
    uint256 _shares
)
    internal
{
    _updatePoolStorage(
        _poolToken,
        _amount,
        _shares,
        _increaseTotalPool,
        _decreasePseudoTotalBorrowAmount,
        _decreaseTotalBorrowShares
    );

    _decreasePositionMappingValue(
        userBorrowShares,
        _nftId,
        _poolToken,
        _shares
    );

    if (userBorrowShares[_nftId][_poolToken] > 0) {
        return;
    }

    _removePositionData({
        _nftId: _nftId,
        _poolToken: _poolToken,
        _getPositionTokenLength: getPositionBorrowTokenLength,
        _getPositionTokenByIndex: getPositionBorrowTokenByIndex,
        _deleteLastPositionData: _deleteLastPositionBorrowData,
        isLending: false
    });
}

This is not enough for a liquidator to redeem his difference because on withdraw the following mappings should hold enough values:

From there we can see in order liquidator to be able to redeem his shares he needs someone else to deposit his funds in the same market, we saw that payback won’t work. Then after totalDepositShares holds enough shares that satisfy the liquidator he can freely withdraw them “stealing” from the original depositor. Then when he wants to withdraw his tokens and exit the protocol it won’t be possible because previous user has redeemed tokens that belonged to him. Lets consider the example that deposits and withdrawals will be alternating. Then the scenario will continue to happen for each new deposit in the system being stolen from the previous person.

Proof of Concept

Here is a gist containing test that can be placed in test/extraFunctionality2.test.js:

https://gist.github.com/AydoanB/1c8012f8e078d5b5de8981dee96a929e

For simplicity all the other tests can be removed, then in the terminal: npm run test-extra2 will execute it.

The main idea is to verify that when such type of liquidation occurs totalPool is zeroed and after Bob deposits in the system liquidator can redeem shares accrued from liquidation using Bob’s assets.

After that when Bob wants to withdraw big part of his deposit WiseCore::_coreWithdrawBare will revert with underflow.

Tools Used

Manual Review

Recommended Mitigation Steps

Finding an appropriate solution is indeed challenging. One approach could be to allow the liquidator to withdraw the remaining funds only when there is a substantial increase in liquidity, such as 5 times or 10 times the amount he has left to withdraw.

Assessed type

Other

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as sufficient quality report

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as duplicate of #238

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory