Lack of slippage control in PendlePowerFarmToken and wrong projection of share prices due to missing sync in preview functions

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L334-L345
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L443-L492

Vulnerability details

Impact

PendlePowerFarmToken does not implement slippage control for the deposit/withdraw functions. Other projection functions related to those like previewMintShares() also lack a supply sync, which can mislead the depositor in the case where there was a prior reward distribution.

Proof of Concept

PendlePowerFarmToken is vulnerable to griefing through slippage (either intentional or unintentional). There are functions that allow the vault share holder to project the outcome of deposit and withdraw interactions. Those are:

• previewMintShares()
• previewAmountWithdrawShares()
• previewBurnShares()

The problem is that none performs a supply sync, so the amount the user will get from those may not correspond to the amount they will get after calling: depositExactAmount(), withdrawExactAmount(), or withdrawExactShares().

The vulnerability arises only when a reward distribution has happened before the victim's call. The supply sync is handled from the following function:

function _syncSupply()
    private
{
    uint256 additonalAssets = previewDistribution();

    if (additonalAssets == 0) {
        return;
    }

    underlyingLpAssetsCurrent += additonalAssets;
    totalLpAssetsToDistribute -= additonalAssets;
}

This is handled properly for the interaction functions but not for the preview ones. The supply should be synced in both places so the user can be more certain of the projected amount.

Here's, for instance, how the depositExactAmount() function looks:

function depositExactAmount(
    uint256 _underlyingLpAssetAmount
)
    external
    syncSupply
    returns (
        uint256,
        uint256
    )
{
    if (_underlyingLpAssetAmount == 0) {
        revert ZeroAmount();
    }

    uint256 shares = previewMintShares(
        _underlyingLpAssetAmount,
        underlyingLpAssetsCurrent
    );

    if (shares == 0) {
        revert NotEnoughLpAssetsTransferred();
    }

    uint256 reducedShares = _applyMintFee(
        shares
    );

    uint256 feeShares = shares
        - reducedShares;

    if (feeShares == 0) {
        revert ZeroFee();
    }

    if (reducedShares == feeShares) {
        revert TooMuchFee();
    }

    _mint(
        msg.sender,
        reducedShares
    );

    _mint(
        PENDLE_POWER_FARM_CONTROLLER,
        feeShares
    );

    underlyingLpAssetsCurrent += _underlyingLpAssetAmount;

    _safeTransferFrom(
        UNDERLYING_PENDLE_MARKET,
        msg.sender,
        PENDLE_POWER_FARM_CONTROLLER,
        _underlyingLpAssetAmount
    );

    return (
        reducedShares,
        feeShares
    );
}

The syncSupply modifier it uses already performs this and it is done before the function's body is entered, so the amount returned from previewMintShares() here will be correct:

modifier syncSupply()
{
    _triggerIndexUpdate();
    _overWriteCheck();
    _syncSupply();
    _updateRewards();
    _setLastInteraction();
    _increaseCardinalityNext();
    uint256 sharePriceBefore = _getSharePrice();
    _;
    _validateSharePriceGrowth(
        _validateSharePrice(
            sharePriceBefore
        )
    );
}

Coded POC (PendlePowerFarmControllerBase.t.sol):

function testVaultLacksSlippageControl() public normalSetup(true) {
    (IERC20 tokenReceived, uint256 balanceReceived) =
        _getTokensToPlayWith(CRVUSD_PENDLE_28MAR_2024, crvUsdMar2024LP_WHALE);
    (uint256 depositAmount, IPendlePowerFarmToken derivativeToken) =
        _prepareDeposit(CRVUSD_PENDLE_28MAR_2024, tokenReceived, balanceReceived);

    address alice = makeAddr("alice");
    tokenReceived.transfer(alice, 10e18);

    // Alice's shares to be minted
    uint256 sharesToBeMinted = derivativeToken.previewMintShares(
        tokenReceived.balanceOf(alice), derivativeToken.underlyingLpAssetsCurrent()
    );
    assert(sharesToBeMinted == tokenReceived.balanceOf(alice));

    // Malicious actor frontruns Alice's deposit
    tokenReceived.approve(address(derivativeToken), 1e18);
    derivativeToken.depositExactAmount(1e18);
    tokenReceived.approve(address(derivativeToken), 1e18);
    derivativeToken.addCompoundRewards(1e18);

    vm.warp(block.timestamp + 12 weeks);

    vm.startPrank(alice);
    // Alice checks amount of shares she will get after deposit
    uint256 newSharesToBeMinted = derivativeToken.previewMintShares(
        tokenReceived.balanceOf(alice), derivativeToken.underlyingLpAssetsCurrent()
    );
    // shares correspond to same amount as before malicious actor's deposits
    assertEq(newSharesToBeMinted, sharesToBeMinted);

    // Alice deposits her whole balance
    tokenReceived.approve(address(derivativeToken), tokenReceived.balanceOf(alice));
    derivativeToken.depositExactAmount(tokenReceived.balanceOf(alice));

    // Alice hasn't received what previewMintShares() gave her
    assert(newSharesToBeMinted != derivativeToken.balanceOf(address(alice)));

    console.log("NEW SHARES TO BE MINTED:", newSharesToBeMinted);
    console.log("BALANCE RECEIVED:", derivativeToken.balanceOf(address(alice)));
    vm.stopPrank();
}

Logs' output:

NEW SHARES TO BE MINTED: 10000000000000000000
BALANCE RECEIVED: 4985000000000000001

Tools Used

Manual Review

Recommended Mitigation Steps,

The preview functions should all sync the supply so users can get a proper projection of the outcome of the particular interaction. I also recommend introducing optional slippage control to depositExactAmount(), withdrawExactShares(), and withdrawExactAmount(). This is an extra layer of safety that will ensure the user never gets less than what they initially anticipated.

Assessed type

Token-Transfer

[GalloDaSballo]

Worth checking

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[c4-pre-sort]

GalloDaSballo marked the issue as high quality report

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[vonMangoldt]

previewmintshares cant sync supply because its a view and views dont write to storage.
Everyone can query the mintfee beforehand. There is no swapping so there is no extra fee.
Way overblown anyway. No need to change anything. Dismissed

[vm06007]

maximum Q/A but overblown and should be disqualified. User can always query impact and fee and expected value, there are view functions for that, these functions used on UI (https://app.wiselending.com/) hence no misinformation or misalignment is present in current implementation, expectations are set through views to be queried.

[c4-judge]

trust1995 marked the issue as unsatisfactory:
Invalid

[0xNentoR]

@trust1995 The sponsor has commented on the preview functions but the lack of slippage protection on depositExactAmount(), withdrawExactAmount(), and withdrawExactShares() hasn't been discussed.

There's a similar finding from last year that you judged as a valid medium: code-423n4/2023-05-maia-findings#901

In the recent PoolTogether contest, a similar slippage issue was reported and again judged as a medium: code-423n4/2024-03-pooltogether-findings#274

Given that Mainnet is in scope and there's no explicit mention of an external periphery within the protocol that handles this, I believe it makes sense that this should be considered valid given that risk of griefing and unwanted loss of funds for the user is present.

[trust1995]

Without the preview() aspect of the report, we are left with insufficient quality to for the briefly described slippage issue.