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

8 stars 6 forks source link

[M-2] Users can withdraw shares before required checks revert #293

Closed c4-bot-1 closed 3 months ago

c4-bot-1 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L274-L302

Vulnerability details

Description:

Not following CEI. users an withdraw shares before the following check revert _checkDebtRatio(wiseLendingNFT) == false in PendlePowerManager::manuallyWithdrawShares

Impact:

This could lead to loss of funds since the user can withdraw funds before the the collateral debt ratio get checked. Users with bad debts can withdraw their funds wich is against the protocol logic.

Tools Used

Manual Review

Proof Of Concept:

    function manuallyWithdrawShares(
        uint256 _keyId,
        uint256 _withdrawShares
    )
        external
        updatePools
        onlyKeyOwner(_keyId)
    {
        uint256 wiseLendingNFT = farmingKeys[
            _keyId
        ];

        _manuallyWithdrawShares(
            wiseLendingNFT,
            _withdrawShares
        );

        if (_checkDebtRatio(wiseLendingNFT) == false) {
            revert DebtRatioTooHigh();
        }

        emit ManualWithdrawShares(
            _keyId,
            wiseLendingNFT,
            _withdrawShares,
            block.timestamp
        );
    }

Recommended Mitigation Steps:

Check the debt ratio before allowing manual withdrawals

    function manuallyWithdrawShares(
        uint256 _keyId,
        uint256 _withdrawShares
    )
        external
        updatePools
        onlyKeyOwner(_keyId)
    {
        uint256 wiseLendingNFT = farmingKeys[
            _keyId
        ];

-        _manuallyWithdrawShares(
-            wiseLendingNFT,
-            _withdrawShares
-        );

-        if (_checkDebtRatio(wiseLendingNFT) == false) {
-            revert DebtRatioTooHigh();
-        }

+        if (_checkDebtRatio(wiseLendingNFT) == false) {
+            revert DebtRatioTooHigh();
+        }

+        _manuallyWithdrawShares(
+            wiseLendingNFT,
+            _withdrawShares
+        );

        emit ManualWithdrawShares(
            _keyId,
            wiseLendingNFT,
            _withdrawShares,
            block.timestamp
        );
    }

Assessed type

ETH-Transfer

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as insufficient quality report

GalloDaSballo commented 4 months ago

Consider scrapping as severely overinflated

vonMangoldt commented 4 months ago

This change would actually make it explotiable since you would check before and then allow any withdraw creating bad debt.

vm06007 commented 3 months ago

Submitted must be disqualified.

c4-judge commented 3 months ago

trust1995 marked the issue as unsatisfactory: Invalid