c4-bot-10 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L464-L483 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmMathLogic.sol#L385-L410

Vulnerability details

Impact

Entering powerFarms with high leverage will fail most of the time when the Balancer starts taking fees for flash loans. There are no fees activated for now but the fact that there is such a functionality available makes the scenario possible.

Actually, WiseLending is aware of the possible flash loan fees but due to Pendle swaps and their slippage (0 when swapping SY for LP tokens) + lending share price ratio, borrowing just enough WETH in order to repay the loan + fees will fail.

Proof of Concept

Let’s consider the example where the user wants to enter powerFarm with initialAmount of 1 ENTRY_ASSET and leverage of 14x, the amount that should be borrowed is 13 WETH.

In PendlePowerFarmLeverageLogic::receiveFlashLoan we can observe that totalDebtBalancer is set to: _flashloanAmounts[0] + _feeAmounts[0], indeed protocol assumes that there will be fees in the future but due to the many swaps before borrowing WETH from the poolMarket, and depending on the allowedSpread _depositAmount can a significantly lower number than intended.

We can follow up on where potential slippages will occur:

on Arbitrum swap from WETH to ENTRY_ASSET will be needed, with slippage up to _allowedSpread.
depositing ENTRY_ASSET in exchange for StandartizedYield tokens from Pendle is also a subject of slippage.
adding SY tokens in order to receive LP tokens from Pendle even has no slippage tolerance specified, accepting any amount of LP tokens:

PendlePowerFarmLeverageLogic.sol#L464-L483

(uint256 netLpOut,) = PENDLE_ROUTER.addLiquiditySingleSy({
        _receiver: address(this),
        _market: address(PENDLE_MARKET),
        _netSyIn: syReceived,
        _minLpOut: 0, //@audit accepting any amount of LP tokens
        _guessPtReceivedFromSy: ApproxParams({
            guessMin: netPtFromSwap - 100,
            guessMax: netPtFromSwap + 100,
            guessOffchain: 0,
            maxIteration: 50,
            eps: 1e15
        })
    });

After receiving LP tokens, they are being deposited in PendlePowerFarmToken which will progressively lose value because of either users exchanging them for PendleTokens with added incentive or the ever increasing _underlyingLpAssetsCurrent due to the reward distribution.

After 4 swaps, the value lost to slippages will definitely reduce the receivedShares that will be deposited in the PENDLE_CHILD pool market, and the maximum borrowable amount of WETH tokens to be able to repay the flash loan.

Function execution will fail in PendlePowerFarmMathLogic::_checkDebtRatio because of the weighted deposit amount being less than the borrows, resulting in a unhealthy position.

PendlePowerFarmMathLogic.sol#L385-L410

/**
 * @dev Internal function checking if a position
 * with {_nftId} has a debt ratio under 100%.
 */
function _checkDebtRatio(uint256 _nftId) internal view returns (bool) {
    uint256 borrowShares = isAave[_nftId] ? _getPositionBorrowSharesAave(_nftId) : _getPositionBorrowShares(_nftId);

    if (borrowShares == 0) {
        return true;
    }

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

Same situation can be observed in closePosition flow, performing the exact same checks + Curve exchange taking fees on stETH ⇒ ETH swaps.

Tools Used

Manual Review

Recommended Mitigation Steps

Apply strict slippage parameters where they are missing - addLiquiditySingleSy, also in case Balancer starts taking flash loans fee, consider implementing functionality to reduce the amount of the flash loan and rely more on the user supplied initial deposit.

Assessed type

DoS

GalloDaSballo commented 6 months ago

This is at the edge of integration risks, but is worth reviewing

See docs on Balancer fees: https://docs.balancer.fi/reference/contracts/flash-loans.html

c4-pre-sort commented 6 months ago

GalloDaSballo marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

GalloDaSballo marked the issue as primary issue

vonMangoldt commented 6 months ago

No problem since we have ethValueBefore and after and can just redeploy. Therfor invalid

trust1995 commented 6 months ago

Agree that this is not a major enough deal to be concerned with at this moment.

c4-judge commented 6 months ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

trust1995 marked the issue as grade-c

blckhv commented 6 months ago

Hey, @trust1995,

In my opinion, relying on the fact that Balancer won't enable fees on their side is not enough for this submission to be invalidated, the fact that flash fees are mentioned in their documentation, this scenario is likely to happen at any given moment. Then high leverage positions for both entering and exiting farms will fail, as stated in my submission.

No problem since we have ethValueBefore and after and can just redeploy. Therfor invalid

This statement is not entirely right, since all the PendlePowerFarmTokens are minted to msg.sender here and redeployment will only cause more damage since all the tokens + funds of the users who entered the power farms will be stuck in the contract without a way to be reclaimed. Indeed the inability to exit high leveraged farm positions is a concern mainly for the users interacting through EOAs, which will be the vast majority of participants.

Thanks.

trust1995 commented 6 months ago

This statement is not entirely right, since all the PendlePowerFarmTokens are minted to msg.sender here and redeployment will only cause more damage since all the tokens + funds of the users who entered the power farms will be stuck in the contract without a way to be reclaimed.

Can you provide evidence for the statement? Secondly, issues about withdrawal were not mentioned in the original submission. The impact of botched deposits != stuck withdrawals.

blckhv commented 6 months ago

Last sentence in my Proof of Concept:

Same situation can be observed in closePosition flow, performing the exact same checks + Curve exchange taking fees on stETH ⇒ ETH swaps.

All the position NFT's are minted to the contract, not to the user, only keys, which are tied to the nft belong to the users:

PendlePowerManager.sol

function _getWiseLendingNFT() internal returns (uint256) {
        if (availableNFTCount == 0) {
            uint256 nftId = POSITION_NFT.mintPosition();

            _registrationFarm(nftId);

            POSITION_NFT.approve(AAVE_HUB_ADDRESS, nftId);

            return nftId;
        }

        return availableNFTs[availableNFTCount--];
    }

PositionNFTs.sol

function mintPosition() external returns (uint256) {
        return _mintPositionForUser(msg.sender);
    }

PendlePowerFarmTokens will be minted to the msg.sender when opening farm position which is the PendlePowerManager contract:

PendlePowerFarmLeverageLogic.sol

 function _logicOpenPosition(
        bool _isAave,
        uint256 _nftId,
        uint256 _depositAmount,
        uint256 _totalDebtBalancer,
        uint256 _allowedSpread
    ) internal {
        ..MORE CODE
        (uint256 receivedShares,) = IPendleChild(PENDLE_CHILD).depositExactAmount(netLpOut); 
       ...MORE CODE

PendlePowerFarmToken.sol

function depositExactAmount(uint256 _underlyingLpAssetAmount) external syncSupply returns (uint256, uint256) {
...MORE CODE
        _mint(msg.sender, reducedShares);
...MORE CODE
    }

In case contract is redeployed LPs from Pendle won't be claimable: PendlePowerFarmLeverageLogic.sol

function _withdrawPendleLPs(uint256 _nftId, uint256 _lendingShares) private returns (uint256 withdrawnLpsAmount) {
    return IPendleChild(PENDLE_CHILD).withdrawExactShares(
        WISE_LENDING.withdrawExactShares(_nftId, PENDLE_CHILD, _lendingShares)
    );
}

PendlePowerFarmToken.sol

function withdrawExactAmount(uint256 _underlyingLpAssetAmount) external syncSupply returns (uint256) {
...MORE CODE
    _burn(msg.sender, shares);

    _withdrawLp(msg.sender, _underlyingLpAssetAmount);
}

These are all consequences in case redeploy happens and the old PowerFarm is deprecated.

trust1995 commented 6 months ago

With the described evidence above, and the re-weighted balance of likelihood and severity, I will correct the verdict to Medium.

c4-judge commented 6 months ago

This previously downgraded issue has been upgraded by trust1995

c4-judge commented 6 months ago

trust1995 marked the issue as satisfactory

c4-judge commented 6 months ago

trust1995 marked the issue as selected for report

stalinMacias commented 6 months ago

Hello @trust1995 and @AydoanB , Can I ask what is the final reason for this report to be accepted, the claim that fees on balancer will disrupt entering or exiting?

When exiting a farm, the flashloan is repaid as the last step, when the liquidity has been withdrawn from the PendleLP, redeemed from the Pendle and swapped on curves, the flashloan (including fees (if any)) is repaid on the _clousingRouteToken() or _closingRouteETH(), in either of the two, the totalDebtBalancer is deducted from all the liquidity that was withdrew from pendle and swapped on curves. If execution reaches the closingRoute functions it means that the slippage enforced by the spread was fine, and at any point before the closingRoute functions, the balancer's fees are not even used.

Yes, the fees to repay the flashloan will be taken from the user's liquidity, but that is the correct behavior, otherwise, the user would have not been able to leverage his position.

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

    //@audit => Computes the eth value of the available liquidity before removing and redeeming liquidity from pendle, as well as before swapping on curves
        uint256 ethValueBefore = _getTokensInETH(
            PENDLE_CHILD,
            withdrawnLpsAmount
        );

    //@audit => removes and redeems liquidity from pendle, as well as swaps it on curves
        ....

    //@audit => Computes the eth value of the available liquidity after redeeming from pendle and swaping on curves.
    //@audit => Applies the spread to the full liquidity.
        uint256 ethValueAfter = _getTokensInETH(
            WETH_ADDRESS,
            ethAmount
        )
            * _allowedSpread
            / PRECISION_FACTOR_E18;

    //@audit => Validates if the liquidity considering the user's spread is within the acceptable range
        if (ethValueAfter < ethValueBefore) {
            revert TooMuchValueLost();
        }

    //@audit-info => At this point, the balancer's fees have not even been used 1 time, all the flashloan (including fees) will be repaid in the below functions

        if (_ethBack == true) {
            _closingRouteETH(
                ethAmount,
                _totalDebtBalancer,
                _caller
            );

            return;
        }

        _closingRouteToken(
            ethAmount,
            _totalDebtBalancer,
            _caller
        );
    }

@trust1995 I think there is not enough evidence yet to mark this report as a valid report.

trust1995 commented 6 months ago

@AydoanB what is your response?

blckhv commented 6 months ago

@trust1995, here it comes:

Pendle LPs will decrease over time due to being exchanged with an incentive in the PendlePowerFarmController contract:

After receiving LP tokens, they are deposited in PendlePowerFarmToken which will progressively lose value because of either the user exchanging them for PendleTokens with added incentive or the ever-increasing _underlyingLpAssetsCurrent due to the reward distribution.
All the flash loan amount is used to payback the shares so if we assume the contract's WETH balance before execution to be 0, virtually it will be 0 - flash loan fees after this line:
```
_paybackExactShares(_isAave, _nftId, _borrowShares); 
```
Then it is likely that all the spread specified will be used, due to MEVs making the ethAmount less than the flash loan + fees (as much as allowedSpread allows), which are not used anywhere as the warden also mentioned, but in this case they don't have to be touched, they effectively make the contract appear on doing all the operations between the receiving and repaying the flash loan, not the flash loan + fees.
Borrowers will have to pay interest, which increases the amount that has to be paid for the same borrower at the time of entering the pool making the difference between balance of user in terms of Wise pools and Pendle bigger.

A bigger spread will make it even worse since for receiving the exact amount of tokens that you've opened position against, in order to be able to repay the flash loan + fees, you have to specify extremely tight slippage, otherwise, you will receive ethAmount < totalDebtBalancer.

Given these facts, Medium severity is appropriate imo with the impact that can be caused by the sponsor's recommendation.

stalinMacias commented 6 months ago

Pendle LPs will decrease over time due to being exchanged with an incentive in the PendlePowerFarmController contract:

Where does this assumption come from? The value of the pendleLPs is queried from a custom oracle.

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

        //@audit => withdraws all the pendleLPs from the PendleChild contract
        uint256 withdrawnLpsAmount = _withdrawPendleLPs(
            _nftId,
            _lendingShares
        );

        //@audit => ETH value of the amount of underlyingLP tokens that were withdrawn from the PENDLE_CHILD token
        uint256 ethValueBefore = _getTokensInETH(
            PENDLE_CHILD,
            withdrawnLpsAmount
        );

    ...
}

they effectively make the contract appear on doing all the operations between the receiving and repaying the flash loan, not the flash loan + fees.

When the flashloan is repaid, it is repaid everything, flashloan and fees, the receiveFlashloan() function computes the total debt to be repaid to balancer, which includes the flashamount + feeAmounts, then this value is forwarded to the open/close logic position.

    function receiveFlashLoan(
        IERC20[] memory _flashloanToken,
        uint256[] memory _flashloanAmounts,
        uint256[] memory _feeAmounts,
        bytes memory _userData
    )
        external
    {
       ...

        //@audit-ok => Amount that needs to be repaid to the VAULT_BALANCER!
        uint256 totalDebtBalancer = _flashloanAmounts[0]
            + _feeAmounts[0];

        (
        ...

        if (initialAmount > 0) {
            _logicOpenPosition(
                isAave,
                nftId,
                _flashloanAmounts[0] + initialAmount,
                //@audit => total debt to repay to balancer, includes flashloan and fees
                totalDebtBalancer,
                allowedSpread
            );

            return;
        }

        _logicClosePosition(
            nftId,
            borrowShares,
            lendingShares,
            //@audit => total debt to repay to balancer, includes flashloan and fees
            totalDebtBalancer,
            allowedSpread,
            caller,
            ethBack,
            isAave
        );
}

Borrowers will have to pay interest, which increases the amount that has to be paid for the same borrower at the time of entering the pool

Yes, but the positions will also increase in value, they are trading the difference in yield when they open the position and when the position is closed. If the trading goes bad, i.e. the position loses value, then the position would be liquidated, but that's another story, for the matter of this report we are talking about a position that is healthy at the moment of being closed, so, the position itself generated more yield than the interest to be repaid.

in order to be able to repay the flash loan + fees, you have to specify extremely tight slippage, otherwise, you will receive ethAmount < totalDebtBalancer.

Then that means the spread will just need to be adjusted, which is a parameter provided by the users when the function is executed, in the end, the flashloan will be repaid (including fees) and the user will keep the difference between what was withdrawn and what's leftover after repaying the flashloan

trust1995 commented 6 months ago

@trust1995, here it comes:

Pendle LPs will decrease over time due to being exchanged with an incentive in the PendlePowerFarmController contract:

After receiving LP tokens, they are deposited in PendlePowerFarmToken which will progressively lose value because of either the user exchanging them for PendleTokens with added incentive or the ever-increasing _underlyingLpAssetsCurrent due to the reward distribution.

All the flash loan amount is used to payback the shares so if we assume the contract's WETH balance before execution to be 0, virtually it will be 0 - flash loan fees after this line:
 _paybackExactShares(_isAave, _nftId, _borrowShares); 
Then it is likely that all the spread specified will be used, due to MEVs making the ethAmount less than the flash loan + fees (as much as allowedSpread allows), which are not used anywhere as the warden also mentioned, but in this case they don't have to be touched, they effectively make the contract appear on doing all the operations between the receiving and repaying the flash loan, not the flash loan + fees.

Borrowers will have to pay interest, which increases the amount that has to be paid for the same borrower at the time of entering the pool making the difference between balance of user in terms of Wise pools and Pendle bigger.

A bigger spread will make it even worse since for receiving the exact amount of tokens that you've opened position against, in order to be able to repay the flash loan + fees, you have to specify extremely tight slippage, otherwise, you will receive ethAmount < totalDebtBalancer.

Given these facts, Medium severity is appropriate imo with the impact that can be caused by the sponsor's recommendation.

You outline a scneario where position is at a loss, so it presumably would have been liquidated. I will require evidence in the form of a coded POC to assess if there is an unintended scenario at play here.

c4-judge commented 6 months ago

trust1995 marked the issue as not selected for report

c4-judge commented 6 months ago

trust1995 marked the issue as unsatisfactory: Insufficient proof

blckhv commented 6 months ago

Hey, @trust1995,

Due to the complexity of the system and the hardcoded Balancer contract address, I've chosen to modify the contracts for easier understanding, instead of managing everything from the test file:

Max slippage is used in the PendlePowerFarmLeverageLogic contract
Balancer Vault address is passed as a constructor argument instead of being hardcoded

You can verify the modifications and their validity in the original Wise Lending repo and validate that nothing irrelevant has been changed. Also you can track the changes with @audit tag Regarding the tests itself:

Tests that should be executed:

testFarmShouldEnterAndExitIntoTokenWithFees Expected outcome: [FAIL. Reason: DebtRatioTooHigh()] testFarmShouldEnterAndExitIntoTokenWithFees()
testFarmShouldEnterAndExitIntoTokenWithoutFees Expected outcome: [PASS] testFarmShouldEnterAndExitIntoTokenWithoutFees()

Notes: I want to emphasise that slippage of 1%, which is used by me for the Uniswap swap isn't something uncommon and actually most of the DEXes operate with > 0.5% and when fees are enabled users are forced to use minimal slippage.

Brief intro to the tests:

User enters farm with 14x leverage and 1 ETH initial amount, resulting in overall position of 15 ETH , flash loan will be 14 ETH.
Uniswap swap is simulated taking the entire slippage allowed (1%), other swaps are assumed to happen without any slippage.
Check for the ethValue lost passes succesfully:

if (ethValueAfter < ethValueBefore) {
        revert TooMuchValueLost();
    }

Instead of, the actual revert happens after borrowing totalDebtBalancer amount from Wise pool here, exactly as mentioned in my initial report, due to the fee amount added after flash loan has been received:

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

Link to the gist: https://gist.github.com/AydoanB/21496bf83731199634ac2a9e775745b7

trust1995 commented 6 months ago

Hey @AydoanB

This statement is not entirely right, since all the PendlePowerFarmTokens are minted to msg.sender here and redeployment will only cause more damage since all the tokens + funds of the users who entered the power farms will be stuck in the contract without a way to be reclaimed.

Can you provide evidence for the statement? Secondly, issues about withdrawal were not mentioned in the original submission. The impact of botched deposits != stuck withdrawals.

I was looking for a POC to show stuck withdrawals, which would merit H/M severity. Appreciate the time taken to PoC the issue but unless that is demonstrated it has to be treated as QA severity.

blckhv commented 6 months ago

@trust1995, I appreciate your statement and I also see now (after writing the POC) that the impact is not enough for H/M, so downgrading to QA and marking as valid will be fine. Thanks.

c4-judge commented 6 months ago

trust1995 removed the grade

c4-judge commented 6 months ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

trust1995 marked the issue as grade-c

Slavchew commented 6 months ago

Hi @trust1995, really sorry to post after PJQA, but shouldn't this be marked grade a and to remove unsatisfactory, if deemed valid but not sufficient for H/M.

There are also a few more issues that you have downgraded to QA, but they are all marked as unsatisfactory.

trust1995 commented 6 months ago

Hi @trust1995, really sorry to post after PJQA, but shouldn't this be marked grade a and to remove unsatisfactory, if deemed valid but not sufficient for H/M.

There are also a few more issues that you have downgraded to QA, but they are all marked as unsatisfactory.

Overall, the complete QA submission does not meet the bar for A/B. PJQA is over, thank you.

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

Using Pendle power farm will fail most of the times when Balancer starts taking fees on flash loans. #273

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type