Unable To Recover Funds From Account

Summary

Due to how tokens with a transfer fee are handled within the custom transfer function, it is not possible for Notional to recover funds from the account.

Vulnerability Detail

The following transfer function is used within the VaultConfiguration._redeem function. Under normal circumstances, the actualTransferExternal returned is equal to the netTransferExternal. On Line 210-215, if the token has a transfer fee, the netDeposit will be returned instead. Note that the netDeposit will always be less than the netTransferExternal and this will be the root cause of this issue.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/balances/TokenHandler.sol#L190

File: TokenHandler.sol
188:     /// @notice Handles transfers into and out of the system denominated in the external token decimal
189:     /// precision.
190:     function transfer(
191:         Token memory token,
192:         address account,
193:         uint256 currencyId,
194:         int256 netTransferExternal
195:     ) internal returns (int256 actualTransferExternal) {
196:         // This will be true in all cases except for deposits where the token has transfer fees. For
197:         // aTokens this value is set before convert from scaled balances to principal plus interest
198:         actualTransferExternal = netTransferExternal;
199: 
200:         if (token.tokenType == TokenType.aToken) {
201:             Token memory underlyingToken = getUnderlyingToken(currencyId);
202:             // aTokens need to be converted when we handle the transfer since the external balance format
203:             // is not the same as the internal balance format that we use
204:             netTransferExternal = AaveHandler.convertFromScaledBalanceExternal(
205:                 underlyingToken.tokenAddress,
206:                 netTransferExternal
207:             );
208:         }
209: 
210:         if (netTransferExternal > 0) {
211:             // Deposits must account for transfer fees.
212:             int256 netDeposit = _deposit(token, account, uint256(netTransferExternal));
213:             // If an aToken has a transfer fee this will still return a balance figure
214:             // in scaledBalanceOf terms due to the selector
215:             if (token.hasTransferFee) actualTransferExternal = netDeposit;
216:         } else if (token.tokenType == TokenType.Ether) {
217:             // netTransferExternal can only be negative or zero at this point
218:             GenericToken.transferNativeTokenOut(account, uint256(netTransferExternal.neg()));
219:         } else {
220:             GenericToken.safeTransferOut(
221:                 token.tokenAddress,
222:                 account,
223:                 // netTransferExternal is zero or negative here
224:                 uint256(netTransferExternal.neg())
225:             );
226:         }
227:     }

Within the VaultConfiguration._redeem function, the above-mentioned transfer function will be triggered at Line 643 if the amountTransferred < underlyingExternalToRepay. This code block will execute if there are insufficient strategy tokens to repay debts and Notional will attempt to recover the remaining tokens from the account directly.

Assume that at Line 623, the value of state variables are as follows:

underlyingExternalToRepay = 100
amountTransferred = 90

Since amountTransferred < underlyingExternalToRepay is true, residualRequired will be set to 10 at Line 628.

At Line 643, it will attempt to recover the residualRequired (10) amount of tokens from the account. Assume that this token has a transfer fee, the actual amount received by Notional is 8 tokens. Thus, the netDeposit will be 8, and subsequently, theactualTransferExternal will be set to 8.

At Line 646, the following code will be executed, and therefore the amountTransferred will be set to 98

amountTransferred = amountTransferred.add(actualTransferExternal.toUint()); // @audit-info 98 = 90 + 8

At Line 651, the following code will always be evaluated as False and cause a revert because the amountTransferred will never be equal or larger than underlyingExternalToRepay if the token has a transfer fee.

require(amountTransferred >= underlyingExternalToRepay, "Insufficient repayment"); // @audit-info amountTransferred=98, underlyingExternalToRepay=100 evaluate to false

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

File: VaultConfiguration.sol
569:     /// @notice This will call the strategy vault and have it redeem the specified amount of strategy tokens
570:     /// for underlying. The amount of underlying required to repay the debt will be transferred back to the protocol
571:     /// and any excess will be returned to the account. If the account does not redeem sufficient strategy tokens to repay
572:     /// debts then this method will attempt to recover the remaining underlying tokens from the account directly.
573:     /// @param vaultConfig vault config
574:     /// @param params redemption parameters
575:     /// @param data arbitrary data to pass to the vault
576:     /// @return assetCashInternalRaised the amount of asset cash (positive) that was returned to repay debts
577:     /// @return underlyingToReceiver the amount of underlying that was returned to the receiver from the vault
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:     }

Impact

The protocol is unable to recover funds from the account if the token has a transfer fee.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider implementing any of the following mitigations:

Update the transfer function used at Line 643 and the require check at Line 651 within the VaultConfiguration._redeem function accordingly so that they are in sync.
Ensure that tokens with transfer fees are never supported within the vault

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Unable To Recover Funds From Account #52