`PendlePowerManager` is incompatible with `PendleRouterV3`

[c4-bot-9]

Lines of code

https://github.com/pendle-finance/pendle-core-v2-public/blob/main/contracts/router/ActionAddRemoveLiqV3.sol#L166-L172 https://github.com/pendle-finance/pendle-core-v2-public/blob/main/contracts/router/ActionAddRemoveLiqV3.sol#L444-L449 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L467-L482 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L165-L171

Vulnerability details

Impact

PendlePowerManager is incompatible with the latest deployment of PendleRouter and will cause reverts when attempting to open a farm position.

Proof of Concept

The latest deployment of the Pendle router, PendleRouterV3, is incompatible with the PendlePowerManager contract. The sponsor is expecting full compatibility with both but this is not the case here. The problem stems from the calls made to PENDLE_ROUTER.removeLiquiditySingleSy() and PENDLE_ROUTER.addLiquiditySingleSy():

function _logicOpenPosition(
    bool _isAave,
    uint256 _nftId,
    uint256 _depositAmount,
    uint256 _totalDebtBalancer,
    uint256 _allowedSpread
)
    internal
{
    // ...
    (
        uint256 netLpOut
        ,
    ) = PENDLE_ROUTER.addLiquiditySingleSy(
        {
            _receiver: address(this),
            _market: address(PENDLE_MARKET),
            _netSyIn: syReceived,
            _minLpOut: 0,
            _guessPtReceivedFromSy: ApproxParams(
                {
                    guessMin: netPtFromSwap - 100,
                    guessMax: netPtFromSwap + 100,
                    guessOffchain: 0,
                    maxIteration: 50,
                    eps: 1e15
                }
            )
        }
    );
  // ...
}

function _logicClosePosition(
    uint256 _nftId,
    uint256 _borrowShares,
    uint256 _lendingShares,
    uint256 _totalDebtBalancer,
    uint256 _allowedSpread,
    address _caller,
    bool _ethBack,
    bool _isAave
)
    private
{
    // ...
    (
        uint256 netSyOut
        ,
    ) = PENDLE_ROUTER.removeLiquiditySingleSy(
        {
            _receiver: address(this),
            _market: address(PENDLE_MARKET),
            _netLpToRemove: withdrawnLpsAmount,
            _minSyOut: 0
        }
    );
  // ...
}

The issue is that the signatures of those have changed in V3:

function addLiquiditySingleSy(
    address receiver,
    address market,
    uint256 netSyIn,
    uint256 minLpOut,
    ApproxParams calldata guessPtReceivedFromSy,
    LimitOrderData calldata limit
) external returns (uint256 netLpOut, uint256 netSyFee);

function removeLiquiditySingleSy(
    address receiver,
    address market,
    uint256 netLpToRemove,
    uint256 minSyOut,
    LimitOrderData calldata limit
) external returns (uint256 netSyOut, uint256 netSyFee);

There's a new parameter called limit that's not accounted for in the calls in the PendlePowerFarmLeverageLogic helper contract. This will lead to calls always reverting with RouterInvalidAction due to Pendle's proxy not being able to locate the selector used.

Coded POC (PendlePowerFarmControllerBase.t.sol):

function _setUpCustom(address _pendleRouter) private {
    _setProperties();

    pendleLockInstance = IPendleLock(AddressesMap[chainId].pendleLock);

    wethInstance = IWETH(AddressesMap[chainId].weth);

    wiseOracleHubInstance = WiseOracleHub(AddressesMap[chainId].oracleHub);

    aaveHubInstance = IAaveHub(AddressesMap[chainId].aaveHub);

    vm.startPrank(wiseLendingInstance.master());

    controllerTester = new PendleControllerTester(
        AddressesMap[chainId].vePendle,
        AddressesMap[chainId].pendleTokenAddress,
        AddressesMap[chainId].voterContract,
        AddressesMap[chainId].voterRewardsClaimer,
        AddressesMap[chainId].oracleHub
    );

    pendlePowerFarmTokenFactory = controllerTester.PENDLE_POWER_FARM_TOKEN_FACTORY();

    PoolManager.CreatePool memory params = PoolManager.CreatePool({
        allowBorrow: true,
        poolToken: AddressesMap[chainId].aweth,
        poolMulFactor: 17500000000000000,
        poolCollFactor: 805000000000000000,
        maxDepositAmount: 1800000000000000000000000
    });

    wiseLendingInstance.createPool(params);

    IAaveHub(AddressesMap[chainId].aaveHub).setAaveTokenAddress(
        AddressesMap[chainId].weth, AddressesMap[chainId].aweth
    );

    if (block.chainid == ETH_CHAIN_ID) {
        wiseOracleHubInstance.addOracle(
            AddressesMap[chainId].aweth,
            wiseOracleHubInstance.priceFeed(AddressesMap[chainId].weth),
            new address[](0)
        );

        wiseOracleHubInstance.recalibrate(AddressesMap[chainId].aweth);
    }

    _addPendleTokenOracle();

    _addPendleMarketOracle(
        AddressesMap[chainId].PendleMarketStEth,
        address(wiseOracleHubInstance.priceFeed(AddressesMap[chainId].weth)),
        AddressesMap[chainId].weth,
        2 ether,
        3 ether
    );

    IPendlePowerFarmToken derivativeToken =
        _addPendleMarket(AddressesMap[chainId].PendleMarketStEth, "name", "symbol", MAX_CARDINALITY);

    pendleChildLpOracleInstance = new PendleChildLpOracle(address(pendleLpOracleInstance), address(derivativeToken));

    address[] memory underlyingTokens = new address[](1);
    underlyingTokens[0] = AddressesMap[chainId].weth;

    wiseOracleHubInstance.addOracle(
        address(derivativeToken), IPriceFeed(address(pendleChildLpOracleInstance)), underlyingTokens
    );

    address[] memory underlyingTokensCurrent = new address[](0);

    wiseOracleHubInstance.addOracle(CRV_TOKEN_ADDRESS, IPriceFeed(CRV_ETH_FEED), underlyingTokensCurrent);

    wiseOracleHubInstance.recalibrate(CRV_TOKEN_ADDRESS);

    curveUsdEthOracleInstance = new CurveUsdEthOracle(IPriceFeed(ETH_USD_FEED), IPriceFeed(CRVUSD_USD_FEED));

    wiseOracleHubInstance.addOracle(
        CRVUSD_TOKEN_ADDRESS, IPriceFeed(address(curveUsdEthOracleInstance)), new address[](0)
    );

    wiseOracleHubInstance.recalibrate(CRVUSD_TOKEN_ADDRESS);

    wiseOracleHubInstance.addTwapOracle(
        CRV_TOKEN_ADDRESS,
        CRV_UNI_POOL_ADDRESS,
        CRV_UNI_POOL_TOKEN0_ADDRESS,
        CRV_UNI_POOL_TOKEN1_ADDRESS,
        UNI_V3_FEE_CRV_UNI_POOL
    );

    wiseOracleHubInstance.addTwapOracleDerivative(
        CRVUSD_TOKEN_ADDRESS,
        CRVUSD_UNI_POOL_TOKEN0_ADDRESS,
        [ETH_USDC_UNI_POOL_ADDRESS, CRVUSD_UNI_POOL_ADDRESS],
        [ETH_USDC_UNI_POOL_TOKEN0_ADDRESS, CRVUSD_UNI_POOL_TOKEN0_ADDRESS],
        [ETH_USDC_UNI_POOL_TOKEN1_ADDRESS, CRVUSD_UNI_POOL_TOKEN1_ADDRESS],
        [UNI_V3_FEE_ETH_USDC_UNI_POOL, UNI_V3_FEE_CRVUSD_UNI_POOL]
    );

    address underlyingFeed = CRVUSD_TOKEN_ADDRESS;

    _addPendleMarketOracle(
        CRVUSD_PENDLE_28MAR_2024, address(curveUsdEthOracleInstance), underlyingFeed, 0.0008 ether, 0.0016 ether
    );

    derivativeToken = _addPendleMarket(CRVUSD_PENDLE_28MAR_2024, "name", "symbol", MAX_CARDINALITY);

    pendleChildLpOracleInstance = new PendleChildLpOracle(address(pendleLpOracleInstance), address(derivativeToken));

    underlyingTokens = new address[](1);
    underlyingTokens[0] = underlyingFeed;

    wiseOracleHubInstance.addOracle(
        address(derivativeToken), IPriceFeed(address(pendleChildLpOracleInstance)), underlyingTokens
    );

    PoolManager.CreatePool[] memory createPoolArray = new PoolManager.CreatePool[](3);

    createPoolArray[0] = PoolManager.CreatePool({
        allowBorrow: true,
        poolToken: CRVUSD_TOKEN_ADDRESS,
        poolMulFactor: 17500000000000000,
        poolCollFactor: 740000000000000000,
        maxDepositAmount: 2000000000000000000000000
    });

    createPoolArray[1] = PoolManager.CreatePool({
        allowBorrow: false,
        poolToken: controllerTester.pendleChildAddress(AddressesMap[chainId].PendleMarketStEth),
        poolMulFactor: 17500000000000000,
        poolCollFactor: 740000000000000000,
        maxDepositAmount: 2000000000000000000000000
    });

    createPoolArray[2] = PoolManager.CreatePool({
        allowBorrow: false,
        poolToken: controllerTester.pendleChildAddress(CRVUSD_PENDLE_28MAR_2024),
        poolMulFactor: 17500000000000000,
        poolCollFactor: 740000000000000000,
        maxDepositAmount: 2000000000000000000000000
    });

    for (uint256 i = 0; i < createPoolArray.length; i++) {
        wiseLendingInstance.createPool(createPoolArray[i]);
    }

    powerFarmNftsInstance = new PowerFarmNFTs("", "");

    powerFarmManagerInstance = new PendlePowerManager(
        address(wiseLendingInstance),
        controllerTester.pendleChildAddress(AddressesMap[chainId].PendleMarketStEth),
        _pendleRouter,
        AddressesMap[chainId].entryAssetPendleMarketStEth,
        AddressesMap[chainId].PendleMarketStEthSy,
        AddressesMap[chainId].PendleMarketStEth,
        AddressesMap[chainId].pendleRouterStatic,
        AddressesMap[chainId].dex,
        950000000000000000,
        address(powerFarmNftsInstance)
    );

    wiseLendingInstance.setVerifiedIsolationPool(address(powerFarmManagerInstance), true);

    vm.stopPrank();

    if (block.chainid == ETH_CHAIN_ID) {
        address wethWhaleEthMain = 0x8EB8a3b98659Cce290402893d0123abb75E3ab28;

        vm.startPrank(wethWhaleEthMain);

        wethInstance.transfer(wiseLendingInstance.master(), 1000 ether);

        vm.stopPrank();
    }

    vm.startPrank(wiseLendingInstance.master());

    IERC20(AddressesMap[chainId].weth).approve(address(powerFarmManagerInstance), 1000000 ether);

    wiseSecurityInstance = IWiseSecurity(wiseLendingInstance.WISE_SECURITY());

    positionNftsInstance = IPositionNFTs(wiseLendingInstance.POSITION_NFT());
}

function testCompatibleWithRouter() public {
    address pendleRouter = 0x0000000001E4ef00d069e71d6bA041b0A16F7eA0;
    _decideChain(true);
    _setUpCustom(pendleRouter);
    _testFarmShouldEnterAndExitIntoToken();
}

function testFail_IncompatibleWithRouterV3() public {
    address pendleRouterV3 = 0x00000000005BBB0EF59571E58418F9a4357b68A0;
    _decideChain(true);
    _setUpCustom(pendleRouterV3);
    // Reverts with "RouterInvalidAction"
    _testFarmShouldEnterAndExitIntoToken();
}

Tools Used

Manual Review

Recommended Mitigation Steps

Support only the latest router (V3) or add conditional checks to use the respective selector for each router version.

Assessed type

Other

[c4-pre-sort]

GalloDaSballo marked the issue as insufficient quality report

[c4-pre-sort]

GalloDaSballo marked the issue as remove high or low quality report

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[GalloDaSballo]

Worth flagging as it seems valid but may not be necessary if the sponsor is opting into using the previous deployment (which may not be deprecated)

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[trust1995]

By definition a codebase can never be guaranteed to be compatible with the latest version. Requesting warden to provide evidence lack of integration with v3 achieves M+ severity.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[0xNentoR]

@trust1995 The reason why I reported this was because the sponsor told me they expect compatibility with both. The problem is that an older version of pendle router is used here and the calls for addLiquiditySingleSy() and removeLiquiditySingleSy() are basically incompatible with the new one and will revert because of a missing parameter. To understand the rationale behind this submission, let's examine the deployed contracts:

Current router version (taken from test files): https://arbiscan.io/address/0x0000000001E4ef00d069e71d6bA041b0A16F7eA0 PendleRouterV3 (latest): https://arbiscan.io/address/0x00000000005BBB0EF59571E58418F9a4357b68A0

Here's the code for both on Deth.net: Old router: https://arbiscan.deth.net/address/0xFc0617465474a6b1CA0E37ec4E67B3EEFf93bc63 New one: https://arbiscan.deth.net/address/0x00000000005BBB0EF59571E58418F9a4357b68A0

The functions can be found inActionAddRemoveLiq and ActionAddRemoveLiqV3

Old one:

  /// @dev swaps SY to PT, then adds liquidity
  function _addLiquiditySingleSy(
      address receiver,
      address market,
      IPYieldToken YT,
      uint256 netSyIn,
      uint256 minLpOut,
      ApproxParams calldata guessPtReceivedFromSy
  ) internal returns (uint256 netLpOut, uint256 netSyFee) {
      MarketState memory state = IPMarket(market).readState(address(this));
      // ...
  }

New one:

function addLiquiditySingleToken(
    address receiver,
    address market,
    uint256 minLpOut,
    ApproxParams calldata guessPtReceivedFromSy,
    TokenInput calldata input,
    LimitOrderData calldata limit
) external payable returns (uint256 netLpOut, uint256 netSyFee, uint256 netSyInterm) {
    (IStandardizedYield SY, , IPYieldToken YT) = IPMarket(market).readTokens();

    netSyInterm = _mintSyFromToken(_entry_addLiquiditySingleSy(market, limit), address(SY), 1, input);

    (netLpOut, netSyFee) = _addLiquiditySingleSy(
        receiver,
        market,
        SY,
        YT,
        netSyInterm,
        minLpOut,
        guessPtReceivedFromSy,
        limit
    );

    // ...
}

function _addLiquiditySingleSy(
    address receiver,
    address market,
    IStandardizedYield SY,
    IPYieldToken YT,
    uint256 netSyIn,
    uint256 minLpOut,
    ApproxParams calldata guessPtReceivedFromSy,
    LimitOrderData calldata limit
) internal returns (uint256 netLpOut, uint256 netSyFee) {
    uint256 netSyLeft = netSyIn;
    uint256 netPtReceived;

    if (!_isEmptyLimit(limit)) {
        (netSyLeft, netPtReceived, netSyFee, ) = _fillLimit(market, SY, netSyLeft, limit);
        _transferOut(address(SY), market, netSyLeft);
    }

    (uint256 netPtOutMarket, , ) = _readMarket(market).approxSwapSyToAddLiquidity(
        YT.newIndex(),
        netSyLeft,
        netPtReceived,
        block.timestamp,
        guessPtReceivedFromSy
    );

    // ...
}

_readMarket() comes from ActionBase, here's it's definition:

 function _readMarket(address market) internal view returns (MarketState memory) {
      return IPMarket(market).readState(address(this));
  }

You can see that both call readState() from the underlying Pendle market. Here you can find all market deployments: https://docs.pendle.finance/Developers/Deployments/Arbitrum#markets

I'll use the first one for the example. Arbiscan: https://arbiscan.io/address/0x7D49E5Adc0EAAD9C027857767638613253eF125f Deth.net: https://arbiscan.deth.net/address/0x7D49E5Adc0EAAD9C027857767638613253eF125f

If you look at the definion of readState(), you'll see the following:

function readState(address router) public view returns (MarketState memory market) {
    market.totalPt = _storage.totalPt;
    market.totalSy = _storage.totalSy;
    market.totalLp = totalSupply().Int();

    (market.treasury, market.lnFeeRateRoot, market.reserveFeePercent) = IPMarketFactory(
        factory
    ).getMarketConfig(router);

    market.scalarRoot = scalarRoot;
    market.expiry = expiry;

    market.lastLnImpliedRate = _storage.lastLnImpliedRate;
}

readState() on all markets reaches out to the factory contract it was deployed with to grab the market config. And there are two versions: MarketFactory and MarketFactoryV3. Again, both can be found in the docs but here are the links:

MarketFactory: https://arbiscan.io/address/0xf5a7De2D276dbda3EEf1b62A9E718EFf4d29dDC8 MarketFactory V3: https://arbiscan.io/address/0x2FCb47B58350cD377f94d3821e7373Df60bD9Ced

You can take a market from the docs and query Market::isValidMarket(). Using the first one, the old factory will return true whereas the new one, false.

So basically what this all means is that the protocol won't be able to use new markets. Pendle can start phasing out the old ones and migrating them over. The fix is rather easy on the protocol's end, they just need to account for the newly added parameter and can support both, if they wish, using a conditional check.

[trust1995]

@vonMangoldt can you confirm the warden's claims around your intentions?

[0xNentoR]

@trust1995 Still no response from the sponsor here

[trust1995]

Without sponsor's take the warden's claim that V3 should be compatible with the design is accepted.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[thebrittfactor]

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.