Wise Lending - 0xStalin's results

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L427-L429 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L663-L675 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L689-L699

Vulnerability details

Impact

All the fees collected from the FEE_MANAGER_NFT will be unclaimable on the FeeManager contract because the totalBadDebtETH will never be able to go back to 0.

Proof of Concept

Liquidations in the WiseLending market are designed to allow the liquidators to liquidate debt from a specific pool and receive the liquidation payment on a different pool (if they want it, they can also receive the liquidation payment in the same type of poolToken if the position has collateralized deposits on such a pool).

• For example, if a position has collateralized deposits on PoolA and PoolB, and has loans on PoolA and PoolC.
  • The liquidator can liquidate the debt of the PoolA and receive tokens from the PoolB as the liquidation payment for liquidating the debt of the poolA.

When a liquidation is done, after all the lending and borrowing storage variables have been updated according to the amount of debt repaid by the liquidator, the liquidation logic checks if there is any bad debt in the position by calling the WISE_SECURITY.checkBadDebtLiquidation() function, any bad debt (the difference between the overallETHBorrowBare - overallETHCollateralsBare) is reported to the FeeManager, to do this, two functions are called, the FEE_MANAGER.increaseTotalBadDebtLiquidation() function and the FEE_MANAGER.setBadDebtUserLiquidation() function.

function checkBadDebtLiquidation(
    uint256 _nftId
)
    external
    onlyWiseLending
{

    uint256 bareCollateral = overallETHCollateralsBare(
        _nftId
    );

    uint256 totalBorrow = overallETHBorrowBare(
        _nftId
    );

    //@audit-info => If the total leftover debt is lower than the total leftover collateral, no bad debt, returns!
    if (totalBorrow < bareCollateral) {
        return;
    }

    //@audit-info => If the total leftover debt is greater than the total leftover collateral, bad debt has been generated!
    unchecked {
        uint256 diff = totalBorrow
            - bareCollateral;

        FEE_MANAGER.increaseTotalBadDebtLiquidation(
            diff
        );

        FEE_MANAGER.setBadDebtUserLiquidation(
            _nftId,
            diff
        );
    }
}

• The FEE_MANAGER.increaseTotalBadDebtLiquidation() function will increase the totalBadDebtETH by the amount of baddebt calculated in the WISE_SECURITY.checkBadDebtLiquidation() function.

function increaseTotalBadDebtLiquidation(
    uint256 _amount
)
    external
    onlyWiseSecurity
{
    _increaseTotalBadDebt(
        _amount
    );

    ...
}

function _increaseTotalBadDebt(
    uint256 _amount
)
    internal
{
    //@audit-info => `totalBadDebtETH` is increased by the baddebt computed in the current liquidation
    totalBadDebtETH += _amount;
    ...
}

• The FEE_MANAGER.setBadDebtUserLiquidation() function will simply override the user's baddebt to the amount that was computed during this liquidation.

function setBadDebtUserLiquidation(
    uint256 _nftId,
    uint256 _amount
)
    external
    onlyWiseSecurity
{
    _setBadDebtPosition(
        _nftId,
        _amount
    );
    ...
}

function _setBadDebtPosition(
    uint256 _nftId,
    uint256 _amount
)
    internal
{
    //@audit-info => `badDebtPosition` is overridden by the baddebt computed in the current liquidation
    badDebtPosition[_nftId] = _amount;
}

The problem with the existing logic to report bad debt to the FeeManager is that the bad debt of the same position will be reported multiple times. Let's do an example where a position has collateralized deposits on 2 pools, and has taken loans on 3 different pools. (For this example we will simulate the state of the position as if bad debt would've been generated in the position.)

• Collateralized Deposits - overallETHCollateralsBare: 2.3_e18 ETH
  • PoolA: 0.8_e18 ETH
  • PoolB: 1.5_e18 ETH
• Loans - overallETHBorrowBare: 2.4_e18 ETH
  • PoolA: 0.4_e18 ETH
  • PoolB: 0.8_e18 ETH
  • PoolC: 1.2_e18 ETH

This position is in a liquidable state, thus, a liquidator will proceed to liquidate debt from the pools where the position has borrowed.

1. A liquidator will liquidate the debt of the poolA and ask to receive the same tokens of poolA as the liquidation payment.

• When the liquidation logic gets to the WISE_SECURITY.checkBadDebtLiquidation() function, the state of the position will be as follow:

  • Collateralized Deposits - overallETHCollateralsBare: 1.9_e18 ETH

    • PoolA: ~0.4_e18 ETH (leftover collateral on PoolA after covering the liquidator's payment)
    • PoolB: 1.5_e18 ETH

  • Loans - overallETHBorrowBare: 2_e18 ETH

    • PoolA: 0 ETH (The liquidator repaid the debt of this pool during this liquidation)
    • PoolB: 0.8_e18 ETH
    • PoolC: 1.2_e18 ETH

  • diff / totalBadDebt = (overallETHBorrowBare - overallETHCollateralsBare) === (2_e18 - 1.9_e18) == 0.1_e18 ETH

    • This means, this liquidation will report to the FeeManager that the position being liquidated has a total of 0.1_e18 ETH worth of bad debt
      • totalBadDebtETH will be increased by the current's position bad debt (0.1_e18 ETH)
      • User's bad debt (badDebtPosition) will be set to the current's position bad debt (0.1_e18 ETH)

2. Now, the position has debt only on PoolB and PoolC (PoolA's debt has been liquidated), and the position has still collateralized deposits on PoolA and PoolB. For this liquidation, the liquidator decides to liquidate the debt of the PoolC and asks to receive poolTokens from the PoolB as the liquidation payment.

• When the liquidation logic gets to the WISE_SECURITY.checkBadDebtLiquidation() function, the state of the position will be as follow:
  • Collateralized Deposits - overallETHCollateralsBare: ~0.7_e18 ETH
    • PoolA: ~0.4_e18 ETH
    • PoolB: ~0.3_e18 ETH (leftover collateral on PoolB after covering the liquidator's payment)
  • Loans - overallETHBorrowBare: 0.8_e18 ETH
    • PoolA: 0 ETH
    • PoolB: 0.8_e18 ETH
    • PoolC: 0 ETH (The liquidator repaid the debt of this pool during this liquidation)
  • diff / totalBadDebt = (overallETHBorrowBare - overallETHCollateralsBare) === (0.7_e18 - 0.8_e18) == 0.1_e18 ETH
    • This means, the second liquidation will also report to the FeeManager that the position has a total of 0.1_e18 ETH worth of bad debt
      • totalBadDebtETH will be increased by the current's position bad debt (0.1_e18 ETH)
        • Here, the totalBadDebtETH has been increased two times by the bad debt of the same position
      • User's bad debt (badDebtPosition) will be set to the current's position bad debt (0.1_e18 ETH)
        • The userBadDebt is registered only as the current bad debt.

3. The same will happen for the liquidation of the debt on the PoolB, the leftover bad debt will be reported again to the FeeManager, which will cause the totalBadDebtEth to be increased again.

4. The result of liquidating the debt across all the pools from this position is that the FeeManager has registered multiple times the same bad debt from the same position as if it would have been bad debt from different positions. In the end, the totalBadDebtEth was increased by 0.4_e18 ETH, and the userBadDebt was set as 0.1_e18 ETH

• When the bad debt of this position is repaid using the FeeManager.paybackBadDebtForToken() function, the repaid amount would be the current bad debt, which would be the 0.1_e18 ETH of bad debt registered in the last liquidation of the position. The totalBadDebtEth will only be decreased by the current userBadDebt.
  • This means, even though the totalBadDebtEth was updated multiple times with the bad debt of the same position, the maximum amount of bad debt that can be decreased when repaying the position's baddebt is the current position's bad debt (Which at most can represent the bad debt that was registered in the last liquidation)
  • For this example, after the bad debt has been repaid, the totalBadDebtEth would end up with an extra 0.3_e18 ETH of bad debt, while the userBadDebt is deleted. This will make the totalBadDebtEth impossible to be taken down to 0 again because the userBadDebt and the totalBadDebtEth were incorrectly updated by the liquidation process

This is a problem that will occur for any position that generates bad debt and has loans in more than 2 pools. The liquidation of the loans on each pool will report to the FeeManager the leftover bad debt in the position. As the liquidators liquidate the loans on the different pools, each liquidation will report the same bad debt to the FeeManager causing the totalBadDebtEth to grow more than what it really should.

Tools Used

Manual Audit

Recommended Mitigation Steps

The recommendation would be to use a similar approach to the FeeManagerHelper._updateUserBadDebt() function

• Instead of straight updating the totalBadDebtETH, first update the userBadDebt, and update the totalBadDebtETH depending on the difference between the newBadDebt and the currentBadDebt

1. The easiest way to implement this recommendation would be to add the below function on the FeeManager contract

//@audit-info => `_newBadDebt` would be the current bad debt on the position that is been liquidated on the WiseLending contract!
function registerBadDebt(_nftId, _newBadDebt) external onlyWiseSecurity {

    //@audit-info => Load the current bad debt of the position registered in the FeeManager
    uint256 currentBadDebt = badDebtPosition[
        _nftId
    ];

    unchecked {
        _setBadDebtPosition(
            _nftId,
            _newBadDebt
        );

        //@audit-info => `totalBadDebtEth` will grow or shrink according to the change of the userBadDebtPosition.
        _newBadDebt > currentBadDebt
            ? _increaseTotalBadDebt(_newBadDebt - currentBadDebt)
            : _decreaseTotalBadDebt(currentBadDebt - _newBadDebt);
    }
}

2. On the WISE_SECURITY.checkBadDebtLiquidation() function, do the next changes

function checkBadDebtLiquidation(
    uint256 _nftId
)
    external
    onlyWiseLending
{

    ...

    //@audit-info => If the total leftover debt is greater than the total leftover collateral, bad debt has been generated!
    unchecked {
        uint256 diff = totalBorrow
            - bareCollateral;

+       FEE_MANAGER.registerBadDebt(_nftId,diff);

-       FEE_MANAGER.increaseTotalBadDebtLiquidation(
-         diff
-       );

-       FEE_MANAGER.setBadDebtUserLiquidation(
-         _nftId,
-         diff
-       );
    }
}

3. The FEE_MANAGER.increaseTotalBadDebtLiquidation() function && the FEE_MANAGER.setBadDebtUserLiquidation() function can now be deleted from the FeeManager contract. They will not be used anymore. The new function will be in charge of tracking and updating the total bad debt as well as the individual user's bad debt.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L129 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L172 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L560-L573

Vulnerability details

Impact

PowerFarm positions that borrowed aWETH can't be liquidated.

Proof of Concept

PowerFarm positions can only be liquidated through the PendlePowerFarm.liquidatePartiallyFromToken() function. If a PowerFarm position is attempted to be liquidated directly in WiseLending by using the WiseLending.liquidatePartiallyFromTokens() function, the execution will revert because the call to the _checkPositionLocked() function, this check validates that locked functions can't be liquidated directly on WiseLending!

function liquidatePartiallyFromTokens(
    uint256 _nftId,
    uint256 _nftIdLiquidator,
    ...
)
    ...
{
    ...

    //@audit-ok => Allows to liquidate positions that are not locked for a farm
    //@audit-info => If the nftId IS locked for a farm, reverts
    _checkPositionLocked(
        _nftId,
        msg.sender
    );
}

function _checkPositionLocked(
    uint256 _nftId,
    address _caller
)
    internal
    view
{
    //@audit-info => If the caller is a powerFarm, returns, skipped the check of positionLocked
    //@audit-info => If the caller is anybody else, check if the position is locked!
    if (_byPassCase(_caller) == true) {
        return;
    }

    //@audit-info => If the nftId IS NOT locked for a farm, returns
    if (positionLocked[_nftId] == false) {
        return;
    }

    //@audit-info => If the nftId IS locked for a farm, reverts
    revert PositionLocked();
}

Knowing that PowerFarm positions can't be liquidated directly in WiseLending, let's explore the reason why PowerFarm positions that borrowed aWETH can't be liquidated.

When users enter a farm they have two options to select the token that will be borrowed to repay the flashloan to the balancer vault.

1. The first option is to borrow aWETH from WiseLending via the AaveHub.
2. The second option is to borrow WETH directly from WiseLending.

Users control what type of token they want to borrow (either aWETH or WETH), by setting as true or false the parameter _isAave when entering a farm.

• If set to true, the borrowedToken will be aWETH.
• If set to false, the borrowedToken will be WETH.

When a user enters a farm, the function executes the next couple of steps:

1. The first step is that the PowerFarm mints a new position, locks it in WiseLending and approve the AaveHub contract to operate on the new-minted position.

• In this step, it is important to emphasize that the ID that is assigned to this new position is based on the totalReserved and totalSupply() of positions as per the PositionNFTs contract's state.
  • The PositionNFTs contract is also used to mint positions for users who use the WiseLending contract as well as the AaveHub, these positions are not isolated/reserved to be used only by PowerFarms.
    • This means that the ID that will be assigned for a new position when entering a farm will vary depending on the rest of the existing positions on the PositionNFTs contract.

function _getWiseLendingNFT()
    internal
    returns (uint256)
{
    //@audit-info => When there are no available nfts!
    if (availableNFTCount == 0) {
        //@audit-info => Mints a new position. The owner of the position will be the PowerFarm!
        uint256 nftId = POSITION_NFT.mintPosition();
        ...
        return nftId;
    }

    ...
}

2. The second step is to transfer the amount that the user will use to enter the farm.
3. The third step is open a position, it will request a flashloan from the Balancer_Vault for the total leveraged amount that the position will use, it will use the flashloaned tokens to deposit and add liquidity into a Pendle Market, then it will deposit the received LPTokens from Pendle onto the PENDLE_CHILD. After depositing into the PENDLE_CHILD it will deposit the PENDLE_CHILD_Tokens into WiseLending, this deposit will be used as collateral for a borrow that the position will take to get the required tokens to payback the liquidator

• The value of the _isAave parameter plays a key role in this step:
  • if it is set to true, the borrowedTokens will be aWETH
    • Borrowing aWETH means that the position will end up owning borrowShares on the aWETH pool.
  • if it is set to false, the borrowedTokens will be WETH
    • Borrowing WETH means that the position will end up owning borrowShares on the WETH pool.

function _borrowExactAmount(
    bool _isAave,
    uint256 _nftId,
    uint256 _totalDebtBalancer
)
    internal
{
    if (_isAave == true) {
        AAVE_HUB.borrowExactAmount(
            _nftId,
            WETH_ADDRESS,
            _totalDebtBalancer
        );

        return;
    }

    WISE_LENDING.borrowExactAmount(
        _nftId,
        WETH_ADDRESS,
        _totalDebtBalancer
    );
}

4. The 4th step is to check the debt ratio to validate that the position has a debt ratio under 100%
5. The 5th step is to repay the flashloan (including the fees)
6. The 6th step is a fundamental process on the logic of the powerfarms. In this step it is reserved the key that is assigned to the position. Or in other words, the WiseLendingNFT it is linked to a new key that is generated based on the totalMinted and totalReserved keys in the PowerFarm.

• Thanks to this step is possible to identify who owns which WiseLendingNFTs on a PowerFarm.

function _reserveKey(
    address _userAddress,
    uint256 _wiseLendingNFT
)
    internal
    returns (uint256)
{
    ...

    //@audit-info => Gets the nextReserveKey!
    uint256 keyId = _getNextReserveKey();

    //@audit-info => The keyId is assigned as the reservedKey of the user!
    reservedKeys[_userAddress] = keyId;
    //@audit-info => The wiseLendingNFTId is assigned as the associated position of the keyId!
    farmingKeys[keyId] = _wiseLendingNFT;

    return keyId;
}

function _getNextReserveKey()
    internal
    returns (uint256)
{
    return totalMinted + _incrementReserved();
}

7. And finally, the last step, and actually, the step where the bug exists is when flagging if the borrowedTokens were aWETH or WETH tokens.

• This step is crucial because the state variable isAave is used to determine if the borrow was taken using the AaveHub (aWETH) or directly from WiseLending (WETH), and, depending on the type of borrowed assets, the PendlePowerFarmMathLogic._checkDebtRatio() function will determine if the position can be liquidated or not.

The existing implementation uses the keyId to flag if the borrowedTokens were aWETH or WETH. The problem with this approach is that anywhere in the rest of the codebase the value that is used to read the information of the isAave state variable is the nftId instead of the keyId.

• Revisiting step2 & step6, the nftId depends on the PositionNFTs contract's state, and the keyId depends solely on the PowerFarm's contract state.

Let's do an example of UserA entering a farm and borrowing the funds by using the AaveHub:

1. A new WiseLendingNFT is minted for this position, such a new nft is assigned the ID 10
2. The function executes all the logic to open a position and takes the borrow to repay the flashloan by using the AaveHub
3. The keyId for this position is generated and the position gets assigned the keyId 3.
4. The isAave uses the value of the keyId to flag that the borrow was made using AaveHub (aWETH tokens were borrowed)

• Notice here how the keyId of value 3 is the one flagged as if it would have taken a borrow using the AaveHub, while the nftId of value 10 is left unassigned

function enterFarm(
    //@audit-info => If true, the borrow will use the AaveHub and will borrow aWETH
    //@audit-info => If false, the borrow will borrow WETH directly from WiseLending
    bool _isAave,
    uint256 _amount,
    uint256 _leverage,
    uint256 _allowedSpread
)
    ...
{
    //@audit-info => Step 2, mints a new position that will be used to take the borrow and deposit the purchased PENDLE_CHILD shares
    uint256 wiseLendingNFT = _getWiseLendingNFT();

    //@audit-info => Transfers from the caller the amount of WETH that will be used to enter the farm!
    _safeTransferFrom(
        WETH_ADDRESS,
        msg.sender,
        address(this),
        _amount
    );

    //@audit-info => Steps 3, 4 & 5
    _openPosition(
        ...
    );

    //@audit-info => Step 6, generates the key that will be associated to the WiseLendingNFT.
    uint256 keyId = _reserveKey(
        msg.sender,
        wiseLendingNFT
    );

    //@audit-issue => The `keyId` is used to flag that the borrow was made using the AaveHub.
    //@audit-issue => Not using the `WiseLendingNFT`. In the rest of the codebase, whenever reading the `isAave` variable, the value that is used is the `positionNFT`.
    isAave[keyId] = _isAave;

    ...
}

Now that the user has entered a farm by borrowing the leveraged funds using the AaveHub, let's see why this type of position can't be liquidated.

• PowerFarm positions can only be liquidated through the PendlePowerFarm.liquidatePartiallyFromToken() function. When this function is executed, before doing anything it checks the debtRatio of the specified WiseLendingNFTId to be liquidated, the execution reverts if the check of the debt ratio returns true.
  • The PendlePowerFarmMathLogic._checkDebtRatio() function, reads the amount of borrowShares the position owes on the aWETH pool if the isAave variable is flagged as true, else, it will read the amount of borrowShares the position owes on the WETH pool
  • Here comes into play how the isAave variable was flagged when the user entered the farm. The isAave variable was set to true for the keyId, which was generated with a value of 3, but the WiseLendingNFTID with a value of 10 was not flagged as if it would have taken a borrow using the AaveHub.
  • The checkDebtRatio reads the value of the flag for the WiseLendingNFT, it doesn't read the flag of the keyId. Or in other words, it will read like this isAave[10] (The WiseLendingNftId), instead of isAave[3] (The keyId).
    • Because isAave is reading using the ID of the WiseLendingNFT (10), the returned value won't be the real flag that was assigned to this position, the value would be returned as false (default value), unless the keyId 10 also borrowed aWETH (because of how the isAave variable is flagged when entering a position!).
      • Because the flag returns false, the function will read the number of borrowShares the position owes on the WETH pool, even though the loan was taken on the aWETH pool.. Because the borrow was made on the aWETH pool, the total amount of borrowShares owed on the WETH pool will be returned as 0, which will cause the checkDebtRatio to return true, which will cause the liquidation execution to revert

function liquidatePartiallyFromToken(
    //@audit-info => WiseLendingNftId been liquidated
    uint256 _nftId,
    ...
)
    ...
{
    return _coreLiquidation(
        //@audit-info => WiseLendingNftId been liquidated
        _nftId,
        _nftIdLiquidator,
        _shareAmountToPay
    );
}


function _coreLiquidation(
    uint256 _nftId,
    ...
)
    ...
{
    //@audit-info => Forwards the nftID of the position being liquidated!
    if (_checkDebtRatio(_nftId) == true) {
        revert DebtRatioTooLow();
    }
}


function _checkDebtRatio(
    //@audit-info => WiseLendingNftId been liquidated
    uint256 _nftId
)
    internal
    view
    returns (bool)
{
    //@audit-info => Using the WiseLendingNFT (position's ID) will read if the loan was taken using the AaveHub (aWETH) or directly from WiseLending (WETH)

    //@audit-issue => When the position was opened, the `isAave` used the `keyId` to flag that the loan was made using the AaveHub, but here is using the `nftId` to read the value of `isAave`. Using our example, `isAave[3]` (The keyId) was flagged as a position that took a aWETH loan, but `isAave[10]` (The nftId) is not flagged, so, it will return the default value, false!
      //@audit-issue => Because the flag is read as false, the function will read the borrowShares owed by the position on the WETH pool, instead of reading the borrowShares on the aWETH pool.
      //@audit-issue => The position has no loans on the WETH pool, it has all the loans on the aWETH, thus, the borrowShares will be 0.
    
    uint256 borrowShares = isAave[_nftId]
        //@audit-info => Loads all the borrowShares owed by the position on the aWETH pool
        ? _getPositionBorrowSharesAave(
            _nftId
        )
        //@audit-info => Loads all the borrowShares owed by the position on the WETH pool
        : _getPositionBorrowShares(
            _nftId
    );

    //@audit-issue => Because the borrowShares were 0, it will simply return true, it doesn't check the actual debtRatio!
    //@audit-issue => Even though the position still owes the borrow that was taken on the aWETH pool, the debt ratio will return true
    if (borrowShares == 0) {
        return true;
    }

    return getTotalWeightedCollateralETH(_nftId)
        >= getPositionBorrowETH(_nftId);
}

Extra Side Effects

1. Manually payback shares won't work because the pool where the repayment should be done will be set as the WETH pool, instead of the aWETH pool.
2. Manually withdrawing shares will erroneously compute the debtRatio, which could cause key owners to withdraw more shares than what they should be allowed to.

Coded PoC

I coded a PoC to demonstrate that the keyId and the positionNFT can have different values and that a PowerFarm position is considered to be locked in WiseLending. This demonstrates that using the keyId to flag the isAave state variable will behave as described in the Proof of Concept section, and that the only way to liquidate PowerFarm position is by using the PendlePowerFarm.liquidatePartiallyFromToken() function

• Add the next test to the test file PendlePowerFarmControllerBase.t.sol:

function testFarmShouldEnterPoC()
    public
    cheatSetup(true)
{

    uint256 keyID = powerFarmManagerInstance.enterFarm(
        false,
        1 ether,
        15 ether,
        entrySpread
    );

    uint256 wiseLendingNFT = powerFarmManagerInstance.farmingKeys(keyID);

    console.log("wiseLendingNFT:" , wiseLendingNFT);
    console.log("keyID:" , keyID);
    console.log("=========");

    assert(keyID != wiseLendingNFT);

    //@audit-info => Validate the position is locked in WiseLending
    //@audit => If the position is locked, it can't be liquidated through the `WiseLending.liquidatePartiallyFromTokens()`
    //@audit => Locked functions can only be liquidated by the `WiseLending.coreLiquidationIsolationPools()`, which can only be called by a PowerFarm!
    bool positionLocked = wiseLendingInstance.positionLocked(wiseLendingNFT);
    console.log("Is position locked: ", positionLocked);
}

• Run the test with the below command:

forge test --match-test testFarmShouldEnterPoC --fork-url https://eth.llamarpc.com -vvv

[PASS] testFarmShouldEnterPoC() (gas: 24257021)
Logs:
  wiseLendingNFT: 7
  =========
  keyID: 1
  =========
  Is position locked:  true

Tools Used

Manual Audit

Recommended Mitigation Steps

The recommendation to fully mitigate this problem is to standardize the type of value that is used to flag the isAave variable, either using only the keyId or the WiseLendingNFT. Additionally, make sure to flag the isAave variable before calling the _openPosition().

• The below code changes propose a mitigation by standardizing the usage of the WiseLendingNFT associated with the keyId to flag and read the isAave variable. Also, it could be possible to standardize by using the keyId, this would require updating all the places where the isAave variable is read to use the keyId instead of the WisLendingNFT associated with the keyId.

function enterFarm(
    ...
)
    ...
{
    uint256 wiseLendingNFT = _getWiseLendingNFT();

    ...

    //@audit => Use the value of the `WiseLendingNFT` to flag the `isAave` variable!
    //@audit => In this way, `isAave` will be correctly flagged using the positionNFT instead of the keyId!
+   isAave[wiseLendingNFT] = _isAave;

    _openPosition(
        ...
    );

    uint256 keyId = _reserveKey(
        msg.sender,
        wiseLendingNFT
    );

-   isAave[keyId] = _isAave;

    ...
}

Apply the same fix on the PendlePowerManager.enterFarmETH() function

function enterFarmETH(
    ...
)
    ...
{

    uint256 wiseLendingNFT = _getWiseLendingNFT();

    ...

+   isAave[wiseLendingNFT] = _isAave;

    _openPosition(
        ...
    );

    uint256 keyId = _reserveKey(
        msg.sender,
        wiseLendingNFT
    );

-   isAave[keyId] = _isAave;

    ...
}

Also, make sure to update the below line in the PendlePowerManager.exitFarm() function:

    function exitFarm(
        ...
    )
        ...
    {
        uint256 wiseLendingNFT = farmingKeys[
            _keyId
        ];

        ...

        _closingPosition(
-           isAave[_keyId],
            //@audit-info => In this way, all the places on the codebase that reads the `isAave` variable will correctly be using the WiseLendingNFT instead of the keyId to determine the type of borrowedTokens (aWETH or WETH)
+           isAave[wiseLendingNFT],
            wiseLendingNFT,
            _allowedSpread,
            _ethBack
        );

        ...
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L194-L199 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L293-L308

Vulnerability details

Impact

When stETH depegs from ETH, the swaps on Curve will revert due to requesting a higher amountOut than what the curves pool will give.

Proof of Concept

When exiting a farm on mainnet, the requested tokensOut is set as stETH for redeeming the SY tokens on the PENDLE_SY contract. Once the PowerFarm has on its balance the stETH tokens, it does a swap from stETH to ETH using the Curves protocol.

The problem is that the implementation is assuming a peg of 1 ETH ~= 1 stETH. Even though both tokens have a tendency to keep the peg, this hasn't been always the case as it can be seen in this dashboard. There have been many episodes of market volatility that affected the price of stETH, notably the one in last June when stETH traded at ~0.93 ETH.

When computing the _minOutAmount, the PowerFarm calculates the ethValue of the received stETH tokens by requesting the price of the WETH asset, and then it applies the reverAllowedSpread to finally determine the _minOutAmount of ETH tokens that will be accepted from the swap on Curves

• The WETH price is pegged 1:1 to ETH, 1 WETH will always be 1 ETH, but, using the price of WETH to determine the minOutAmount is problematic, because, as seen in the dashboard, historically, 1 stETH has deppeged from 1:1 from ETH
  • Example: Assume a user sets a slippage of 1%. If stETH were to depeg to 0.95 per ETH, when swapping it would try to make sure that the user received at least 0.99 ETH. Whenever trying to swap it will revert because curves can't give the requested minOutAmount of ETH tokens, it could give at most 0.95 ETH per stETH.

function _logicClosePosition(
    ...
)
    private
{
    ...

    //@audit-info => When exiting a farm on mainnet, the `tokenOut` is set to be `stETH`
    address tokenOut = block.chainid == 1
        ? ST_ETH_ADDRESS
        : ENTRY_ASSET;

    ...

    uint256 ethAmount = _getEthBack(
        tokenOutAmount,
        _getTokensInETH(
            //@audit-info => When exiting a farm on mainnet, the price of the `tokenOut` is requested as if it were the price of `WETH`
            //@audit-issue => Here is where the code assumes a peg of `1 stETH to be 1 ETH`
            block.chainid == 1
                ? WETH_ADDRESS
                : ENTRY_ASSET,
            tokenOutAmount
        )
            * reverseAllowedSpread
            / PRECISION_FACTOR_E18
    );

    ...
}

function _getEthBack(
    uint256 _swapAmount,
    uint256 _minOutAmount
)
    internal
    returns (uint256)
{
    if (block.chainid == ETH_CHAIN_ID) {
        //@audit-info => Does a swap of stETH for ETH on the Curves exchange
        return _swapStETHintoETH(
            _swapAmount,
            _minOutAmount
        );
    }

    ...
}

function _swapStETHintoETH(
    uint256 _swapAmount,
    uint256 _minOutAmount
)
    internal
    returns (uint256)
{
    return CURVE.exchange(
        {
            fromIndex: 1,
            toIndex: 0,
            exactAmountFrom: _swapAmount,
            //@audit-info => minimum amount of ETH that the PowerFarm will accept for swapping `exactAmountFrom` of `stETH` tokens!
            minReceiveAmount: _minOutAmount
        }
    );
}

Tools Used

Manual Audit & H-06 finding on Asymmetry Finance contest & Asymmetry's mitigation review & Asymmetry's mitigation review

Recommended Mitigation Steps

The recommendation would be to implement a mitigation similar to the one implemented on the referenced issues. Basically, fetch the current price of stETH from a Chainlink Oracle and multiply the minOutAmount by the current price of stETH. In this way, the minOutAmount that is sent to the Curves exchange will now be within the correct limits based on the current price of stETH.

• Also, make sure to multiply the ethValueBefore by the current price of stETH (Only when exiting farms on mainnet). In this way, both amounts, ethValueAfter and ethValueBefore will be computed based on the current price of stETH, allowing the slippage to validate that no ethValue was lost during the process of removing liquidity from pendle, redeeming the sy tokens and swapping on curves. In the end, both, ethValueAfter and ethValueBefore will represent the ethValue based on the stETH price.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L263-L265 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L125-L128

Vulnerability details

Impact

Users can withdraw uncollateralized deposits even though their position is liquidable, as opposed to the README, if the position is in liquidation mode, users should use their uncollateralized deposits to avoid liquidation instead of removing them

Proof of Concept

When withdrawing deposits from public pools, at the end of the tx is executed the WiseLending._healthStateCheck() function, which depending on the value of the powerFarmCheck will determine if the position's collateral is enough to cover the borrows.

• If powerFarmCheck is true, it will use the bare value of the collateral, meaning, the collateralFactor is not applied to the collateral's value
• If powerFarmCheck is false, it will use the weighted value of the collateral, meaning, the collateralFactor is applied to the collateral's value.

When withdrawing an uncollateralized deposit, the WiseCore._coreWithdrawToken() function calls the WiseSecurity.checksWithdraw() function to determine the value of the powerFarmCheck, if the pool from where the tokens are being withdrawn is uncollateralized, the powerFarmCheck will be set to true, which will cause that the WiseLending._healthStateCheck() function uses the bare value of the full collateral to determine if the collateral can cover the existing borrows.

• Using the bare value of the collateral does not accurately reflect if a position is liquidable or not, the liquidation's logic in the WiseSecurity.checksLiquidation() function uses the weightedCollateral instead of the bareCollateral to determine if the position is liquidable or not. So, this difference to determine if the position's collateral can cover the borrows when withdrawing uncollateralized deposits and when doing liquidation causes a discrepancy to allow the withdrawal of uncollateralized deposits even though the position is in liquidation mode.

For example, a user requests a withdrawal of an uncollateralized deposit in a position with the below values:

• 1.5e18 ETH of bare collateral
• 1.2e18 ETH of weightedCollateral
• 1.3e18 ETH of borrows
  • The withdrawal will be allowed even though the position is in liquidation mode. Because of the powerFarmCheck being set to true, the WiseLending._healthStateCheck() function will check if 95% of the bareCollateral can cover the borrows, 95% of 1.5e18 ETH would be 1.425e18 ETH, thus, the withdrawal will be possible, even though the position is in liquidation mode.

WiseSecurity.sol

function checksWithdraw(
    ...
)
    ...
{
    ...

    if (_isUncollateralized(_nftId, _poolToken) == true) {
        return true;
    }

    ...
}

function _getState(
    uint256 _nftId,
    bool _powerFarm
)
    internal
    view
    returns (bool)
{
    ...

    //@audit-info => If `powerFarmCheck` is true, overalCollateral will be computed using the value of the `bareCollateral`
    //@audit-info => If `powerFarmCheck` is false, overalCollateral will be computed using the value of the `weightedCollateral`

    uint256 overallCollateral = _powerFarm == true
        ? overallETHCollateralsBare(_nftId)
        : overallETHCollateralsWeighted(_nftId);

    //@audit-info => If 95% of the overalCollateral > borrowAmount, withdrawal will be allowed!
    return overallCollateral
        * BORROW_PERCENTAGE_CAP
        / PRECISION_FACTOR_E18
        < borrowAmount;
}

• Now, when using the same values but for a liquidation, we have that the weightedCollateral is not enough to cover the borros, thus, the position is liquidable.

WiseSecurity.sol

function checksLiquidation(
    ...
)
    external
    view
{
    ...

    //@audit-info => When doing liquidations, the value of the `weightedCollateral` is used to determine if the position is liquidable or not!
    canLiquidate(
        borrowETHTotal,
        weightedCollateralETH
    );

    ...
}

Tools Used

Manual Audit

Recommended Mitigation Steps

If the protocol wants to enforce that users use their uncollateralized deposits to avoid liquidations when the positions are liquidable, don't set to true the powerFarmCheck when doing withdrawals for uncollateralized deposits. Allow the WiseLending._healthStateCheck() function to validate if the position is indeed in liquidation mode by using the weightedCollateral instead of the bareCollateral value.

WiseSecurity.sol

function checksWithdraw(
    ..
)
    ..
{
    ...

-   if (_isUncollateralized(_nftId, _poolToken) == true) {
-       return true;
-   }

    ...
}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurityHelper.sol#L895-L899

Vulnerability details

Impact

Liquidations can be DoSed which increments the risk of bad debt being generated on position.

Proof of Concept

When a liquidator is liquidating a position in WiseLending, the liquidator needs to specify the amount of shares to be repaid. The liquidation logic checks if the positions are indeed liquidable, if so, it validates if the number of shares to be liquidated exceeds the total amount of shares that can be liquidated by using the WiseSecurityHelper.checkMaxShares() function. If the amount of shares the liquidator intends to liquidate exceeds the maximum amount of liquidable shares, the execution is reverted

• When the position has generated bad debt, the liquidation can liquidate all the user's borrowShares on the pool been liquidated
• When the position has not generated bad debt, the maximum amount of liquidable shares is 50% of the existing user's borrowShares on the pool being liquidated

The problem with this approach is that borrowers can frontrun the liquidation and repay as little as 1 share of the existing debt on the same pool that the liquidator decided to liquidate the debt from. This will cause when the liquidation is executed, the total borrow shares of the user on the pool being liquidated to be lower. If the liquidator was trying to liquidate the maximum possible amount of shares, now, the maxShares that can be liquidated will be slightly less than the amount of shares that the liquidator specified, which will cause the tx to revert.

function checkMaxShares(
    uint256 _nftId,
    address _tokenToPayback,
    uint256 _borrowETHTotal,
    uint256 _unweightedCollateralETH,
    uint256 _shareAmountToPay
)
    public
    view
{
    //@audit-ok => total borrowShares a position owns for a _tokenToPayback pool
    uint256 totalSharesUser = WISE_LENDING.getPositionBorrowShares(
        _nftId,
        _tokenToPayback
    );

    //@audit-info => If baddebt, maxShares that can be liquidated are the totalSharesUser
    //@audit-info => If not baddebt, maxShares can be 50% of the total borrowShares
    uint256 maxShares = checkBadDebtThreshold(_borrowETHTotal, _unweightedCollateralETH)
        ? totalSharesUser
        : totalSharesUser * MAX_LIQUIDATION_50 / PRECISION_FACTOR_E18;

    if (_shareAmountToPay <= maxShares) {
        return;
    }

    //@audit-issue => reverts if the amount of shares to be repaid exceeds maxShares!
    revert TooManyShares();
}

For example, if there is a position in a liquidable state that has 100 borrowShares on the PoolA, and, a liquidator decides to liquidate the maximum possible amount of shares from this position it will send a tx to liquidate 50 shares from that position on the PoolA. The position's owner can use the WiseLending.paybackExactShares() function to repay 1 share and frontrun the liquidator's tx. Now, when the liquidator's tx is executed, the position is still liquidable, but, it only has 99 borrowShares on the PoolA, as a result of this, the WiseSecurityHelper.checkMaxShares() function will determine that the maximum possible amount of liquidable shares is 49.5, and, because the liquidator specified that he intended to liquidate 50 shares, the tx will be reverted.

Tools Used

Manual Audit

Recommended Mitigation Steps

In the WiseSecurityHelper.checkMaxShares() function, if the _shareAmountToPay exceeds maxShares, don't revert, re-adjust the number of shares that can be liquidated. Return the final value of _shareAmountToPay and forward it back to the WiseLending.liquidatePartiallyFromTokens() function. Then, use the final value of shareAmountToPay to compute the exact amount of tokens to be repaid in the WiseLending.liquidatePartiallyFromTokens() function.

WiseSecurity.sol

function checksLiquidation(
    ...
    uint256 _shareAmountToPay
)
    external
    view
+   returns (uint256)
{
    ...


-   checkMaxShares(
+   return checkMaxShares( 
        _nftIdLiquidate,
        _tokenToPayback,
        borrowETHTotal,
        unweightedCollateralETH,
        _shareAmountToPay
    );
}

WiseSecurityHelper.sol

function checkMaxShares(
    ...
    uint256 _shareAmountToPay
)
    public
    view
+   returns (uint256)    
{
    ...

    uint256 maxShares = checkBadDebtThreshold(_borrowETHTotal, _unweightedCollateralETH)
            ? totalSharesUser
            : totalSharesUser * MAX_LIQUIDATION_50 / PRECISION_FACTOR_E18;

    if (_shareAmountToPay <= maxShares) {
-       return;
+       return _shareAmountToPay;
+   } else {
+       return maxShares;
+   }

-   revert TooManyShares();
}

WiseLending.sol

function liquidatePartiallyFromTokens(
    ...
    uint256 _shareAmountToPay
)
    ...
{
    ...

-   data.shareAmountToPay = _shareAmountToPay;

    ...

    //@audit-info => First, determine the maximum amount of liquidable shares
+   data.shareAmountToPay = WISE_SECURITY.checksLiquidation(
+     _nftId,
+     _paybackToken,
+     _shareAmountToPay
+   );

    //@audit-info => Then, compute the exact amount of tokens required to liquidate the final amount of liquidable shares
    data.paybackAmount = paybackAmount(
        _paybackToken,
-       _shareAmountToPay
+       data.shareAmountToPay
    );

    ...

-   WISE_SECURITY.checksLiquidation(
-       _nftId,
-       _paybackToken,
-       _shareAmountToPay
-   );

    ...
}

Assessed type

Context