xiaoming90 - Debt cannot be repaid without redeeming vault share

[sherlock-admin]

xiaoming90

medium

Debt cannot be repaid without redeeming vault share

Summary

Debt cannot be repaid without redeeming the vault share. As such, users have to redeem a certain amount of vault shares/strategy tokens at the current market price to work around this issue, which deprives users of potential gains from their vault shares if they maintain ownership until the end.

Vulnerability Detail

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L277

File: VaultAccountAction.sol
224:     function exitVault(
225:         address account,
226:         address vault,
227:         address receiver,
228:         uint256 vaultSharesToRedeem,
229:         uint256 lendAmount,
230:         uint32 minLendRate,
231:         bytes calldata exitVaultData
232:     ) external payable override nonReentrant returns (uint256 underlyingToReceiver) {
..SNIP..
261:         // If insufficient strategy tokens are redeemed (or if it is set to zero), then
262:         // redeem with debt repayment will recover the repayment from the account's wallet
263:         // directly.
264:         underlyingToReceiver = underlyingToReceiver.add(vaultConfig.redeemWithDebtRepayment(
265:             vaultAccount, receiver, vaultSharesToRedeem, exitVaultData
266:         ));

There is a valid scenario where users want to repay debt without redeeming their vault shares/strategy tokens (mentioned in the comments above "or if it is set to zero" at Line 251-263). In this case, the users will call exitVault with vaultSharesToRedeem parameter set to zero. The entire debt to be repaid will then be recovered directly from the account's wallet.

Following is the function trace of the VaultAccountAction.exitVault:

VaultAccountAction.exitVault
└─VaultConfiguration.redeemWithDebtRepayment
  └─VaultConfiguration._redeem
    └─IStrategyVault.redeemFromNotional
      └─MetaStable2TokenAuraVault._redeemFromNotional
        └─MetaStable2TokenAuraHelper.redeem
          └─Balancer2TokenPoolUtils._redeem
            └─StrategyUtils._redeemStrategyTokens

https://github.com/notional-finance/leveraged-vaults/blob/c707f7781e36d7a1259214dde2221f892a81a9c1/contracts/vaults/common/internal/strategy/StrategyUtils.sol#L153

File: StrategyUtils.sol
147:     function _redeemStrategyTokens(
148:         StrategyContext memory strategyContext,
149:         uint256 strategyTokens
150:     ) internal returns (uint256 poolClaim) {
151:         poolClaim = _convertStrategyTokensToPoolClaim(strategyContext, strategyTokens);
152: 
153:         if (poolClaim == 0) {
154:             revert Errors.ZeroPoolClaim();
155:         }

The problem is that if the vault shares/strategy tokens to be redeemed are zero, the poolClaim will be zero and cause a revert within the StrategyUtils._redeemStrategyTokens function call. Thus, users who want to repay debt without redeeming their vault shares/strategy tokens will be unable to do so.

Impact

Users cannot repay debt without redeeming their vault shares/strategy tokens. To do so, they have to redeem a certain amount of vault shares/strategy tokens at the current market price to work around this issue so that poolClaim > 0, which deprives users of potential gains from their vault shares if they maintain ownership until the end.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L277

Tool used

Manual Review

Recommendation

Within the VaultConfiguration.redeemWithDebtRepayment function, skip the vault share redemption if vaultShares is zero. In this case, the amountTransferred will be zero, and the subsequent code will attempt to recover the entire underlyingExternalToRepay amount directly from account's wallet.

function redeemWithDebtRepayment(
	VaultConfig memory vaultConfig,
	VaultAccount memory vaultAccount,
	address receiver,
	uint256 vaultShares,
	bytes calldata data
) internal returns (uint256 underlyingToReceiver) {
	uint256 amountTransferred;
	uint256 underlyingExternalToRepay;
	{
..SNIP..
+		if (vaultShares > 0) {
			// Repayment checks operate entirely on the underlyingExternalToRepay, the amount of
			// prime cash raised is irrelevant here since tempCashBalance is cleared to zero as
			// long as sufficient underlying has been returned to the protocol.
			(amountTransferred, underlyingToReceiver, /* primeCashRaised */) = _redeem(
				vaultConfig,
				underlyingToken,
				vaultAccount.account,
				receiver,
				vaultShares,
				vaultAccount.maturity,
				underlyingExternalToRepay,
				data
			); 
+		}
..Recover any unpaid debt amount from the account directly..
..SNIP..

Alternatively, update the StrategyUtils._redeemStrategyTokens function to handle zero vault share appropriately. However, note that the revert at Line 154 is added as part of mitigation to the "minting zero-share" bug in the past audit. Therefore, any changes to this part of the code must ensure that the "minting zero-share" bug is not being re-introduced. Removing the code at 153-155 might result in the user's vault share being "burned" but no assets in return under certain conditions.

File: StrategyUtils.sol
147:     function _redeemStrategyTokens(
148:         StrategyContext memory strategyContext,
149:         uint256 strategyTokens
150:     ) internal returns (uint256 poolClaim) {
151:         poolClaim = _convertStrategyTokensToPoolClaim(strategyContext, strategyTokens);
152: 
153:         if (poolClaim == 0) {
154:             revert Errors.ZeroPoolClaim();
155:         }

[jeffywu]

Agree with the issue, however, the fix will be to remove the error from _redeemStrategyTokens. We should always call the vault in case there is something it needs to do during the vault action.

[jeffywu]

Fixed in: notional-finance/leveraged-vaults#54

[xiaoming9090]

@0xleastwood + @xiaoming9090 : Verified. Fixed in the codebase of Notional's Leverage Vault in PR notional-finance/leveraged-vaults#54