sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

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

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

high

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

Summary

During the emergency 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.

Vulnerability Detail

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

Background

How does the emergency vault settlement process work?

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

  1. Anyone can call the settleVaultEmergency function to trigger the emergency vault settlement as it is permissionless
  2. The _getEmergencySettlementParams function will calculate the excess BPT tokens within the vault to be settled/sold
  3. The amount of excess BPT tokens will be converted to an equivalence amount of strategy tokens to be settled
  4. The strategy tokens will be settled by withdrawing staked BPT tokens from Aura Finance back to the vault for redemption.
  5. The vault will then redeem the BTP tokens from Balancer to redeem its underlying assets (WETH and stETH)
  6. 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.
  7. The WETH within the vault will be sent to Notional, and Notional will mint the asset tokens (cEther) for the vault in return.
  8. 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 emergency vault settlement process, the code logic did not validate that the total amount of assets that it received from selling off/settling the excess BPT 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 does not affect the other processes (e.g. enter vault, exit vault) because their code logic will verify the amount of output/purchased tokens received via the amountBought return value from the trade or perform some sort of pre and post balance validation.

However, this issue affects the emergency vault settlement process. The main reason is that the amountBought return value is ignored entirely. None of the code logic attempts to verify that the amountBought return value is valid or perform any pre and post balance validation.

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_emergency_single_maturity(StratStableETHstETH):
    (env, vault) = StratStableETHstETH
    primaryBorrowAmount = 5e8
    depositAmount = 10e18
    maturity = enterMaturity(env, vault, 1, 0, depositAmount, primaryBorrowAmount, accounts[0])
    settings = vault.getStrategyContext()["baseStrategy"]["vaultSettings"]
    vault.setStrategyVaultSettings(get_updated_vault_settings(settings, maxBalancerPoolShare=0), {"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))
    )
    vault.settleVaultEmergency(maturity, redeemParams, {"from": accounts[1]})
    vaultState = env.notional.getVaultState(vault.address, maturity)
    assert vaultState["totalStrategyTokens"] == 0

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/settling the excess BPT was appropriate. Otherwise, the transaction should have already reverted.

❯ brownie test tests/balancer/settlement/test_settlement_stable_eth_steth.py --network mainnet-fork
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 6.70s ================================================================================================
Terminating local RPC client...

Attack Paths

Following are the detailed attack paths for exploiting this vulnerability:

Attack Path 1 - Stealing Bought Tokens:

  1. A malicious user called Bob calls the permissionless settleVaultEmergency function to trigger the emergency vault settlement process and specify that the trade is to be executed on 0x.
  2. The vault determines that it needs to offload 100 stETH tokens to bring the vault back to a healthy ratio. Assume that the exchange rate between WETH and stETH is 1.
  3. When calling the settleVaultEmergency function, Bob creates a 0x trade order to sell the secondary tokens (stWETH) for primary tokens (WETH).
  4. 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.
  5. 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.
  6. However, due to the lack of validation, the vault happily accepts the fact that it receives nothing and did not revert.
  7. The emergency vault settlement process is completed, and Bob received 100 WETH.

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

  1. A malicious user called Bob calls the permissionless settleVaultEmergency function to trigger the emergency vault settlement process.
  2. 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/trading/TradingModule.sol#L109

Tool used

Manual Review

Recommendation

Ensure that the emergency vault settlement process verifies the amount of output/purchased tokens received via the amountBought 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

jeffywu commented 1 year ago

This is a duplicate of #75