Normal Settlement Process Do Not Verify That The Vault Receives The Appropriate Amount Of Primary Tokens After Sale Of Secondary Tokens

Summary

During the normal vault settlement, the vault did not verify that it received the appropriate amount of primary tokens after the sale of secondary tokens. Therefore, a malicious user can exploit this vulnerability to steal all the bought tokens during the trade and possibly drain all the BTP in the vault.

Vulnerability Details

Note: This issue affects the 0x adaptor and the normal vault settlement process. It does not affect the other DEX adaptors and processes.

Background

How does the normal vault settlement process work?

The following describes how the normal vault settlement process work, which is useful for understanding this issue.

Anyone can call the settleVaultNormal function to trigger the normal vault settlement during the settlement period as it is permissionless
The user will indicate the number of strategy tokens to be redeemed to repay the debt. Within the SettlementUtils._executeSettlement function, it will ensure that only a sufficient amount of strategy tokens are being redeemed for BPT to repay the debt. No more or less. The reason is that the remaining BPT will continue to be staked at Aura Reward Pool to accrue BPT and rewards.
The strategy tokens will be settled by withdrawing staked BPT tokens from Aura Finance back to the vault for redemption.
The vault will then redeem the BTP tokens from Balancer to redeem its underlying assets (WETH and stETH)
The primary and secondary assets of the vault are WETH and stETH respectively. The secondary asset (stETH) will be traded for the primary asset (WETH) in one of the supported DEXes. In the end, only the primary assets (WETH) should remain within the vault.
The WETH will mint for asset tokens (cEther), and be used to repay the vault debt.
After completing the emergency vault settlement process, the vault will gain asset tokens (cEther) after settling/selling its excess BPT tokens.

Issue Description

It was observed that during the normal vault settlement process, the code logic did not validate that the total amount of assets that it received from selling off the secondary tokens (stETH) was appropriate. The vault happily accepts any amount of primary tokens it receives from the sale of the secondary tokens during the trade. Even if the vault does not receive any tokens at all after the trade, it does not revert.

This issue affects the normal vault settlement process. The main reason is that the finalPrimaryBalance return value from the below TwoTokenPoolUtils._redeem is ignored entirely.

None of the code logic attempts to verify that the finalPrimaryBalance return value is valid or perform any pre and post balance validation.

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

File: TwoTokenPoolUtils.sol
235:     function _redeem(
236:         TwoTokenPoolContext memory poolContext,
237:         StrategyContext memory strategyContext,
238:         AuraStakingContext memory stakingContext,
239:         address account,
240:         uint256 strategyTokens,
241:         uint256 maturity,
242:         RedeemParams memory params
243:     ) internal returns (uint256 finalPrimaryBalance) {
244:         uint256 bptClaim = strategyContext._convertStrategyTokensToBPTClaim(strategyTokens);
245: 
246:         if (bptClaim == 0) return 0;
247: 
248:         // Underlying token balances from exiting the pool
249:         (uint256 primaryBalance, uint256 secondaryBalance)
250:             = _unstakeAndExitPool(
251:                 poolContext, stakingContext, bptClaim, params.minPrimary, params.minSecondary
252:             );
253: 
254:         finalPrimaryBalance = primaryBalance;
255:         if (secondaryBalance > 0) {
256:             uint256 primaryPurchased = _sellSecondaryBalance(
257:                 poolContext, strategyContext, params, secondaryBalance
258:             );
259: 
260:             finalPrimaryBalance += primaryPurchased;
261:         }
262: 
263:         // Update global strategy token balance
264:         strategyContext.vaultState.totalStrategyTokenGlobal -= strategyTokens.toUint80();
265:         strategyContext.vaultState.setStrategyVaultState(); 
266:     }

Proof-of-Concept

A straightforward way to simulate the vault receiving zero primary tokens (WETH) from the sale of the secondary tokens (stETH) is to temporarily change the source code to update the recipient of the purchase tokens to a burn address, and recompile the codes. In this case, the vault will not receive any primary token (WETH) in return after selling off the secondary tokens (stETH).

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L109

File: TradingModule.sol
109:     function executeTradeWithDynamicSlippage(
110:         uint16 dexId,
111:         Trade memory trade,
112:         uint32 dynamicSlippageLimit
113:     ) external override returns (uint256 amountSold, uint256 amountBought) {
114:         // This method calls back into the implementation via the proxy so that it has proper
115:         // access to storage.
116:         trade.limit = PROXY.getLimitAmount(
117:             trade.tradeType,
118:             trade.sellToken,
119:             trade.buyToken,
120:             trade.amount,
121:             dynamicSlippageLimit
122:         );
123: 
124:         (
125:             address spender,
126:             address target,
127:             uint256 msgValue,
128:             bytes memory executionData
-129:         ) = PROXY.getExecutionData(dexId, address(this), trade);
+129:         ) = PROXY.getExecutionData(dexId, address(0), trade);
130: 
131:         return
132:             TradingUtils._executeInternal(
133:                 trade,
134:                 dexId,
135:                 spender,
136:                 target,
137:                 msgValue,
138:                 executionData
139:             );
140:     }

The following PoC is modified from the test cases in test_settlement_stable_eth_steth.py.

import math
import time
import pytest
import brownie
from brownie import accounts
from brownie.network.state import Chain
from tests.fixtures import *
from tests.balancer.helpers import enterMaturity
from scripts.common import (
    get_updated_vault_settings, 
    get_dynamic_trade_params,
    get_univ3_single_data,
    get_redeem_params,
    DEX_ID,
    TRADE_TYPE
)

chain = Chain()

def test_normal_single_maturity(StratStableETHstETH):
    (env, vault) = StratStableETHstETH
    primaryBorrowAmount = 5e8
    depositAmount = 10e18
    maturity = enterMaturity(env, vault, 1, 0, depositAmount, primaryBorrowAmount, accounts[0])
    chain.sleep(maturity - 3600 * 24 * 6 - chain.time())
    chain.mine()
    # Disable oracle freshness check
    env.tradingModule.setMaxOracleFreshness(2 ** 32 - 1, {"from": env.notional.owner()})
    redeemParams = get_redeem_params(
        0, 0, get_dynamic_trade_params(DEX_ID["CURVE"], TRADE_TYPE["EXACT_IN_SINGLE"], 5e6, True, bytes(0))
    )
    vaultState = env.notional.getVaultState(vault.address, maturity)
    assert vaultState["totalAssetCash"] == 0
    assert vaultState["totalStrategyTokens"] == vaultState["totalVaultShares"]
    tokensToRedeem = math.floor(vaultState["totalStrategyTokens"] * 0.5)

    # Can't settle with bad slippage setting
    with brownie.reverts():
        vault.settleVaultNormal.call(maturity, tokensToRedeem, get_redeem_params(
            0, 0, get_dynamic_trade_params(DEX_ID["CURVE"], TRADE_TYPE["EXACT_IN_SINGLE"], 10e6, True, bytes(0))
        ), {"from": accounts[1]})

    # Can't redeem beyond maxUnderlyingSurplus
    settings = vault.getStrategyContext()["baseStrategy"]["vaultSettings"]
    oldMaxUnderlyingSurplus = settings["maxUnderlyingSurplus"]
    vault.setStrategyVaultSettings(
        get_updated_vault_settings(settings, maxUnderlyingSurplus=0), {"from": env.notional.owner()}
    )
    with brownie.reverts():
        vault.settleVaultNormal.call(maturity, tokensToRedeem, redeemParams, {"from": accounts[1]})
    vault.setStrategyVaultSettings(
        get_updated_vault_settings(settings, maxUnderlyingSurplus=oldMaxUnderlyingSurplus), {"from": env.notional.owner()}
    )

    # Test settlement
    vault.settleVaultNormal(maturity, tokensToRedeem, redeemParams, {"from": accounts[1]})
    vaultState = env.notional.getVaultState(vault.address, maturity)
    # assert pytest.approx(vaultState["totalAssetCash"], rel=1e-2) == 37256494853
    assert vaultState["totalAssetCash"] > 0 # Added by xiaoming90 
    assert vaultState["totalStrategyTokens"] == vaultState["totalVaultShares"] - tokensToRedeem

    # Can't deposit during settlement (totalAssetCash > 0)
    with brownie.reverts():
        enterMaturity(env, vault, 1, 0, depositAmount, primaryBorrowAmount, accounts[1], True)

The following test output shows that the function did not revert although the vault did not receive any primary token (WETH) in return after selling off the secondary tokens (stETH). Therefore, this is sufficient to prove that there is no validation within the code to ensure that the total amount of assets that it received from selling off the secondary tokens (stETH) was appropriate. Otherwise, the transaction should have already reverted.

❯ brownie test tests/balancer/settlement/test_settlement_stable_eth_steth.py 
Brownie v1.18.1 - Python development framework for Ethereum

===================================================== test session starts ======================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
plugins: eth-brownie-1.18.1, hypothesis-6.27.3, forked-1.4.0, xdist-1.34.0, web3-5.27.0
collected 1 item                                                                                                               

Launching 'npx hardhat node --port 8545 --fork https://eth-mainnet.g.alchemy.com/v2/DRXs2eoKsWyJxZuPZkSoN5SkS2Og-hEN --fork-block-number 15447569'...

tests/balancer/settlement/test_settlement_stable_eth_steth.py .                                                          [100%]

====================================================== 1 passed in 7.21s =======================================================
Terminating local RPC client...

Attack Paths

Following are the detailed attack paths for exploiting this vulnerability:

Attack Path 1 - Stealing Bought Tokens:

A malicious user called Bob calls the permissionless settleVaultNormal function to trigger the normal vault settlement process and specify that the trade is to be executed on 0x.
Assume that the vault needs to claim 100 BPT to repay its debt. Assume that after claiming the BPT, the vault receives 100 WETH and 100 stETH.
When calling the settleVaultNormal function, Bob creates a 0x trade order to sell the secondary tokens (stWETH) for primary tokens (WETH).
Within the trade order, Bob configures the recipient parameter to his address. This will result in all bought tokens being redirected to Bob's wallet. This is possible due to another vulnerability found in the system. Refer to my other issue "Bought Token Can Be Sent To Attacker's Wallet Using 0x" for more detail.
When the trade is executed, the vault will sell 100 stETH tokens but get nothing in return. This is because all the bought tokens (100 WETH) have been forwarded to Bob's wallet.
However, due to the lack of validation, the vault happily accepts the fact that it receives nothing and did not revert.
The normal vault settlement process proceeds as per normal from here, and Bob received 100 WETH. However, since not all the debts are repaid, the vault is not settled yet.
Since the vault is not settled, Bob can possibly repeat the same steps above a few times in an attempt to drain as much BPT from the vault as possible.

Attack Path 2 - Performing Denial-of-Service On The Vault

A malicious user called Bob calls the permissionless settleVaultNormal function to trigger the emergency vault settlement process.
Bob forces the trade to be executed with a really bad slippage, causing a significant amount of assets to be lost. The vault ended up receiving much fewer primary tokens in return, thus causing a loss of assets for the vaults and their users. This is possible due to other vulnerabilities found in the systems. Refer to my other issues "Existing Slippage Control Can Be Bypassed During Vault Settlement" and "Slippage Control trade.limit Is Ignored By 0x Adaptor" for more details.

Impact

Since the vault happily accepts any amount of primary tokens it receives from the sale of the secondary tokens during the trade, a malicious user can cause the trade to suffer huge slippage or steal all the bought tokens. This will lead to serious loss of assets for the vaults and their users.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L235 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L109

Tool used

Manual Review

Recommendation

Ensure that the normal vault settlement process verifies the amount of output/purchased tokens received via the finalPrimaryBalance return value from the trade or perform some sort of pre and post balance validation.

These measures have already been implemented in other processes (e.g. enter vault, exit vault), but it was not consistently applied to the emergency vault settlement process.

Duplicate of #75

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Normal Settlement Process Do Not Verify That The Vault Receives The Appropriate Amount Of Primary Tokens After Sale Of Secondary Tokens #71