LoopFi / Penpie

[WP-M1] PositionActionPenpie._onWithdraw() may have forgotten to convert the return value of vault.withdraw() from [wad] base to [CDPVault.tokenScale()] base.

This could lead to incorrect amounts being used on L86 and L88, causing wrong return values.

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/proxy/PositionActionPenpie.sol#L69-L89

    /// @notice Withdraw collateral from the vault
    /// @param vault Address of the vault
    /// @param position Address of the position
    /// @param dst Token the caller expects to receive
    /// @param amount Amount of collateral to withdraw [wad]
    /// @return Amount of collateral withdrawn [CDPVault.tokenScale()]
    function _onWithdraw(
        address vault,
        address position,
        address dst,
        uint256 amount,
        uint256 /*minAmountOut*/
    ) internal override returns (uint256) {
        uint256 collateralWithdrawn = ICDPVault(vault).withdraw(address(position), amount);
        address collateralToken = address(ICDPVault(vault).token());

        if (dst != collateralToken && dst != address(0)) {
            penpieHelper.withdrawMarket(dst, collateralWithdrawn);
        }
        return collateralWithdrawn;
    }

[WP-M2] The return value of PositionActionPenpie._onIncreaseLever() is expected to be [wad] based, but the current implementation is [CDPVault.tokenScale()] based

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/proxy/PositionActionPenpie.sol#L91-L115

    /// @notice Hook to increase lever by depositing collateral into the CDPVault
    /// @param leverParams LeverParams struct
    /// @param /*upFrontToken*/ the address of the token passed up front
    /// @param /*upFrontAmount*/ the amount of tokens passed up front [CDPVault.tokenScale()]
    /// @param /*swapAmountOut*/ the amount of tokens received from the stablecoin flash loan swap [CDPVault.tokenScale()]
    /// @return addCollateralAmount Amount of collateral added to CDPVault position [wad]
    function _onIncreaseLever(
        LeverParams memory leverParams,
        address /*upFrontToken*/,
        uint256 /*upFrontAmount*/,
        uint256 /*swapAmountOut*/
    ) internal override returns (uint256 addCollateralAmount) {
        if (leverParams.auxAction.args.length != 0) {
            _delegateCall(address(poolAction), abi.encodeWithSelector(poolAction.join.selector, leverParams.auxAction));
        }
        addCollateralAmount = IERC20(leverParams.collateralToken).balanceOf(address(this));
        IERC20(leverParams.collateralToken).forceApprove(address(penpieStaking), addCollateralAmount);
        penpieHelper.depositMarketFor(leverParams.collateralToken, address(this), addCollateralAmount);

        addCollateralAmount = ICDPVault(leverParams.vault).token().balanceOf(address(this));
        ICDPVault(leverParams.vault).token().forceApprove(leverParams.vault, addCollateralAmount);

        // deposit into the CDP Vault
        return addCollateralAmount;
    }
    /// @notice Callback function for the flash loan taken out in increaseLever
    /// @param data The encoded bytes that were passed into the flash loan
    function onFlashLoan(
        address /*initiator*/,
        address /*token*/,
        uint256 amount,
        uint256 fee,
        bytes calldata data
    ) external returns (bytes32) {
@@ 425,446 @@ if (msg.sender != address(flashlender)) revert PositionAction__onFlashLoan__invalidSender(); (LeverParams memory leverParams, address upFrontToken, uint256 upFrontAmount) = abi.decode( data, (LeverParams, address, uint256) ); // perform a pre swap from arbitrary token to collateral token if necessary if (leverParams.auxSwap.assetIn != address(0)) { bytes memory auxSwapData = _delegateCall( address(swapAction), abi.encodeWithSelector(swapAction.swap.selector, leverParams.auxSwap) ); upFrontAmount = abi.decode(auxSwapData, (uint256)); } // handle the flash loan swap bytes memory swapData = _delegateCall( address(swapAction), abi.encodeWithSelector(swapAction.swap.selector, leverParams.primarySwap) ); uint256 swapAmountOut = abi.decode(swapData, (uint256));
// deposit collateral and handle any CDP specific actions uint256 collateral = _onIncreaseLever(leverParams, upFrontToken, upFrontAmount, swapAmountOut); // derive the amount of normal debt from the swap amount out uint256 addDebt = amount + fee; // add collateral and debt ICDPVault(leverParams.vault).modifyCollateralAndDebt( leverParams.position, address(this), address(this), toInt256(collateral), toInt256(addDebt) ); underlyingToken.forceApprove(address(flashlender), addDebt); return CALLBACK_SUCCESS; }
@@ 406,413 @@ /// @notice Modifies a Position's collateral and debt balances /// @dev Checks that the global debt ceiling and the vault's debt ceiling have not been exceeded via the CDM, /// - that the Position is still safe after the modification, /// - that the msg.sender has the permission of the owner to decrease the collateral-to-debt ratio, /// - that the msg.sender has the permission of the collateralizer to put up new collateral, /// - that the msg.sender has the permission of the creditor to settle debt with their credit, /// - that that the vault debt floor is exceeded /// - that the vault minimum collateralization ratio is met
/// @param owner Address of the owner of the position /// @param collateralizer Address of who puts up or receives the collateral delta /// @param creditor Address of who provides or receives the credit delta for the debt delta /// @param deltaCollateral Amount of collateral to put up (+) or to remove (-) from the position [wad] /// @param deltaDebt Amount of normalized debt (gross, before rate is applied) to generate (+) or /// to settle (-) on this position [wad] function modifyCollateralAndDebt( address owner, address collateralizer, address creditor, int256 deltaCollateral, int256 deltaDebt ) public {
@@ 427,505 @@ if ( // position is either more safe than before or msg.sender has the permission from the owner ((deltaDebt > 0 || deltaCollateral < 0) && !hasPermission(owner, msg.sender)) || // msg.sender has the permission of the collateralizer to collateralize the position using their cash (deltaCollateral > 0 && !hasPermission(collateralizer, msg.sender)) || // msg.sender has the permission of the creditor to use their credit to repay the debt (deltaDebt < 0 && !hasPermission(creditor, msg.sender)) ) revert CDPVault__modifyCollateralAndDebt_noPermission(); // if the vault is paused allow only debt decreases if (deltaDebt > 0 || deltaCollateral != 0) { _requireNotPaused(); } Position memory position = positions[owner]; DebtData memory debtData = _calcDebt(position); uint256 newDebt; uint256 newCumulativeIndex; uint256 profit; int256 quotaRevenueChange; if (deltaDebt > 0) { uint256 debtToIncrease = uint256(deltaDebt); // Internal debt calculation remains in 18-decimal precision (newDebt, newCumulativeIndex) = CreditLogic.calcIncrease( debtToIncrease, position.debt, debtData.cumulativeIndexNow, position.cumulativeIndexLastUpdate ); position.cumulativeQuotaInterest = debtData.cumulativeQuotaInterest; position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow; quotaRevenueChange = _calcQuotaRevenueChange(deltaDebt); uint256 scaledDebtIncrease = wmul(debtToIncrease, poolUnderlyingScale); pool.lendCreditAccount(scaledDebtIncrease, creditor); } else if (deltaDebt < 0) { uint256 debtToDecrease = abs(deltaDebt); uint256 maxRepayment = calcTotalDebt(debtData); if (debtToDecrease >= maxRepayment) { debtToDecrease = maxRepayment; deltaDebt = -toInt256(debtToDecrease); } uint256 scaledDebtDecrease = wmul(debtToDecrease, poolUnderlyingScale); poolUnderlying.safeTransferFrom(creditor, address(pool), scaledDebtDecrease); uint128 newCumulativeQuotaInterest; if (debtToDecrease == maxRepayment) { newDebt = 0; newCumulativeIndex = debtData.cumulativeIndexNow; profit = debtData.accruedInterest; newCumulativeQuotaInterest = 0; } else { (newDebt, newCumulativeIndex, profit, newCumulativeQuotaInterest) = calcDecrease( debtToDecrease, position.debt, debtData.cumulativeIndexNow, position.cumulativeIndexLastUpdate, debtData.cumulativeQuotaInterest ); } quotaRevenueChange = _calcQuotaRevenueChange(-int(debtData.debt - newDebt)); uint256 scaledRemainingDebt = wmul(debtData.debt - newDebt, poolUnderlyingScale); uint256 scaledProfit = wmul(profit, poolUnderlyingScale); pool.repayCreditAccount(scaledRemainingDebt, scaledProfit, 0); position.cumulativeQuotaInterest = newCumulativeQuotaInterest; position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow; } else { newDebt = position.debt; newCumulativeIndex = debtData.cumulativeIndexLastUpdate; }
if (deltaCollateral > 0) { uint256 amount = wmul(deltaCollateral.toUint256(), tokenScale); token.safeTransferFrom(collateralizer, address(this), amount); } else if (deltaCollateral < 0) { uint256 amount = wmul(abs(deltaCollateral), tokenScale); token.safeTransfer(collateralizer, amount); }
@@ 515,530 @@ position = _modifyPosition(owner, position, newDebt, newCumulativeIndex, deltaCollateral, totalDebt); VaultConfig memory config = vaultConfig; uint256 spotPrice_ = spotPrice(); uint256 collateralValue = wmul(position.collateral, spotPrice_); if ( (deltaDebt > 0 || deltaCollateral < 0) && !_isCollateralized(calcTotalDebt(_calcDebt(position)), collateralValue, config.liquidationRatio) ) revert CDPVault__modifyCollateralAndDebt_notSafe(); if (quotaRevenueChange != 0) { int256 scaledQuotaRevenueChange = wmul(poolUnderlyingScale, quotaRevenueChange); IPoolV3(pool).updateQuotaRevenue(scaledQuotaRevenueChange); } emit ModifyCollateralAndDebt(owner, collateralizer, creditor, deltaCollateral, deltaDebt);
}
    /// @notice Callback function for the flash loan taken out in increaseLever
    /// @param data The encoded bytes that were passed into the flash loan
    function onFlashLoan(
        address /*initiator*/,
        address /*token*/,
        uint256 amount,
        uint256 fee,
        bytes calldata data
    ) external returns (bytes32) {
@@ 425,446 @@ if (msg.sender != address(flashlender)) revert PositionAction__onFlashLoan__invalidSender(); (LeverParams memory leverParams, address upFrontToken, uint256 upFrontAmount) = abi.decode( data, (LeverParams, address, uint256) ); // perform a pre swap from arbitrary token to collateral token if necessary if (leverParams.auxSwap.assetIn != address(0)) { bytes memory auxSwapData = _delegateCall( address(swapAction), abi.encodeWithSelector(swapAction.swap.selector, leverParams.auxSwap) ); upFrontAmount = abi.decode(auxSwapData, (uint256)); } // handle the flash loan swap bytes memory swapData = _delegateCall( address(swapAction), abi.encodeWithSelector(swapAction.swap.selector, leverParams.primarySwap) ); uint256 swapAmountOut = abi.decode(swapData, (uint256));
// deposit collateral and handle any CDP specific actions uint256 collateral = _onIncreaseLever(leverParams, upFrontToken, upFrontAmount, swapAmountOut); // derive the amount of normal debt from the swap amount out uint256 addDebt = amount + fee; // add collateral and debt ICDPVault(leverParams.vault).modifyCollateralAndDebt( leverParams.position, address(this), address(this), toInt256(collateral), toInt256(addDebt) ); underlyingToken.forceApprove(address(flashlender), addDebt); return CALLBACK_SUCCESS; }
@@ 406,413 @@ /// @notice Modifies a Position's collateral and debt balances /// @dev Checks that the global debt ceiling and the vault's debt ceiling have not been exceeded via the CDM, /// - that the Position is still safe after the modification, /// - that the msg.sender has the permission of the owner to decrease the collateral-to-debt ratio, /// - that the msg.sender has the permission of the collateralizer to put up new collateral, /// - that the msg.sender has the permission of the creditor to settle debt with their credit, /// - that that the vault debt floor is exceeded /// - that the vault minimum collateralization ratio is met
/// @param owner Address of the owner of the position /// @param collateralizer Address of who puts up or receives the collateral delta /// @param creditor Address of who provides or receives the credit delta for the debt delta /// @param deltaCollateral Amount of collateral to put up (+) or to remove (-) from the position [wad] /// @param deltaDebt Amount of normalized debt (gross, before rate is applied) to generate (+) or /// to settle (-) on this position [wad] function modifyCollateralAndDebt( address owner, address collateralizer, address creditor, int256 deltaCollateral, int256 deltaDebt ) public {
@@ 427,505 @@ if ( // position is either more safe than before or msg.sender has the permission from the owner ((deltaDebt > 0 || deltaCollateral < 0) && !hasPermission(owner, msg.sender)) || // msg.sender has the permission of the collateralizer to collateralize the position using their cash (deltaCollateral > 0 && !hasPermission(collateralizer, msg.sender)) || // msg.sender has the permission of the creditor to use their credit to repay the debt (deltaDebt < 0 && !hasPermission(creditor, msg.sender)) ) revert CDPVault__modifyCollateralAndDebt_noPermission(); // if the vault is paused allow only debt decreases if (deltaDebt > 0 || deltaCollateral != 0) { _requireNotPaused(); } Position memory position = positions[owner]; DebtData memory debtData = _calcDebt(position); uint256 newDebt; uint256 newCumulativeIndex; uint256 profit; int256 quotaRevenueChange; if (deltaDebt > 0) { uint256 debtToIncrease = uint256(deltaDebt); // Internal debt calculation remains in 18-decimal precision (newDebt, newCumulativeIndex) = CreditLogic.calcIncrease( debtToIncrease, position.debt, debtData.cumulativeIndexNow, position.cumulativeIndexLastUpdate ); position.cumulativeQuotaInterest = debtData.cumulativeQuotaInterest; position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow; quotaRevenueChange = _calcQuotaRevenueChange(deltaDebt); uint256 scaledDebtIncrease = wmul(debtToIncrease, poolUnderlyingScale); pool.lendCreditAccount(scaledDebtIncrease, creditor); } else if (deltaDebt < 0) { uint256 debtToDecrease = abs(deltaDebt); uint256 maxRepayment = calcTotalDebt(debtData); if (debtToDecrease >= maxRepayment) { debtToDecrease = maxRepayment; deltaDebt = -toInt256(debtToDecrease); } uint256 scaledDebtDecrease = wmul(debtToDecrease, poolUnderlyingScale); poolUnderlying.safeTransferFrom(creditor, address(pool), scaledDebtDecrease); uint128 newCumulativeQuotaInterest; if (debtToDecrease == maxRepayment) { newDebt = 0; newCumulativeIndex = debtData.cumulativeIndexNow; profit = debtData.accruedInterest; newCumulativeQuotaInterest = 0; } else { (newDebt, newCumulativeIndex, profit, newCumulativeQuotaInterest) = calcDecrease( debtToDecrease, position.debt, debtData.cumulativeIndexNow, position.cumulativeIndexLastUpdate, debtData.cumulativeQuotaInterest ); } quotaRevenueChange = _calcQuotaRevenueChange(-int(debtData.debt - newDebt)); uint256 scaledRemainingDebt = wmul(debtData.debt - newDebt, poolUnderlyingScale); uint256 scaledProfit = wmul(profit, poolUnderlyingScale); pool.repayCreditAccount(scaledRemainingDebt, scaledProfit, 0); position.cumulativeQuotaInterest = newCumulativeQuotaInterest; position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow; } else { newDebt = position.debt; newCumulativeIndex = debtData.cumulativeIndexLastUpdate; }
if (deltaCollateral > 0) { uint256 amount = wmul(deltaCollateral.toUint256(), tokenScale); token.safeTransferFrom(collateralizer, address(this), amount); } else if (deltaCollateral < 0) { uint256 amount = wmul(abs(deltaCollateral), tokenScale); token.safeTransfer(collateralizer, amount); }
@@ 515,530 @@ position = _modifyPosition(owner, position, newDebt, newCumulativeIndex, deltaCollateral, totalDebt); VaultConfig memory config = vaultConfig; uint256 spotPrice_ = spotPrice(); uint256 collateralValue = wmul(position.collateral, spotPrice_); if ( (deltaDebt > 0 || deltaCollateral < 0) && !_isCollateralized(calcTotalDebt(_calcDebt(position)), collateralValue, config.liquidationRatio) ) revert CDPVault__modifyCollateralAndDebt_notSafe(); if (quotaRevenueChange != 0) { int256 scaledQuotaRevenueChange = wmul(poolUnderlyingScale, quotaRevenueChange); IPoolV3(pool).updateQuotaRevenue(scaledQuotaRevenueChange); } emit ModifyCollateralAndDebt(owner, collateralizer, creditor, deltaCollateral, deltaDebt);
}

[WP-L3] Unnecessary if condition in _onDecreaseLever()

The if block must be executed for the function to work properly. Market LP tokens must first be exited from the market before they can be swapped into the borrowed token to repay the debt. Therefore, this step cannot be skipped.

function _onDecreaseLever(
    LeverParams memory leverParams,
    uint256 subCollateral
) internal override returns (uint256 tokenOut) {
    _onWithdraw(leverParams.vault, leverParams.position, address(0), subCollateral, 0);
    (address pendleToken, , ) = abi.decode(leverParams.auxAction.args, (address, uint256, address));
    penpieHelper.withdrawMarket(pendleToken, subCollateral);
    if (leverParams.auxAction.args.length != 0) {
        bytes memory exitData = _delegateCall(
            address(poolAction),
            abi.encodeWithSelector(poolAction.exit.selector, leverParams.auxAction)
        );

        tokenOut = abi.decode(exitData, (uint256));
    }
}

Recommendation

Remove the if (leverParams.auxAction.args.length != 0) check in PositionActionPenpie.sol, PositionActionTranchess.sol, and PositionActionPendle.sol.

function _onDecreaseLever(
    LeverParams memory leverParams,
    uint256 subCollateral
) internal override returns (uint256 tokenOut) {
    _onWithdraw(leverParams.vault, leverParams.position, address(0), subCollateral, 0);
    (address pendleToken, , ) = abi.decode(leverParams.auxAction.args, (address, uint256, address));
    penpieHelper.withdrawMarket(pendleToken, subCollateral);
    if (leverParams.auxAction.args.length != 0) {
        bytes memory exitData = _delegateCall(
            address(poolAction),
            abi.encodeWithSelector(poolAction.exit.selector, leverParams.auxAction)
        );

        tokenOut = abi.decode(exitData, (uint256));
    }
}

[WP-L4] Consider keeping leverParams.collateralToken as CDPVault.token in PositionActionPenpie._onIncreaseLever(leverParams, ...) to maintain consistency with _onIncreaseLever() implementations in other PositionActions like PositionAction4626, and to avoid confusion with the LeverParams.collateralToken comments.

Currently, the implementation of PositionActionPenpie._onIncreaseLever(leverParams, ...) requires leverParams.collateralToken to be CDPVault.token.underlying() instead of CDPVault.token.

Consider using leverParams.collateralToken.underlying() as the first parameter for penpieHelper.depositMarketFor() in PositionActionPenpie._onIncreaseLever(), similar to how it's done in PositionAction4626._onIncreaseLever().

/// @notice General parameters relevant for both increasing and decreasing leverage
struct LeverParams {
    // position to lever
    address position;
    // the vault to lever
    address vault;
    // the vault's token
    address collateralToken;
    // the swap parameters to swap collateral to underlying token or vice versa
    SwapParams primarySwap;
    // optional swap parameters to swap an arbitrary token to the collateral token or vice versa
    SwapParams auxSwap;
    // optional action parameters
    PoolActionParams auxAction;
}
@@ 91,96 @@ /// @notice Hook to increase lever by depositing collateral into the CDPVault /// @param leverParams LeverParams struct /// @param /*upFrontToken*/ the address of the token passed up front /// @param /*upFrontAmount*/ the amount of tokens passed up front [CDPVault.tokenScale()] /// @param /*swapAmountOut*/ the amount of tokens received from the stablecoin flash loan swap [CDPVault.tokenScale()] /// @return addCollateralAmount Amount of collateral added to CDPVault position [wad]
function _onIncreaseLever( LeverParams memory leverParams, address /*upFrontToken*/, uint256 /*upFrontAmount*/, uint256 /*swapAmountOut*/ ) internal override returns (uint256 addCollateralAmount) { if (leverParams.auxAction.args.length != 0) { _delegateCall(address(poolAction), abi.encodeWithSelector(poolAction.join.selector, leverParams.auxAction)); } addCollateralAmount = IERC20(leverParams.collateralToken).balanceOf(address(this)); IERC20(leverParams.collateralToken).forceApprove(address(penpieStaking), addCollateralAmount); penpieHelper.depositMarketFor(leverParams.collateralToken, address(this), addCollateralAmount); addCollateralAmount = ICDPVault(leverParams.vault).token().balanceOf(address(this)); ICDPVault(leverParams.vault).token().forceApprove(leverParams.vault, addCollateralAmount); // deposit into the CDP Vault return addCollateralAmount; }

As a reference (PositionAction4626):

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/proxy/PositionAction4626.sol#L81-L137

@@ 81,86 @@ /// @notice Hook to decrease lever by depositing collateral into the Yearn Vault and the Yearn Vault /// @param leverParams LeverParams struct /// @param upFrontToken the token passed up front /// @param upFrontAmount the amount of tokens passed up front [IYVault.decimals()] /// @param swapAmountOut the amount of tokens received from the stablecoin flash loan swap [IYVault.decimals()] /// @return Amount of collateral added to CDPVault position [wad]
function _onIncreaseLever( LeverParams memory leverParams, address upFrontToken, uint256 upFrontAmount, uint256 swapAmountOut ) internal override returns (uint256) { uint256 upFrontCollateral; uint256 addCollateralAmount = swapAmountOut; if (leverParams.collateralToken == upFrontToken && leverParams.auxSwap.assetIn == address(0)) { // if there was no aux swap then treat this amount as the ERC4626 token upFrontCollateral = upFrontAmount; } else { // otherwise treat as the ERC4626 underlying addCollateralAmount += upFrontAmount; } address underlyingToken = IERC4626(leverParams.collateralToken).asset(); // join into the pool if needed
@@ 105,126 @@ if (leverParams.auxAction.args.length != 0) { address joinToken = swapAction.getSwapToken(leverParams.primarySwap); address joinUpfrontToken = upFrontToken; if (leverParams.auxSwap.assetIn != address(0)) { joinUpfrontToken = swapAction.getSwapToken(leverParams.auxSwap); } // update the join parameters with the new amounts PoolActionParams memory poolActionParams = poolAction.updateLeverJoin( leverParams.auxAction, joinToken, joinUpfrontToken, swapAmountOut, upFrontAmount ); _delegateCall(address(poolAction), abi.encodeWithSelector(poolAction.join.selector, poolActionParams)); // retrieve the total amount of collateral after the join addCollateralAmount = IERC20(underlyingToken).balanceOf(address(this)); }
// deposit into the ERC4626 vault IERC20(underlyingToken).forceApprove(leverParams.collateralToken, addCollateralAmount); addCollateralAmount = IERC4626(leverParams.collateralToken).deposit(addCollateralAmount, address(this)) + upFrontCollateral; // deposit into the CDP vault IERC20(leverParams.collateralToken).forceApprove(leverParams.vault, addCollateralAmount); return addCollateralAmount; }
Penpie's Related Code

PendleMarketDepositHelper/PendleMarketDepositHelper/contracts/pendle/PendleMarketDepositHelper.sol

    function depositMarketFor(
        address _market,
        address _for,
        uint256 _amount
    ) external onlyActivePool(_market) nonReentrant {
        _depositMarket(_market, _for, msg.sender, _amount);
    }

PendleMarketDepositHelper/PendleMarketDepositHelper/contracts/pendle/PendleMarketDepositHelper.sol

    function _depositMarket(
        address _market,
        address _for,
        address _from,
        uint256 _amount
    ) internal {
        IPendleStaking(pendleStaking).depositMarket(
            _market,
            _for,
            _from,
            _amount
        );

        emit NewDeposit(_for, _market, _amount);
    }

PendleStaking/PendleStaking/contracts/pendle/PendleStakingBaseUpg.sol

    function depositMarket(
        address _market,
        address _for,
        address _from,
        uint256 _amount
    ) external override nonReentrant whenNotPaused _onlyActivePoolHelper(_market) {
        Pool storage poolInfo = pools[_market];
        _harvestMarketRewards(poolInfo.market, false);

        IERC20(poolInfo.market).safeTransferFrom(_from, address(this), _amount);

        // mint the receipt to the user driectly
        IMintableERC20(poolInfo.receiptToken).mint(_for, _amount);

        emit NewMarketDeposit(_for, _market, _amount, poolInfo.receiptToken, _amount);
    }

PendleStaking/PendleStaking/contracts/pendle/PendleStakingBaseUpg.sol

    function registerPool(
        address _market,
        uint256 _allocPoints,
        string memory name,
        string memory symbol
    ) external onlyOwner {
        if (pools[_market].market != address(0)) {
            revert PoolOccupied();
        }

        IERC20 newToken = IERC20(
            ERC20FactoryLib.createReceipt(_market, masterPenpie, name, symbol)
        );

        address rewarder = IMasterPenpie(masterPenpie).createRewarder(
            address(newToken),
            address(PENDLE)
        );

        IPendleMarketDepositHelper(marketDepositHelper).setPoolInfo(_market, rewarder, true);

        IMasterPenpie(masterPenpie).add(
            _allocPoints,
            address(_market),
            address(newToken),
            address(rewarder)
        );

        pools[_market] = Pool({
            isActive: true,
            market: _market,
            receiptToken: address(newToken),
            rewarder: address(rewarder),
            helper: marketDepositHelper,
            lastHarvestTime: block.timestamp
        });
        poolTokenList.push(_market);

        emit PoolAdded(_market, address(rewarder), address(newToken));
    }
/// @notice General parameters relevant for both increasing and decreasing leverage
struct LeverParams {
    // position to lever
    address position;
    // the vault to lever
    address vault;
    // the vault's token
    address collateralToken;
    // the swap parameters to swap collateral to underlying token or vice versa
    SwapParams primarySwap;
    // optional swap parameters to swap an arbitrary token to the collateral token or vice versa
    SwapParams auxSwap;
    // optional action parameters
    PoolActionParams auxAction;
}
@@ 91,96 @@ /// @notice Hook to increase lever by depositing collateral into the CDPVault /// @param leverParams LeverParams struct /// @param /*upFrontToken*/ the address of the token passed up front /// @param /*upFrontAmount*/ the amount of tokens passed up front [CDPVault.tokenScale()] /// @param /*swapAmountOut*/ the amount of tokens received from the stablecoin flash loan swap [CDPVault.tokenScale()] /// @return addCollateralAmount Amount of collateral added to CDPVault position [wad]
function _onIncreaseLever( LeverParams memory leverParams, address /*upFrontToken*/, uint256 /*upFrontAmount*/, uint256 /*swapAmountOut*/ ) internal override returns (uint256 addCollateralAmount) { if (leverParams.auxAction.args.length != 0) { _delegateCall(address(poolAction), abi.encodeWithSelector(poolAction.join.selector, leverParams.auxAction)); } addCollateralAmount = IERC20(leverParams.collateralToken).balanceOf(address(this)); IERC20(leverParams.collateralToken).forceApprove(address(penpieStaking), addCollateralAmount); penpieHelper.depositMarketFor(leverParams.collateralToken, address(this), addCollateralAmount); addCollateralAmount = ICDPVault(leverParams.vault).token().balanceOf(address(this)); ICDPVault(leverParams.vault).token().forceApprove(leverParams.vault, addCollateralAmount); // deposit into the CDP Vault return addCollateralAmount; }

[WP-L5] Aggregation oracle should use the earliest timestamp

The updated time of the price should be the earliest timestamp of the source prices.

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/oracle/CombinedAggregatorV3Oracle.sol#L38-L48

/// @notice Return the latest price combined from two Chainlink like oracle and the timestamp from the first aggregator
function latestRoundData()
    public
    view
    returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound)
{
    (uint256 value, uint256 timestamp) = getAggregatorData(aggregator, aggregatorScale, aggregatorHeartbeat);
    (uint256 value2, ) = getAggregatorData(aggregator2, aggregator2Scale, aggregator2Heartbeat);
    uint256 price = wmul(value, value2);
    return (0, int256(price), 0, timestamp, 0);
}

[WP-L6] Consider adding if (token == vault.token) continue; in RewardManagerPenpie._updateRewardIndex() similar to RewardManager._updateRewardIndex()

This prevents accidentally using vault.token as reward token.

    function _updateRewardIndex()
        internal
        virtual
        override
        returns (address[] memory tokens, uint256[] memory indexes)
    {
        tokens = _getRewardTokens();
        indexes = new uint256[](tokens.length);

        if (tokens.length == 0) return (tokens, indexes);

        if (lastRewardBlock != block.number) {
            // if we have not yet update the index for this block
            lastRewardBlock = block.number;

            uint256 totalShares = _rewardSharesTotal();
            address[] memory stakingTokens = new address[](1);
            stakingTokens[0] = stakingToken;
            address[][] memory rewardTokens = new address[][](1);
            // Claim external rewards
            masterPenpie.multiclaimFor(stakingTokens, rewardTokens, vault);
            for (uint256 i = 0; i < tokens.length; ++i) {
                address token = tokens[i];

                // the entire token balance of the contract must be the rewards of the contract
                RewardState memory _state = rewardState[token];
                (uint256 lastBalance, uint256 index) = (_state.lastBalance, _state.index);

                uint256 accrued = IERC20(tokens[i]).balanceOf(vault) - lastBalance;
                uint256 deltaIndex;
                uint256 advanceBalance;
                if (totalShares != 0) {
                    deltaIndex = accrued.divDown(totalShares);
                    advanceBalance = deltaIndex.mulDown(totalShares);
                }

                if (index == 0) index = INITIAL_REWARD_INDEX;
                if (totalShares != 0) index += deltaIndex;

                rewardState[token] = RewardState({
                    index: index.Uint128(),
                    lastBalance: (lastBalance + advanceBalance).Uint128()
                });

                indexes[i] = index;
            }
        } else {
            for (uint256 i = 0; i < tokens.length; i++) {
                indexes[i] = rewardState[tokens[i]].index;
            }
        }
    }

As a reference:

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/pendle-rewards/RewardManager.sol#L44-L95

    function _updateRewardIndex()
        internal
        virtual
        override
        returns (address[] memory tokens, uint256[] memory indexes)
    {
        tokens = market.getRewardTokens();
        indexes = new uint256[](tokens.length);

        if (tokens.length == 0) return (tokens, indexes);

        if (lastRewardBlock != block.number) {
            // if we have not yet update the index for this block
            lastRewardBlock = block.number;

            uint256 totalShares = _rewardSharesTotal();
            // Claim external rewards on Market
            market.redeemRewards(address(vault));

            for (uint256 i = 0; i < tokens.length; ++i) {
                address token = tokens[i];

                if (token == address(market)) continue;
                // the entire token balance of the contract must be the rewards of the contract

                RewardState memory _state = rewardState[token];
                (uint256 lastBalance, uint256 index) = (_state.lastBalance, _state.index);

                uint256 accrued = IERC20(tokens[i]).balanceOf(vault) - lastBalance;
                uint256 deltaIndex;
                uint256 advanceBalance;
                if (totalShares != 0) {
                    deltaIndex = accrued.divDown(totalShares);
                    advanceBalance = deltaIndex.mulDown(totalShares);
                }

                if (index == 0) index = INITIAL_REWARD_INDEX;
                if (totalShares != 0) index += deltaIndex;

                rewardState[token] = RewardState({
                    index: index.Uint128(),
                    lastBalance: (lastBalance + advanceBalance).Uint128()
                });

                indexes[i] = index;
            }
        } else {
            for (uint256 i = 0; i < tokens.length; i++) {
                indexes[i] = rewardState[tokens[i]].index;
            }
        }
    }
    function _updateRewardIndex()
        internal
        virtual
        override
        returns (address[] memory tokens, uint256[] memory indexes)
    {
        tokens = _getRewardTokens();
        indexes = new uint256[](tokens.length);

        if (tokens.length == 0) return (tokens, indexes);

        if (lastRewardBlock != block.number) {
            // if we have not yet update the index for this block
            lastRewardBlock = block.number;

            uint256 totalShares = _rewardSharesTotal();
            address[] memory stakingTokens = new address[](1);
            stakingTokens[0] = stakingToken;
            address[][] memory rewardTokens = new address[][](1);
            // Claim external rewards
            masterPenpie.multiclaimFor(stakingTokens, rewardTokens, vault);
            for (uint256 i = 0; i < tokens.length; ++i) {
                address token = tokens[i];

                // the entire token balance of the contract must be the rewards of the contract
                RewardState memory _state = rewardState[token];
                (uint256 lastBalance, uint256 index) = (_state.lastBalance, _state.index);

                uint256 accrued = IERC20(tokens[i]).balanceOf(vault) - lastBalance;
                uint256 deltaIndex;
                uint256 advanceBalance;
                if (totalShares != 0) {
                    deltaIndex = accrued.divDown(totalShares);
                    advanceBalance = deltaIndex.mulDown(totalShares);
                }

                if (index == 0) index = INITIAL_REWARD_INDEX;
                if (totalShares != 0) index += deltaIndex;

                rewardState[token] = RewardState({
                    index: index.Uint128(),
                    lastBalance: (lastBalance + advanceBalance).Uint128()
                });

                indexes[i] = index;
            }
        } else {
            for (uint256 i = 0; i < tokens.length; i++) {
                indexes[i] = rewardState[tokens[i]].index;
            }
        }
    }

[WP-N7] Comment inconsistent with implementation

tokenOut in PositionActionPenpie.sol#L134 represents the underlying amount of Pendle SY tokens.

 /// @notice Hook to decrease lever by withdrawing collateral from the CDPVault
/// @param leverParams LeverParams struct
/// @param subCollateral Amount of collateral to subtract in CDPVault decimals [wad]
/// @return tokenOut Amount of underlying token withdrawn from CDPVault [CDPVault.tokenScale()]
function _onDecreaseLever(
    LeverParams memory leverParams,
    uint256 subCollateral
) internal override returns (uint256 tokenOut) {
    _onWithdraw(leverParams.vault, leverParams.position, address(0), subCollateral, 0);
    (address pendleToken, , ) = abi.decode(leverParams.auxAction.args, (address, uint256, address));
    penpieHelper.withdrawMarket(pendleToken, subCollateral);
    if (leverParams.auxAction.args.length != 0) {
        bytes memory exitData = _delegateCall(
            address(poolAction),
            abi.encodeWithSelector(poolAction.exit.selector, leverParams.auxAction)
        );

        tokenOut = abi.decode(exitData, (uint256));
    }
}

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/proxy/PoolAction.sol#L332-L342

function exit(PoolActionParams memory poolActionParams) public returns (uint256 retAmount) {
    if (poolActionParams.protocol == Protocol.BALANCER) {
        retAmount = _balancerExit(poolActionParams);
    } else if (poolActionParams.protocol == Protocol.PENDLE) {
        retAmount = _pendleExit(poolActionParams);
    } else if (poolActionParams.protocol == Protocol.TRANCHESS) {
        retAmount = _tranchessExit(poolActionParams);
    } else if (poolActionParams.protocol == Protocol.SPECTRA) {
        retAmount = _spectraExit(poolActionParams);
    } else revert PoolAction__exit_unsupportedProtocol();
}
function _pendleExit(PoolActionParams memory poolActionParams) internal returns (uint256 retAmount) {
@@ 376,400 @@ (address market, uint256 netLpIn, address tokenOut) = abi.decode( poolActionParams.args, (address, uint256, address) ); (IStandardizedYield SY, IPPrincipalToken PT, IPYieldToken YT) = IPMarket(market).readTokens(); if (poolActionParams.recipient != address(this)) { IPMarket(market).transferFrom(poolActionParams.recipient, market, netLpIn); } else { IPMarket(market).transfer(market, netLpIn); } uint256 netSyToRedeem; if (PT.isExpired()) { (uint256 netSyRemoved, ) = IPMarket(market).burn(address(SY), address(YT), netLpIn); uint256 netSyFromPt = YT.redeemPY(address(SY)); netSyToRedeem = netSyRemoved + netSyFromPt; } else { (uint256 netSyRemoved, uint256 netPtRemoved) = IPMarket(market).burn(address(SY), market, netLpIn); bytes memory empty; (uint256 netSySwappedOut, ) = IPMarket(market).swapExactPtForSy(address(SY), netPtRemoved, empty); netSyToRedeem = netSyRemoved + netSySwappedOut; }
return SY.redeem(poolActionParams.recipient, netSyToRedeem, tokenOut, poolActionParams.minOut, true); }
 /// @notice Hook to decrease lever by withdrawing collateral from the CDPVault
/// @param leverParams LeverParams struct
/// @param subCollateral Amount of collateral to subtract in CDPVault decimals [wad]
/// @return tokenOut Amount of underlying token withdrawn from CDPVault [CDPVault.tokenScale()]
function _onDecreaseLever(
    LeverParams memory leverParams,
    uint256 subCollateral
) internal override returns (uint256 tokenOut) {
    _onWithdraw(leverParams.vault, leverParams.position, address(0), subCollateral, 0);
    (address pendleToken, , ) = abi.decode(leverParams.auxAction.args, (address, uint256, address));
    penpieHelper.withdrawMarket(pendleToken, subCollateral);
    if (leverParams.auxAction.args.length != 0) {
        bytes memory exitData = _delegateCall(
            address(poolAction),
            abi.encodeWithSelector(poolAction.exit.selector, leverParams.auxAction)
        );

        tokenOut = abi.decode(exitData, (uint256));
    }
}
function _pendleExit(PoolActionParams memory poolActionParams) internal returns (uint256 retAmount) {
@@ 376,400 @@ (address market, uint256 netLpIn, address tokenOut) = abi.decode( poolActionParams.args, (address, uint256, address) ); (IStandardizedYield SY, IPPrincipalToken PT, IPYieldToken YT) = IPMarket(market).readTokens(); if (poolActionParams.recipient != address(this)) { IPMarket(market).transferFrom(poolActionParams.recipient, market, netLpIn); } else { IPMarket(market).transfer(market, netLpIn); } uint256 netSyToRedeem; if (PT.isExpired()) { (uint256 netSyRemoved, ) = IPMarket(market).burn(address(SY), address(YT), netLpIn); uint256 netSyFromPt = YT.redeemPY(address(SY)); netSyToRedeem = netSyRemoved + netSyFromPt; } else { (uint256 netSyRemoved, uint256 netPtRemoved) = IPMarket(market).burn(address(SY), market, netLpIn); bytes memory empty; (uint256 netSySwappedOut, ) = IPMarket(market).swapExactPtForSy(address(SY), netPtRemoved, empty); netSyToRedeem = netSyRemoved + netSySwappedOut; }
return SY.redeem(poolActionParams.recipient, netSyToRedeem, tokenOut, poolActionParams.minOut, true); }

[WP-I8] Combined4626AggregatorV3Oracle Has Hidden Requirements for ERC4626 Implementation

For custom ERC4626 implementations, convertToAssets() may not accurately reflect the amount of redeemable assets.

Examples:

  • convertToAssets() may not include fees
  • Actual redemption may require application and queuing, making immediate redemption impossible
  • The underlying token can be hookable and allow readonly reentrancy
  • Other implementation errors of the ERC4626

It's critical to check and ensure the ERC4626 to be integrated is verified and audited for safe interaction.

Also, for the ERC4626 that incurs a fee in redemption, use previewRedeem() instead of convertToAssets() to get accurate output amounts.

https://github.com/LoopFi/loop-contracts/blob/911c64d324aab6f3f6a5b9dd9fc7c742a4596932/src/oracle/Combined4626AggregatorV3Oracle.sol#L19-L47

    constructor(address _aggregator, uint256 _aggregatorHeartbeat, address _vault) {
        aggregator = AggregatorV3Interface(_aggregator);
        aggregatorScale = 10 ** uint256(aggregator.decimals());
        aggregatorHeartbeat = _aggregatorHeartbeat;

        vault = IERC4626(_vault);
        vaultScale = 10 ** uint256(vault.decimals());
        assetScale = 10 ** IERC20Metadata(vault.asset()).decimals();
    }

@@ 29,34 @@ function getAggregatorData() public view returns (uint256, uint256) { (, int256 answer, , uint256 updatedAt, ) = aggregator.latestRoundData(); bool isValid = (answer > 0 && block.timestamp - updatedAt <= aggregatorHeartbeat); if (!isValid) revert Combined4626AggregatorV3Oracle__invalidAggregatorValue(); return (wdiv(uint256(answer), aggregatorScale), uint256(updatedAt)); }
/// @notice Return the latest price and the timestamp from the AggregatorV3 oracle function latestRoundData() public view returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) { (uint256 value, uint256 timestamp) = getAggregatorData(); uint256 redemptionRate = vault.convertToAssets(vaultScale); redemptionRate = wdiv(redemptionRate, assetScale); value = wmul(redemptionRate, value); return (0, int256(value), 0, timestamp, 0); }