xiaoming90 - Vault's totalStrategyTokenGlobal will not be in sync

[github-actions]

xiaoming90

high

Vault's totalStrategyTokenGlobal will not be in sync

Summary

The strategyContext.vaultState.totalStrategyTokenGlobal variable that tracks the number of strategy tokens held in the vault will not be in sync and will cause accounting issues within the vault.

Vulnerability Detail

This affects both the TwoToken and Boosted3Token vaults

The StrategyUtils._convertStrategyTokensToBPTClaim function might return zero if a small number of strategyTokenAmount is passed into the function. If (strategyTokenAmount * context.vaultState.totalBPTHeld) is smaller than context.vaultState.totalStrategyTokenGlobal, the bptClaim will be zero.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/strategy/StrategyUtils.sol#L18

File: StrategyUtils.sol
17:     /// @notice Converts strategy tokens to BPT
18:     function _convertStrategyTokensToBPTClaim(StrategyContext memory context, uint256 strategyTokenAmount)
19:         internal pure returns (uint256 bptClaim) {
20:         require(strategyTokenAmount <= context.vaultState.totalStrategyTokenGlobal);
21:         if (context.vaultState.totalStrategyTokenGlobal > 0) {
22:             bptClaim = (strategyTokenAmount * context.vaultState.totalBPTHeld) / context.vaultState.totalStrategyTokenGlobal;
23:         }
24:     }

In Line 441 of the Boosted3TokenPoolUtils._redeem function below, if bptClaim is zero, it will return zero and exit the function immediately.

If a small number of strategyTokens is passed into the _redeem function and the bptClaim ends up as zero, the caller of the _redeem function will assume that all the strategyTokens have been redeemed.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/Boosted3TokenPoolUtils.sol#L432

File: Boosted3TokenPoolUtils.sol
432:     function _redeem(
433:         ThreeTokenPoolContext memory poolContext,
434:         StrategyContext memory strategyContext,
435:         AuraStakingContext memory stakingContext,
436:         uint256 strategyTokens,
437:         uint256 minPrimary
438:     ) internal returns (uint256 finalPrimaryBalance) {
439:         uint256 bptClaim = strategyContext._convertStrategyTokensToBPTClaim(strategyTokens);
440: 
441:         if (bptClaim == 0) return 0;
442: 
443:         finalPrimaryBalance = _unstakeAndExitPool({
444:             stakingContext: stakingContext,
445:             poolContext: poolContext,
446:             bptClaim: bptClaim,
447:             minPrimary: minPrimary
448:         });
449: 
450:         strategyContext.vaultState.totalBPTHeld -= bptClaim;
451:         strategyContext.vaultState.totalStrategyTokenGlobal -= strategyTokens.toUint80();
452:         strategyContext.vaultState.setStrategyVaultState(); 
453:     }

The following function shows an example of the caller of the _redeem function at Line 171 below accepting the zero value as it does not revert when the zero value is returned by the _redeem function. Thus, it will consider the small number of strategyTokens to be redeemed. Note that the _redeemFromNotional function calls the _redeem function under the hood.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/BaseStrategyVault.sol#L163

File: BaseStrategyVault.sol
163:     function redeemFromNotional(
164:         address account,
165:         address receiver,
166:         uint256 strategyTokens,
167:         uint256 maturity,
168:         uint256 underlyingToRepayDebt,
169:         bytes calldata data
170:     ) external onlyNotional returns (uint256 transferToReceiver) {
171:         uint256 borrowedCurrencyAmount = _redeemFromNotional(account, strategyTokens, maturity, data);
172: 
173:         uint256 transferToNotional;
174:         if (account == address(this) || borrowedCurrencyAmount <= underlyingToRepayDebt) {
175:             // It may be the case that insufficient tokens were redeemed to repay the debt. If this
176:             // happens the Notional will attempt to recover the shortfall from the account directly.
177:             // This can happen if an account wants to reduce their leverage by paying off debt but
178:             // does not want to sell strategy tokens to do so.
179:             // The other situation would be that the vault is calling redemption to deleverage or
180:             // settle. In that case all tokens go back to Notional.
181:             transferToNotional = borrowedCurrencyAmount;
182:         } else {
183:             transferToNotional = underlyingToRepayDebt;
184:             unchecked { transferToReceiver = borrowedCurrencyAmount - underlyingToRepayDebt; }
185:         }
186: 
187:         if (_UNDERLYING_IS_ETH) {
188:             if (transferToReceiver > 0) payable(receiver).transfer(transferToReceiver);
189:             if (transferToNotional > 0) payable(address(NOTIONAL)).transfer(transferToNotional);
190:         } else {
191:             if (transferToReceiver > 0) _UNDERLYING_TOKEN.checkTransfer(receiver, transferToReceiver);
192:             if (transferToNotional > 0) _UNDERLYING_TOKEN.checkTransfer(address(NOTIONAL), transferToNotional);
193:         }
194:     }

Subsequently, on Notional side, it will deduct the redeemed strategy tokens from itsvaultState.totalStrategyTokens state (Refer to Line 177 below)

https://github.com/notional-finance/contracts-v2/blob/63eb0b46ec37e5fc5447bdde3d951dd90f245741/contracts/external/actions/VaultAction.sol#L157

File: VaultAction.sol
156:     /// @notice Redeems strategy tokens to cash
157:     function _redeemStrategyTokensToCashInternal(
158:         VaultConfig memory vaultConfig,
159:         uint256 maturity,
160:         uint256 strategyTokensToRedeem,
161:         bytes calldata vaultData
162:     ) private nonReentrant returns (int256 assetCashRequiredToSettle, int256 underlyingCashRequiredToSettle) {
163:         // If the vault allows further re-entrancy then set the status back to the default
164:         if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
165:             reentrancyStatus = _NOT_ENTERED;
166:         }
167: 
168:         VaultState memory vaultState = VaultStateLib.getVaultState(vaultConfig.vault, maturity);
169:         (int256 assetCashReceived, uint256 underlyingToReceiver) = vaultConfig.redeemWithoutDebtRepayment(
170:             vaultConfig.vault, strategyTokensToRedeem, maturity, vaultData
171:         );
172:         require(assetCashReceived > 0);
173:         // Safety check to ensure that the vault does not somehow receive tokens in this scenario
174:         require(underlyingToReceiver == 0);
175: 
176:         vaultState.totalAssetCash = vaultState.totalAssetCash.add(uint256(assetCashReceived));
177:         vaultState.totalStrategyTokens = vaultState.totalStrategyTokens.sub(strategyTokensToRedeem);
178:         vaultState.setVaultState(vaultConfig.vault);
179: 
180:         emit VaultRedeemStrategyToken(vaultConfig.vault, maturity, assetCashReceived, strategyTokensToRedeem);
181:         return _getCashRequiredToSettle(vaultConfig, vaultState, maturity);
182:     }

However, the main issue is that when a small number of strategyTokens are redeemed and bptClaim is zero, the _redeem function will exit at Line 441 immediately. Thus, the redeemed strategy tokens are not deducted from the strategyContext.vaultState.totalStrategyTokenGlobal accounting variable on the Vault side.

Thus, strategyContext.vaultState.totalStrategyTokenGlobal on the Vault side will not be in sync with the vaultState.totalStrategyTokens on the Notional side.

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/Boosted3TokenPoolUtils.sol#L432

File: Boosted3TokenPoolUtils.sol
432:     function _redeem(
433:         ThreeTokenPoolContext memory poolContext,
434:         StrategyContext memory strategyContext,
435:         AuraStakingContext memory stakingContext,
436:         uint256 strategyTokens,
437:         uint256 minPrimary
438:     ) internal returns (uint256 finalPrimaryBalance) {
439:         uint256 bptClaim = strategyContext._convertStrategyTokensToBPTClaim(strategyTokens);
440: 
441:         if (bptClaim == 0) return 0;
442: 
443:         finalPrimaryBalance = _unstakeAndExitPool({
444:             stakingContext: stakingContext,
445:             poolContext: poolContext,
446:             bptClaim: bptClaim,
447:             minPrimary: minPrimary
448:         });
449: 
450:         strategyContext.vaultState.totalBPTHeld -= bptClaim;
451:         strategyContext.vaultState.totalStrategyTokenGlobal -= strategyTokens.toUint80();
452:         strategyContext.vaultState.setStrategyVaultState(); 
453:     }

Impact

The strategyContext.vaultState.totalStrategyTokenGlobal variable that tracks the number of strategy tokens held in the vault will not be in sync and will cause accounting issues within the vault. This means that the actual total strategy tokens in circulation and the strategyContext.vaultState.totalStrategyTokenGlobal will be different. The longer the issue is left unfixed, the larger the differences between them.

The strategyContext.vaultState.totalStrategyTokenGlobal will be larger than expected because it does not deduct the number of strategy tokens when it should be under certain conditions.

One example of the impact is as follows: The affected variable is used within the _convertStrategyTokensToBPTClaim and _convertBPTClaimToStrategyTokens, _getBPTHeldInMaturity functions. These functions are used within the deposit and redeem functions of the vault. Therefore, the number of strategy tokens or assets the users receive will not be accurate and might be less or more than expected.

Code Snippet

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/strategy/StrategyUtils.sol#L18

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/Boosted3TokenPoolUtils.sol#L432

https://github.com/sherlock-audit/2022-12-notional/blob/main/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L209

Tool used

Manual Review

Recommendation

The number of strategy tokens redeemed needs to be deducted from the vault's totalStrategyTokenGlobal regardless of the bptClaim value. Otherwise, the vault's totalStrategyTokenGlobal will not be in sync.

When bptClaim is zero, it does not always mean that no strategy token has been redeemed. Based on the current vault implementation, the bptClaim might be zero because the number of strategy tokens to be redeemed is too small and thus it causes Solidity to round down to zero.

function _redeem(
    ThreeTokenPoolContext memory poolContext,
    StrategyContext memory strategyContext,
    AuraStakingContext memory stakingContext,
    uint256 strategyTokens,
    uint256 minPrimary
) internal returns (uint256 finalPrimaryBalance) {
    uint256 bptClaim = strategyContext._convertStrategyTokensToBPTClaim(strategyTokens);
+	strategyContext.vaultState.totalStrategyTokenGlobal -= strategyTokens.toUint80();
+	strategyContext.vaultState.setStrategyVaultState();
+
    if (bptClaim == 0) return 0;

    finalPrimaryBalance = _unstakeAndExitPool({
        stakingContext: stakingContext,
        poolContext: poolContext,
        bptClaim: bptClaim,
        minPrimary: minPrimary
    });

    strategyContext.vaultState.totalBPTHeld -= bptClaim;
-   strategyContext.vaultState.totalStrategyTokenGlobal -= strategyTokens.toUint80();
    strategyContext.vaultState.setStrategyVaultState(); 
}

[weitianjie2000]

valid, will fix