When redeeming from the Pendle Adapter, liquidity is removed from the Pendle Router in exchange for the base asset (USDe or wstETH.)
function _protocolWithdraw(
uint256 amount,
uint256
) internal virtual override {
address asset = asset();
uint256 protocolAmount = amount - IERC20(asset).balanceOf(address(this));
// Empty structs
LimitOrderData memory limitOrderData;
SwapData memory swapData;
TokenOutput memory tokenOutput = TokenOutput(
asset,
protocolAmount, // @audit-issue overly restrictive slippage tolerance breaks redemption/withdrawals
asset,
address(0),
swapData
);
pendleRouter.removeLiquiditySingleToken(
address(this),
pendleMarket,
amountToLp(amount, totalAssets()),
tokenOutput,
limitOrderData
);
}The 2nd field for TokenOutput is the minTokenOut and sets the expected amount received from Pendle. This serves as slippage protection but it is overly restrictive in this case. It will revert with precision loss of just 1.
An attacker can also decrease the assets returned by Pendle by decreasing the Pendle Market's totalSy prior to a user's withdrawal from the Adapter. This breaks withdrawal for the user.
Recommendation: Consider setting the slippage protection in some way. This needs to be a static value that can not be manipulated via MEV. Typically, this value is set by the function caller as the minimum amount they expect to receive.
The Pendle Adapter inherits from the Adapter Base which implements some of the withdraw logic.
function _withdraw(
address caller,
address receiver,
address owner,
uint256 assets,
uint256 shares
) internal virtual override {
if (shares == 0 || assets == 0) revert ZeroAmount();
if (caller != owner) {
_spendAllowance(owner, caller, shares);
}
if (!paused()) {
_protocolWithdraw(assets, shares);
}
_burn(owner, shares);
// @audit-issue this fails when assets withdrawn via _protocolWithdraw are less
IERC20(asset()).safeTransfer(receiver, assets);
if (autoHarvest) harvest();
emit Withdraw(caller, receiver, owner, assets, shares);
}When the Adapter holds no balance of the base asset, _protocolWithdraw() is expected to withdraw at least the exact amount of assets or else the withdraw would revert. In the case of Pendle, withdrawing from the protocol could be removing liquidity tokens from a Market. The amount of assets withdrawn for removing liquidity is based on the totalSy and totalSupply of the market which can be manipulated.
An attacker can front-run the withdrawal by removing SY tokens from the Pendle Market to decrease the amount of assets withdrawn by the user by at least 1. When the user receives 1 less asset, the transfer of assets will fail due to lack of balance.
Recommendation: Consider overriding withdraw and replace its transfer logic to transfer only the amount of assets received from the protocol withdrawal. Since the amount can be less or more, the transfer amount can be computed by subtracting the balance prior to the protocol withdrawal from the balance after the protocol withdrawal.
Pauses withdraw the Adapter's whole balance from the Pendle Market and unpauses deposit its whole asset balance into Pendle.
function unpause() external onlyOwner {
// @audit-issue this can be frontrun. greater losses in these cases since larger amounts are deposited
_protocolDeposit(totalAssets(), totalSupply());
_unpause();
}No slippage protection is applied when the Adapter deposits into Pendle. This leaves the large deposit open to sandwich attacks that can steal a large chunk of the Adapter's assets.
function _protocolDeposit(
uint256 amount,
uint256
) internal virtual override {
// ... snip ...
pendleRouter.addLiquiditySingleToken(
address(this),
pendleMarket,
0, // @audit-issue no slippage protection
approxParams,
tokenInput,
limitOrderData
);
}Recommendation: Consider implementing slippage protection somehow where the user is able to set the minimum amount of LP tokens it receives from Pendle.
Every time harvest data is set, a token's reward data is pushed on to the rewardTokensData array.
for (uint256 i = 0; i < len; i++) {
rewardTokensData.push(rewData[i]);
_approveSwapTokens(rewData[i].pathAddresses, _balancerRouter);
}When setting harvest data is run more than once, the rewardTokensData will have repeated elements. In the case of adding a new reward token,
the owner will need to set harvest data again and include the new reward token. However, this will lead to harvest() being unable
to claim the new reward token because the only part of the rewardTokensData will be accessed.
function harvest() public override takeFees {
// ... snip ...
address[] memory rewTokens = _getRewardTokens();
uint256 rewLen = rewTokens.length;
for (uint256 i = 0; i < rewLen; i++) {
address rewardToken = rewTokens[i];
amount = IERC20(rewardToken).balanceOf(address(this));
BalancerRewardTokenData memory rewData = rewardTokensData[i];
if (amount > rewData.minTradeAmount) {
// perform all the balancer single swaps for this reward token
// ... snip ...
}
}
}Note that only the rewardTokensData within the length of rewTokens will be accessed and rewTokens will have repeated elements and will be larger than rewTokens after the second call.
Some SY tokens, like SY tokens for Balancer LPs, support adding reward tokens.
Recommendation:
Consider using a mapping for rewardTokensData instead of an array.
When assets are withdrawn from the Pendle Adapter, the Adapter converts the asset amount so it knows the amount of LP tokens to withdraw from the Pendle Market. In the most common case, it does the conversion with the following:
lpBalance * assetAmount / totalAssets
lpBalance - The balance of LP tokens held by the Adapter
assetAmount - Amount of assets to be withdrawn
totalAssets = asset balance in this adapter + asset balance in LP
Th issue lies in totalAssets which includes the Adapter's asset balance. The correct computation would be for totalAssets
to only include the asset balance deposited by the Adapter in the LP.
If 10% of total assets is the asset balance of the Adapter, then the conversion result will be considerably less than it should be. Withdrawals could revert because of this if there is not enough assets in the Adapter to satisfy the withdrawal. This becomes more of an issue when protocol deposits are delayed.
Recommendation:
Consider fixing the conversion to LP amount by deducting the Adapter's asset balance from the totalAssets. Note that it may also be necessary to check that total assets is more than the asset balance before the subtraction.
function amountToLp(
uint256 amount,
uint256 totAssets // totalAssets - asset balance + assets deposited in the Market
) internal view returns (uint256 lpAmount) {
uint256 lpBalance = IERC20(pendleMarket).balanceOf(address(this));
+ totAssets -= asset().balanceOf(address(this));
amount == totAssets
? lpAmount = lpBalance
: lpAmount = lpBalance.mulDiv(amount, totAssets, Math.Rounding.Ceil);
}Both Pendle Harvest contracts do not implement any slippage protections when swapping their claimed reward tokens for the asset token.
// In `PendleAdapterBalancerCurveHarvest::harvest()`
curveRouter.exchange(curveSwap.route, curveSwap.swapParams, amount, 0, curveSwap.pools);// In `PendleAdapterBalancerCurveHarvest::_singleBalancerSwap()` and `PendleAdapterBalancerHarvest::_singleBalancerSwap()`
amountOut = balancerRouter.swap(
swap,
fundManagement,
0, // @audit-issue no slippage protection
block.timestamp + swapDelay
);This leaves harvesting open to sandwich attacks that cause maximum slippage.
Recommendation: Consider adding slippage protection for all swaps done by the Pendle Harvest contracts.
[M-04] Unpausing can break when the Adapter's asset balance is more than the Pendle Market's supply cap
Some Pendle Markets have supply caps that need to be considered. However, when the Adapter deposits assets into the Pendle Market, it does not check whether the market has enough supply cap. The Adapter will deposit the maxDeposit amount if the amount happens to be equal to maxDeposit and it will deposit the asset balance if it is not equal.
ref: PendleAdapter::_protocolDeposit()
uint256 netInput = amount == maxDeposit(address(this))
? amount
: IERC20(asset).balanceOf(address(this));This can break unpausing since unpausing will try to deposit the Adapter's total assets into Pendle. When the Adapter's total assets is more than the supply cap of the SY token, unpausing will fail indefinitely.
Recommendation:
Consider using the minimum of the asset balance and maxDeposit as the deposit amount for Pendle. This ensures that deposit amount will not exceed the SY token's supply cap.
When the Adapter deposits assets into Pendle, it compares the amount of assets to be deposited with maxDeposit.
uint256 netInput = amount == maxDeposit(address(this))
? amount
: IERC20(asset).balanceOf(address(this));However, maxDeposit is not a value denominated in the base asset. It is in SY token which would need to be converted to the base asset.
function maxDeposit(address who) public view override returns (uint256) {
try ISYTokenV3(pendleSYToken).supplyCap() returns (uint256 supplyCap) {
return supplyCap - ISYTokenV3(pendleSYToken).totalSupply();
} catch {
return super.maxDeposit(who);
}
}This can cause Adapter unpauses to revert indefinitely.
Recommendation: Consider converting the result of max deposit to the base asset by multiplying it with the exchange rate. The following pseudocode can serve as reference.
// remaining supplyCap * pendleSYToken.exchangeRate(10 ** pendleSYToken.decimals())
uint256 remCap = supplyCap - ISYTokenV3(pendleSYToken).totalSupply();
return remCap * pendleSYToken.exchangeRate(10 ** pendleSYToken.decimals()) / exchangeRateDecimals;[L-01] Token approvals will fail for tokens that do not have return values for their ERC20 functions
There are token approvals done in PendleAdapterBalancerCurveHarvest, PendleAdapter, and PendleAdapterBalancerHarvest. All of which
use the IERC20 interface that expects approve() to return a boolean. However, not all tokens have return values for their ERC20 functions.
The most prominent example of this is USDT.
Recommendation:
Consider using OpenZeppelin's SafeERC20.forceApprove() which supports ERC20 tokens with no return values for their ERC20 functions and with approval race protections.
Withdrawals from the Pendle Adapter triggers withdrawals from Pendle. The amount withdrawn, however, can not be more than the Adapter's asset balance.
function _protocolWithdraw(
uint256 amount,
uint256
) internal virtual override {
address asset = asset();
uint256 protocolAmount = amount - IERC20(asset).balanceOf(address(this));This scenario is more likely when asset balances are not automatically deposited on every deposit or when the strategy withdraws assets with strategyWithdraw(). In both cases, the Adapter will hold a greater asset balance.
Recommendation: Consider checking that the withdrawal amount is less than the asset balance and only withdraw from Pendle if the Adapter does not have enough assets to satisfy the withdrawal.
minOutis 0 atBaseCurveCompound::sellRewardsViaCurve(). Consider encoding 3 different values indata. One would be the minOut for _protocolDeposit(), 2nd would be the minOut for Curve, and 3rd would be the limits array for Balancer. Regarding Balancer - the swap is GIVEN_IN and if I’m not mistaken, setting negative limits is the way to set minOut for the assets withdrawn from the Balancer Vault. Thelimitsset in the tests all use really large positive values. - Having limits set via a state variable won’t help in protecting against slippage since the amounts traded can vary widely. May be worth considering the above recommended fix of passing Curve and Balancer minOuts viadatain HarvestpushFunds().assetsamount can be specified by the owner/keeper so that it is within the supply cap of the SY Token.decimalsshould reflect the accounting asset’sdecimals.forceApprovealready sets allowance to 0 before setting the new allowance. The following lines can be removed. No need to reset to 0 here No need to remove old allowances here No need to reset to 0 here_protocolWithdraw()