xiaoming90 - Existing Slippage Control Can Be Bypassed During Vault Settlement

[sherlock-admin]

xiaoming90

high

Existing Slippage Control Can Be Bypassed During Vault Settlement

Summary

The existing slippage control can be bypassed/disabled during vault settlement, thus allowing the trade to be executed without consideration of its slippage.

Vulnerability Detail

Note 1: This issue affects MetaStable2 and Boosted3 balancer leverage vaults

Note 2: This issue affects the following three (3) processes. However, the root cause and the remediation action are the same for all. Therefore, only the PoC for the "Emergency vault settlement" process will be documented in this report, and the other two processes will be omitted for brevity. Refer to "Appendix I - Normal and Post Maturity Vault Settlement" for more details.

• Emergency vault settlement
• Normal vault settlement
• Post-Maturity vault settlement.

Note 3: The issue affects all the supported DEXs (Curve, Balancer V2, Uniswap V2, Uniswap V3 and 0x) within Notional

The emergencySettlementSlippageLimitPercent of the vault is set to 10% as per the environment file provided by Notional.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/scripts/BalancerEnvironment.py#L43

File: BalancerEnvironment.py
43:             "postMaturitySettlementSlippageLimitPercent": 10e6, # 10%
44:             "emergencySettlementSlippageLimitPercent": 10e6, # 10%

When a user calls the settleVaultEmergency function, the vault will validate that the slippage (DynamicTradeParams.oracleSlippagePercent) defined by the caller is within the acceptable slippage range by calling SettlementUtils._decodeParamsAndValidate function.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/external/Boosted3TokenAuraHelper.sol#L53

File: MetaStable2TokenAuraHelper.sol
52:     function settleVaultEmergency(
53:         MetaStable2TokenAuraStrategyContext calldata context, 
54:         uint256 maturity, 
55:         bytes calldata data
56:     ) external {
57:         RedeemParams memory params = SettlementUtils._decodeParamsAndValidate(
58:             context.baseStrategy.vaultSettings.emergencySettlementSlippageLimitPercent,
59:             data
60:         );
61: 
62:         uint256 bptToSettle = context.baseStrategy._getEmergencySettlementParams({
63:             poolContext: context.poolContext.basePool, 
64:             maturity: maturity, 
65:             totalBPTSupply: IERC20(context.poolContext.basePool.pool).totalSupply()
66:         });

The SettlementUtils._decodeParamsAndValidate function will validate that the slippage (DynamicTradeParams.oracleSlippagePercent) passed in by the caller does not exceed the designated threshold (10%). In Line 41-42, the transaction will revert if the DynamicTradeParams.oracleSlippagePercent exceeds the slippageLimitPercent. Note that slippageLimitPercent is equal to emergencySettlementSlippageLimitPercent which is 10%.

There is an edge case with the condition at Line 41. Consider the following cases:

• If callbackData.oracleSlippagePercent = 9% and slippageLimitPercent = 10%, the condition will evaluate as False and transaction will not revert
• If callbackData.oracleSlippagePercent = 11% and slippageLimitPercent = 10%, the condition will evaluate as True and transaction will revert because it exceeds the designated threshold.
• If callbackData.oracleSlippagePercent = 0% and slippageLimitPercent = 10%, the condition will evaluate as False and transaction will not revert

The problem is that when callbackData.oracleSlippagePercent is 0%, this effectively means that there is no slippage limit. This essentially exceeded the designated threshold (10%), and the transaction should revert instead, but it did not.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/settlement/SettlementUtils.sol#L32

File: SettlementUtils.sol
27:     /// @notice Validates that the slippage passed in by the caller
28:     /// does not exceed the designated threshold.
29:     /// @param slippageLimitPercent configured limit on the slippage from the oracle price allowed
30:     /// @param data trade parameters passed into settlement
31:     /// @return params abi decoded redemption parameters
32:     function _decodeParamsAndValidate(
33:         uint32 slippageLimitPercent,
34:         bytes memory data
35:     ) internal view returns (RedeemParams memory params) {
36:         params = abi.decode(data, (RedeemParams));
37:         DynamicTradeParams memory callbackData = abi.decode(
38:             params.secondaryTradeParams, (DynamicTradeParams)
39:         );
40: 
41:         if (callbackData.oracleSlippagePercent > slippageLimitPercent) {
42:             revert Errors.SlippageTooHigh(callbackData.oracleSlippagePercent, slippageLimitPercent);
43:         }
44:     }

Within executeTradeWithDynamicSlippage function, it will calculate the trade.limit by calling the PROXY.getLimitAmount. The trade.limit is the maximum amount of sellToken that can be sold OR the minimum amount of buyToken the contract is expected to receive from the DEX depending on whether you are performing a sell or buy.

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:         );

Within the TradingUtils._getLimitAmount function, when the slippageLimit is set to 0,

• If it is a sell trade, the limitAmount will be set to type(uint256).max. See Line 187
• If it is a buy trade, the limitAmount will be set to 0. See Line 207

These effectively remove the slippage limit. Therefore, a malicious user can specify the callbackData.oracleSlippagePercent to be 0% to bypass the slippage validation check.

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

File: TradingUtils.sol
162:     function _getLimitAmount(
163:         TradeType tradeType,
164:         address sellToken,
165:         address buyToken,
166:         uint256 amount,
167:         uint32 slippageLimit,
168:         uint256 oraclePrice,
169:         uint256 oracleDecimals
170:     ) internal view returns (uint256 limitAmount) {
171:         uint256 sellTokenDecimals = 10 **
172:             (
173:                 sellToken == Deployments.ETH_ADDRESS
174:                     ? 18
175:                     : IERC20(sellToken).decimals()
176:             );
177:         uint256 buyTokenDecimals = 10 **
178:             (
179:                 buyToken == Deployments.ETH_ADDRESS
180:                     ? 18
181:                     : IERC20(buyToken).decimals()
182:             );
183: 
184:         if (tradeType == TradeType.EXACT_OUT_SINGLE || tradeType == TradeType.EXACT_OUT_BATCH) {
185:             // 0 means no slippage limit
186:             if (slippageLimit == 0) {
187:                 return type(uint256).max;
188:             }
189:             // For exact out trades, we need to invert the oracle price (1 / oraclePrice)
190:             // We increase the precision before we divide because oraclePrice is in
191:             // oracle decimals
192:             oraclePrice = (oracleDecimals * oracleDecimals) / oraclePrice;
193:             // For exact out trades, limitAmount is the max amount of sellToken the DEX can
194:             // pull from the contract
195:             limitAmount =
196:                 ((oraclePrice + 
197:                     ((oraclePrice * uint256(slippageLimit)) /
198:                         Constants.SLIPPAGE_LIMIT_PRECISION)) * amount) / 
199:                 oracleDecimals;
200: 
201:             // limitAmount is in buyToken precision after the previous calculation,
202:             // convert it to sellToken precision
203:             limitAmount = (limitAmount * sellTokenDecimals) / buyTokenDecimals;
204:         } else {
205:             // 0 means no slippage limit
206:             if (slippageLimit == 0) {
207:                 return 0;
208:             }
209:             // For exact in trades, limitAmount is the min amount of buyToken the contract
210:             // expects from the DEX
211:             limitAmount =
212:                 ((oraclePrice -
213:                     ((oraclePrice * uint256(slippageLimit)) /
214:                         Constants.SLIPPAGE_LIMIT_PRECISION)) * amount) /
215:                 oracleDecimals;
216: 
217:             // limitAmount is in sellToken precision after the previous calculation,
218:             // convert it to buyToken precision
219:             limitAmount = (limitAmount * buyTokenDecimals) / sellTokenDecimals;
220:         }
221:     }

Proof-of-Concept

The following test case shows that when the slippage is set to 11% (11e6), the transaction will be reverted and fails the test. This is working as intended because the slippage (11%) exceeded the threshold (emergencySettlementSlippageLimitPercent = 10%).

def test_emergency_single_maturity_success(StratBoostedPoolUSDCPrimary):
    (env, vault) = StratBoostedPoolUSDCPrimary
    primaryBorrowAmount = 5000e8
    depositAmount = 10000e6
    env.tokens["USDC"].approve(env.notional, 2 ** 256 - 1, {"from": env.whales["USDC"]})
    maturity = enterMaturity(env, vault, 2, 0, depositAmount, primaryBorrowAmount, env.whales["USDC"])
    strategyContext = vault.getStrategyContext()
    settings = dict(strategyContext["baseStrategy"]["vaultSettings"].dict())
    settings["maxBalancerPoolShare"] = 0
    vault.setStrategyVaultSettings(
        list(settings.values()), 
        {"from": env.notional.owner()}
    )
    # minPrimary is calculated internally for boosted pools 
    redeemParams = get_redeem_params(0, 0, 
        get_dynamic_trade_params(
            DEX_ID["UNISWAP_V3"], TRADE_TYPE["EXACT_IN_SINGLE"], 11e6, True, get_univ3_single_data(3000)
        )
    )
    vault.settleVaultEmergency(maturity, redeemParams, {"from": env.notional.owner()})
    vaultState = env.notional.getVaultState(vault.address, maturity)
    assert vaultState["totalStrategyTokens"] == 0

❯ brownie test tests/balancer/settlement/test_settlement_boosted_usdc.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                                                                                                                                                                                                  
Attached to local RPC client listening at '127.0.0.1:8545'...

tests/balancer/settlement/test_settlement_boosted_usdc.py F                                                                                                                                                 [100%]

==================================================================================================== FAILURES =====================================================================================================

The following test case shows that when the slippage is set to 0, the transaction does not revert and passes the test. This is not working as intended because having no slippage (0) technically exceeded the threshold (emergencySettlementSlippageLimitPercent = 10%).

def test_emergency_single_maturity_success(StratBoostedPoolUSDCPrimary):
    (env, vault) = StratBoostedPoolUSDCPrimary
    primaryBorrowAmount = 5000e8
    depositAmount = 10000e6
    env.tokens["USDC"].approve(env.notional, 2 ** 256 - 1, {"from": env.whales["USDC"]})
    maturity = enterMaturity(env, vault, 2, 0, depositAmount, primaryBorrowAmount, env.whales["USDC"])
    strategyContext = vault.getStrategyContext()
    settings = dict(strategyContext["baseStrategy"]["vaultSettings"].dict())
    settings["maxBalancerPoolShare"] = 0
    vault.setStrategyVaultSettings(
        list(settings.values()), 
        {"from": env.notional.owner()}
    )
    # minPrimary is calculated internally for boosted pools 
    redeemParams = get_redeem_params(0, 0, 
        get_dynamic_trade_params(
            DEX_ID["UNISWAP_V3"], TRADE_TYPE["EXACT_IN_SINGLE"], 0, True, get_univ3_single_data(3000)
        )
    )
    vault.settleVaultEmergency(maturity, redeemParams, {"from": env.notional.owner()})
    vaultState = env.notional.getVaultState(vault.address, maturity)
    assert vaultState["totalStrategyTokens"] == 0

❯ brownie test tests/balancer/settlement/test_settlement_boosted_usdc.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                                                                                                                                                                                                  
Attached to local RPC client listening at '127.0.0.1:8545'...

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

========================================================================================== 1 passed, 1 warning in 4.31s ===========================================================================================

Impact

Malicious users can trigger the permissionless settleVaultEmergency function and cause the trade to suffer huge slippage. This results in loss of assets for the vaults and their users.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/scripts/BalancerEnvironment.py#L43 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/external/Boosted3TokenAuraHelper.sol#L53 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/settlement/SettlementUtils.sol#L32 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L109 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol#L162

Tool used

Manual Review

Recommendation

Update the SettlementUtils._decodeParamsAndValidate function to revert if the slippage is set to zero.

File: SettlementUtils.sol
27:     /// @notice Validates that the slippage passed in by the caller
28:     /// does not exceed the designated threshold.
29:     /// @param slippageLimitPercent configured limit on the slippage from the oracle price allowed
30:     /// @param data trade parameters passed into settlement
31:     /// @return params abi decoded redemption parameters
32:     function _decodeParamsAndValidate(
33:         uint32 slippageLimitPercent,
34:         bytes memory data
35:     ) internal view returns (RedeemParams memory params) {
36:         params = abi.decode(data, (RedeemParams));
37:         DynamicTradeParams memory callbackData = abi.decode(
38:             params.secondaryTradeParams, (DynamicTradeParams)
39:         );
40: 
-41:         if (callbackData.oracleSlippagePercent > slippageLimitPercent) {
+41:         if (callbackData.oracleSlippagePercent == 0 || callbackData.oracleSlippagePercent > slippageLimitPercent) {
42:             revert Errors.SlippageTooHigh(callbackData.oracleSlippagePercent, slippageLimitPercent);
43:         }
44:     }

Appendix I - Normal and Post Maturity Vault Settlement

The settlementSlippageLimitPercent and postMaturitySettlementSlippageLimitPercent of the vault are set to 5% and 10% respectively per the environment file provided by Notional.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/scripts/BalancerEnvironment.py#L42

File: BalancerEnvironment.py
42:             "settlementSlippageLimitPercent": 5e6, # 5%
43:             "postMaturitySettlementSlippageLimitPercent": 10e6, # 10%

When a user calls the settleVaultNormal or settleVaultPostMaturity function, the vault will validate that the slippage (DynamicTradeParams.oracleSlippagePercent) defined by the caller is within the acceptable slippage range by calling SettlementUtils._decodeParamsAndValidate function.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/MetaStable2TokenAuraVault.sol#L105

File: MetaStable2TokenAuraVault.sol
105:     function settleVaultNormal(
106:         uint256 maturity,
107:         uint256 strategyTokensToRedeem,
108:         bytes calldata data
109:     ) external {
110:         if (maturity <= block.timestamp) {
111:             revert Errors.PostMaturitySettlement();
112:         }
113:         if (block.timestamp < maturity - SETTLEMENT_PERIOD_IN_SECONDS) {
114:             revert Errors.NotInSettlementWindow();
115:         }
116:         MetaStable2TokenAuraStrategyContext memory context = _strategyContext();
117:         SettlementUtils._validateCoolDown(
118:             context.baseStrategy.vaultState.lastSettlementTimestamp,
119:             context.baseStrategy.vaultSettings.settlementCoolDownInMinutes
120:         );
121:         RedeemParams memory params = SettlementUtils._decodeParamsAndValidate(
122:             context.baseStrategy.vaultSettings.settlementSlippageLimitPercent,
123:             data
124:         );
125:         MetaStable2TokenAuraHelper.settleVault(
126:             context, maturity, strategyTokensToRedeem, params
127:         );
128:         context.baseStrategy.vaultState.lastSettlementTimestamp = uint32(block.timestamp);
129:         context.baseStrategy.vaultState.setStrategyVaultState();
130:     }
131: 
132:     function settleVaultPostMaturity(
133:         uint256 maturity,
134:         uint256 strategyTokensToRedeem,
135:         bytes calldata data
136:     ) external onlyNotionalOwner {
137:         if (block.timestamp < maturity) {
138:             revert Errors.HasNotMatured();
139:         }
140:         MetaStable2TokenAuraStrategyContext memory context = _strategyContext();
141:         SettlementUtils._validateCoolDown(
142:             context.baseStrategy.vaultState.lastPostMaturitySettlementTimestamp,
143:             context.baseStrategy.vaultSettings.postMaturitySettlementCoolDownInMinutes
144:         );
145:         RedeemParams memory params = SettlementUtils._decodeParamsAndValidate(
146:             context.baseStrategy.vaultSettings.postMaturitySettlementSlippageLimitPercent,
147:             data
148:         );
149:         MetaStable2TokenAuraHelper.settleVault(
150:             context, maturity, strategyTokensToRedeem, params
151:         );
152:         context.baseStrategy.vaultState.lastPostMaturitySettlementTimestamp = uint32(block.timestamp);    
153:         context.baseStrategy.vaultState.setStrategyVaultState();  
154:     }

Since the same vulnerable SettlementUtils._decodeParamsAndValidate function is being used here, the settleVaultNormal and settleVaultPostMaturity functions are affected by this issue too.

[jeffywu]

@weitianjie2000

[jeffywu]

Slippage control removal is now set to uint256.max which will resolve this issue.

[rayn731]

The original version is that it means no limit if the slippageLimit is 0, it could bypass the slippageLimitPercent. The fix uses type(uint256).max to define no slippage limit.