xiaoming90

high

Permissionless `settleVault` Function Within `CrossCurrencyfCashVault` Vault Can Be Exploited To Steal Profit

Summary

The permissionless settleVault function within CrossCurrencyfCashVault vault can be exploited to steal the profit of the vaults.

Vulnerability Detail

The CrossCurrencyfCashVault.settleVault function is permissionless and can be called by anyone.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyfCashVault.sol#L121

File: CrossCurrencyfCashVault.sol
121:     function settleVault(uint256 maturity, uint256 strategyTokens, bytes calldata settlementTrade) external {
122:         require(maturity <= block.timestamp, "Cannot Settle");
123:         VaultState memory vaultState = NOTIONAL.getVaultState(address(this), maturity);
124:         require(vaultState.isSettled == false);
125:         require(vaultState.totalStrategyTokens >= strategyTokens);
             ..SNIP..

Following is an example of a CrossCurrencyfCashVault.settleVault call taken from the test scripts for reference.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/tests/test_cross_currency.py#L265

File: test_cross_currency.py
265:     txn = usdcDaiVault.settleVault(
266:         maturity,
267:         vaultState['totalStrategyTokens'],
268:         encode_redeem_params(
269:             minPurchaseAmount=Wei(129_500e6),
270:             maxBorrowRate=0,
271:             dexId='UNISWAP_V3',
272:             exchangeData={
273:                 'fee': 100
274:             }
275:         ),
276:         {"from": accounts[1]}
277:     )

When calling the CrossCurrencyfCashVault.settleVault function, the caller can specify the redemption configuration by configuring the RedeemParams. The caller can instruct the leverage vault on how the trading of the lending underlying assets (e.g. DAI) back to borrowing underlying assets (e.g. USDC) should occur. Currently, the Notional Trade Module supports trading with slippage limits across multiple DEX protocols (Curve, Balancer V2, Uniswap V2 & V3 and 0x).

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyfCashVault.sol#L52

File: CrossCurrencyfCashVault.sol
52:     struct RedeemParams {
53:         // Minimum purchase amount of the borrow underlying token, this is
54:         // based on the amount of lend underlying received and must be set to a non-zero
55:         // value to establish a slippage limit.
56:         uint256 minPurchaseAmount;
57:         // Maximum annualized borrow rate, can be set to zero for no slippage limit
58:         uint32 maxBorrowRate;
59:         // ID of the desired DEX to trade on, _depositFromNotional will always trade
60:         // using an EXACT_IN_SINGLE trade which is supported by all DEXes
61:         uint16 dexId;
62:         // Exchange data depending on the selected dexId
63:         bytes exchangeData;
64:     }

The minPurchaseAmount within the RedeemParams specifies the slippage limit of the trade. If it is set to zero, there is no slippage limit.

An attacker can call the CrossCurrencyfCashVault.settleVault function with no slippage limit by configuring minPurchaseAmount to 0, thus giving the attacker the ability to force the vault to trade with a DEX/pool and cause it to suffer huge slippage.

During vault settlement, the VaultAccount._redeemStrategyTokensToCashInternal function is triggered. Within the function, it will attempt to redeem the strategy tokens without any debt repayment by calling the VaultConfiguration.redeemWithoutDebtRepayment function.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAction.sol#L132

File: VaultAction.sol
131:     /// @notice Redeems strategy tokens to cash
132:     function _redeemStrategyTokensToCashInternal(
133:         VaultConfig memory vaultConfig,
134:         uint256 maturity,
135:         uint256 strategyTokensToRedeem,
136:         bytes calldata vaultData
137:     ) private nonReentrant returns (int256 assetCashRequiredToSettle, int256 underlyingCashRequiredToSettle) {
138:         // If the vault allows further re-entrancy then set the status back to the default
139:         if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
140:             reentrancyStatus = _NOT_ENTERED;
141:         }
142: 
143:         VaultState memory vaultState = VaultStateLib.getVaultState(vaultConfig.vault, maturity);
144:         (int256 assetCashReceived, uint256 underlyingToReceiver) = vaultConfig.redeemWithoutDebtRepayment(
145:             vaultConfig.vault, strategyTokensToRedeem, maturity, vaultData
146:         );
147:         require(assetCashReceived > 0);
148:         // Safety check to ensure that the vault does not somehow receive tokens in this scenario
149:         require(underlyingToReceiver == 0);
150: 
151:         vaultState.totalAssetCash = vaultState.totalAssetCash.add(uint256(assetCashReceived));
152:         vaultState.totalStrategyTokens = vaultState.totalStrategyTokens.sub(strategyTokensToRedeem);
153:         vaultState.setVaultState(vaultConfig.vault);
154: 
155:         emit VaultRedeemStrategyToken(vaultConfig.vault, maturity, assetCashReceived, strategyTokensToRedeem);
156:         return _getCashRequiredToSettle(vaultConfig, vaultState, maturity);
157:     }

The VaultConfiguration.redeemWithoutDebtRepayment function will in turn call the internal _redeem function. Notice that since no debt repayment is required, the RedeemParams.assetInternalToRepayDebt is set to 0 in the parameters.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L521

File: VaultConfiguration.sol
520:     /// @notice Redeems without any debt repayment and sends tokens back to the account
521:     function redeemWithoutDebtRepayment(
522:         VaultConfig memory vaultConfig,
523:         address account,
524:         uint256 strategyTokens,
525:         uint256 maturity,
526:         bytes calldata data
527:     ) internal returns (int256 assetCashInternalRaised, uint256 underlyingToReceiver) {
528:         // Asset cash internal raised is only used by the vault, in all other cases it
529:         // should return 0
530:         (assetCashInternalRaised, underlyingToReceiver) = _redeem(
531:             vaultConfig, 
532:             RedeemParams(account, account, strategyTokens, maturity, 0),
533:             data
534:         );
535:     }

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L562

File: VaultConfiguration.sol
558:     // @param account address of the account to pass to the vault
559:     // @param strategyTokens amount of strategy tokens to redeem
560:     // @param maturity the maturity of the vault shares
561:     // @param assetInternalToRepayDebt amount of asset cash in internal denomination required to repay debts
562:     struct RedeemParams {
563:         address account;
564:         address receiver;
565:         uint256 strategyTokens;
566:         uint256 maturity;
567:         int256 assetInternalToRepayDebt;
568:     }

Within the _redeem function, since params.assetInternalToRepayDebt is set to 0, underlyingExternalToRepay is set to zero, which means that no debt repayment is required. The entire code block from Line 588 to 598 will be skipped.

At Line 619, it calls the IStrategyVault(vaultConfig.vault).redeemFromNotional function to instruct the vault to perform the following steps:

Redeem the strategy tokens (e.g. fDAI) to lending underlying token (DAI)
Swap the lending underlying token (DAI) for borrowing underlying token (USDC) via the DEX (e.g. Uniswap, Curve, 0x)
If an attacker configures RedeemParams.minPurchaseAmount to zero when calling CrossCurrencyfCashVault.settleVault function, the trade will occur with no slippage control. In the worst-case scenario, it is possible to swap 1,000,000 DAI (lending underlying token) for only 10 USDC (borrowing underlying token) in an extremely imbalanced Uniswap pool manipulated by a flash-loan attack.

At Line 623, the amountTransferred will be the amount of underlying borrowing token (USDC) received by the vault during the DEX's trade. In the previous worst-case scenario, if the vault only received 10 USDC, the amountTransferred will be set to 10.

Since amountTransferred=10 and underlyingExternalToRepay=0:

The entire code block from Line 626 to 649 will be skipped since (amountTransferred < underlyingExternalToRepay) evaluate to false
The require statement at Line 651 will pass.

At Line 659, 10 USDC worth of asset cash (cUSDC) will be minted to the vault. After the vault settlement, the vault hold 0 strategy token and 10 USDC worth of asset cash (cUSDC) assuming the worst-case scenario described above. Notice that this is a significant deviation from the 1,000,000 DAI valuation of the vault before the settlement.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L578

File: VaultConfiguration.sol
578:     function _redeem(
579:         VaultConfig memory vaultConfig,
580:         RedeemParams memory params,
581:         bytes calldata data
582:     ) internal returns (int256 assetCashInternalRaised, uint256 underlyingToReceiver) {
583:         (Token memory assetToken, Token memory underlyingToken) = getTokens(vaultConfig);
584: 
585:         // Calculates the amount of underlying tokens required to repay the debt, adjusting for potential
586:         // dust values.
587:         uint256 underlyingExternalToRepay;
588:         if (params.assetInternalToRepayDebt < 0) {
589:             if (assetToken.tokenType == TokenType.NonMintable) {
590:                 // NonMintable token amounts are exact and don't require any asset rate conversions or adjustments
591:                 underlyingExternalToRepay = params.assetInternalToRepayDebt.neg().toUint();
592:             } else {
593:                 // Mintable tokens require an off by one adjustment to account for rounding errors between
594:                 // transfer and minting
595:                 int256 x = vaultConfig.assetRate.convertToUnderlying(params.assetInternalToRepayDebt).neg();
596:                 underlyingExternalToRepay = underlyingToken.convertToUnderlyingExternalWithAdjustment(x).toUint();
597:             }
598:         }
599: 
600:         uint256 amountTransferred;
601:         if (params.strategyTokens > 0) {
602:             uint256 balanceBefore = underlyingToken.balanceOf(address(this));
603:             // There are four possibilities here during the transfer:
604:             //   1. If the account == vaultConfig.vault then the strategy vault must always transfer
605:             //      tokens back to Notional. underlyingToReceiver will equal 0, amountTransferred will
606:             //      be the value of the redemption.
607:             //   2. If the account has debt to repay and is redeeming sufficient tokens to repay the debt,
608:             //      the vault will transfer back underlyingExternalToRepay and transfer underlyingToReceiver
609:             //      directly to the receiver.
610:             //   3. If the account has redeemed insufficient tokens to repay the debt, the vault will transfer
611:             //      back as much as it can (less than underlyingExternalToRepay) and underlyingToReceiver will
612:             //      be zero. If this occurs, then the next if block will be triggered where we attempt to recover
613:             //      the shortfall from the account's wallet.
614:             //   4. During liquidation, the liquidator will redeem their strategy token profits without any debt
615:             //      to repay (underlyingExternalToRepay == 0). This means that all the profits will be returned
616:             //      to the liquidator (params.receiver) from the vault (underlyingToReceiver will be the full value
617:             //      of the redemption) and amountTransferred will equal 0. A similar scenario will occur when
618:             //      accounts exit post maturity and have no debt associated with their account.
619:             underlyingToReceiver = IStrategyVault(vaultConfig.vault).redeemFromNotional(
620:                 params.account, params.receiver, params.strategyTokens, params.maturity, underlyingExternalToRepay, data
621:             );
622:             uint256 balanceAfter = underlyingToken.balanceOf(address(this));
623:             amountTransferred = balanceAfter.sub(balanceBefore);
624:         }
625: 
626:         if (amountTransferred < underlyingExternalToRepay) {
627:             // Recover any unpaid debt amount from the account directly
628:             uint256 residualRequired = underlyingExternalToRepay - amountTransferred;
629: 
630:             // Since ETH does not allow pull payment, the account needs to transfer sufficient
631:             // ETH to repay the debt. We don't use WETH here since the rest of Notional does not
632:             // use WETH. Any residual ETH is transferred back to the account. Vault actions that
633:             // do not repay debt during redeem will not enter this code block.
634:             if (underlyingToken.tokenType == TokenType.Ether) {
635:                 require(residualRequired <= msg.value, "Insufficient repayment");
636:                 // Transfer out the unused part of msg.value, we've received all underlying external required
637:                 // at this point
638:                 GenericToken.transferNativeTokenOut(params.account, msg.value - residualRequired);
639:                 amountTransferred = underlyingExternalToRepay;
640:             } else {
641:                 // actualTransferExternal is a positive number here to signify assets have entered
642:                 // the protocol
643:                 int256 actualTransferExternal = underlyingToken.transfer(
644:                     params.account, vaultConfig.borrowCurrencyId, residualRequired.toInt()
645:                 );
646:                 amountTransferred = amountTransferred.add(actualTransferExternal.toUint());
647:             }
648:         }
649:         // amountTransferred should never be much more than underlyingExternalToRepay (it should
650:         // be exactly equal) as long as the vault behaves according to spec.
651:         require(amountTransferred >= underlyingExternalToRepay, "Insufficient repayment");
652: 
653:         // NonMintable tokens do not need to be minted, the amount transferred is the amount
654:         // of asset cash raised.
655:         int256 assetCashExternal;
656:         if (assetToken.tokenType == TokenType.NonMintable) {
657:             assetCashExternal = amountTransferred.toInt();
658:         } else if (amountTransferred > 0) {
659:             assetCashExternal = assetToken.mint(vaultConfig.borrowCurrencyId, amountTransferred);
660:         }
661: 
662:         // Due to the adjustment in underlyingExternalToRepay, this returns a dust amount more
663:         // than the value of assetInternalToRepayDebt. This value is only used when we are
664:         // redeeming strategy tokens to the vault.
665:         assetCashInternalRaised = assetToken.convertToInternal(assetCashExternal);
666:     }

What happens if there is a cash shortfall within the vault after the settlement?

Per the code and comment on Line 406-411, Notional does not allow the CrossCurrencyfCashVault vault to have a cash shortfall. If there is a cash shortfall after all the strategy tokens have been redeemed to asset cash (cUSDC), it will resolve the shortfall using the protocol reserve by calling the VaultConfiguration.resolveShortfallWithReserve function. Since protocol reserves are being used to fill up the cash shortfall within the vault, it entails the loss of assets for the protocol if such an incident occurs.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAction.sol#L375

File: VaultAction.sol
372:     /// @notice Settles a vault and sets the final settlement rates
373:     /// @param vault the vault to settle
374:     /// @param maturity the maturity of the vault
375:     function settleVault(address vault, uint256 maturity) external override nonReentrant {
..SNIP..
405: 
406:         if (vaultState.totalAssetCash < assetCashRequiredToSettle) {
407:             // Don't allow the pooled portion of the vault to have a cash shortfall unless all
408:             // strategy tokens have been redeemed to asset cash.
409:             require(vaultState.totalStrategyTokens == 0, "Redeem all tokens");
410: 
411:             // After this point, we have a cash shortfall and will need to resolve it.
412:             // Underflow checked above
413:             int256 assetCashShortfall = (assetCashRequiredToSettle - vaultState.totalAssetCash).toInt();
414:             uint256 assetCashRaised = VaultConfiguration.resolveShortfallWithReserve(
415:                 vaultConfig.vault, vaultConfig.borrowCurrencyId, assetCashShortfall, maturity
416:             ).toUint();
417: 
418:             vaultState.totalAssetCash = vaultState.totalAssetCash.add(assetCashRaised);
419:             vaultState.setVaultState(vault);
420:         }
..SNIP..

Proof-of-Concept

Following are the possible attack paths against the CrossCurrencyfCashVault vault

Attack 1

The CrossCurrencyfCashVault USDC/DAI vault is ready to be settled.
The attacker borrows a large amount of USDC using a flash-loan
The attacker deposits a portion of the USDC to Uniswap pool to mint LP shares
The attacker swaps a portion of the USDC to DAI in a Uniswap pool, leaving the pool with almost no DAI liquidity. This debalances the exchange rate between the USDC and DAI in the Uniswap pool.
The attacker calls the settleVault function with minPurchaseAmount set to 0, which means there is no slippage control. This makes the vault trade with the imbalanced Uniswap pool at an unfavorable exchange rating, resulting in huge slippage.
Trading is a zero-sum game. During the trade, the vault losses are another party's gain. In this case, it is the Liquidity Providers of the Uniswap pool gain from Notional's vault losses. The attacker proceeds to withdraw its LP shares from the Uniswap pool and exit the Notional's leverage vault.
If the gain from the LP position + the assets returned from the exit vault is more than the cost of the attack, this attack can be profitable to carry out.
This attack does not have to be performed against Uniswap pool only. Since the vault supports multiple DEX protocols (Curve, Balancer V2, Uniswap V2 & V3 and 0x), it is up to the attacker's creativity to come out with other possible attack paths to exploit the trustless settleVault function. Also, the attacker can always choose the Protocol and/or Pool with the least liquidity out of the many options to imbalance it to facilitate the attack.

Side Note 1: This issue reassembles some attacks seen in the past where the attacker takes advantage of the permissionless Yearn's earn() function. Refer to https://github.com/yearn/yearn-security/blob/master/disclosures/2020-10-30.md and

https://github.com/yearn/yearn-security/blob/master/disclosures/2021-02-04.md.

Side Note 2: This attack can be carried out without a flash-loan if the attacker has enough funds to obtain majority of the vault shares.

Attack 2

Assume there is a protocol called XYZ that offers fixed-rate, fixed-term crypto asset lending and borrowing. XYZ sees Notional as its only direct competitor. XYZ has strong financial backing and is willing to do whatever it takes to sabotage Notional. The cost of attack is not so much of a concern for XYZ as the main goal is to damage the community and reputation of Notional, so that XYZ will benefit in the long run.

Whenever any of the vaults is ready to be settled, the attacker will intentionally imbalance a pool within one of the DEX protocols supported by Notional Trading Module.
The attacker calls the settleVault function with minPurchaseAmount set to 0, which means there is no slippage control. This makes the vault to trade with the imbalanced Uniswap pool at an unfavorable exchange rating, resulting in huge slippage.
This will cause a shortfall within the vault. When there is a cash shortfall, Notional will resolve the shortfall using the protocol reserve.
Repeat the above attack multiple times against the vaults until all the protocol reserves are drained.

Impact

Loss of assets for protocol and its users

Code Snippet

Tool used

Manual Review

Recommendation

Consider making the settleVault function permissioned. Additionally, the security risks of allowing the settleVault function to be called by anyone outweigh the benefits of this function being permissionless.

CrossCurrencyfCashVault vault relies on the TradeModule.executeTrade to execute its trade. Alternatively, the settleVault function can be kept permissionless, but consider using the Trade.executeTradeWithDynamicSlippage function with hardcoded dynamicSlippageLimit to reduce the impact.

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Permissionless `settleVault` Function Within `CrossCurrencyfCashVault` Vault Can Be Exploited To Steal Profit #81

Permissionless `settleVault` Function Within `CrossCurrencyfCashVault` Vault Can Be Exploited To Steal Profit

Summary

Vulnerability Detail

What happens if there is a cash shortfall within the vault after the settlement?

Proof-of-Concept

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Permissionless `settleVault` Function Within `CrossCurrencyfCashVault` Vault Can Be Exploited To Steal Profit #81

Permissionless settleVault Function Within CrossCurrencyfCashVault Vault Can Be Exploited To Steal Profit

Summary

Vulnerability Detail

What happens if there is a cash shortfall within the vault after the settlement?

Proof-of-Concept

Impact

Code Snippet

Tool used

Recommendation

Permissionless `settleVault` Function Within `CrossCurrencyfCashVault` Vault Can Be Exploited To Steal Profit