xiaoming90 - Existing Slippage Control Can Be Bypassed During Reinvest Rewards

[sherlock-admin]

xiaoming90

high

Existing Slippage Control Can Be Bypassed During Reinvest Rewards

Summary

The existing slippage control can be bypassed/disabled when reinvesting rewards, 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: The issue affects all the supported DEXs (Curve, Balancer V2, Uniswap V2, Uniswap V3 and 0x) within Notional

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

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

File: BalancerEnvironment.py
45:             "maxRewardTradeSlippageLimitPercent": 5e6,

When a user calls the reinvestReward function, the vault will validate that the slippage defined by the caller is within the acceptable slippage range within the TwoTokenAuraRewardUtils._executeRewardTrades function at Line 126. It will pass the strategyContext.vaultSettings.maxRewardTradeSlippageLimitPercent predefined by Notional to the TwoTokenAuraRewardUtils._executeRewardTrades function.

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

File: MetaStable2TokenAuraVault.sol
164:     function reinvestReward(ReinvestRewardParams calldata params) external {
165:         MetaStable2TokenAuraHelper.reinvestReward(_strategyContext(), params);
166:     }

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

File: MetaStable2TokenAuraHelper.sol
114:     function reinvestReward(
115:         MetaStable2TokenAuraStrategyContext calldata context,
116:         ReinvestRewardParams calldata params
117:     ) external {
118:         StrategyContext calldata strategyContext = context.baseStrategy;
119:         TwoTokenPoolContext calldata poolContext = context.poolContext; 
120:         StableOracleContext calldata oracleContext = context.oracleContext;
121: 
122:         (
123:             address rewardToken, 
124:             uint256 primaryAmount, 
125:             uint256 secondaryAmount
126:         ) = poolContext._executeRewardTrades(
127:             context.stakingContext,
128:             strategyContext.tradingModule,
129:             params.tradeData,
130:             strategyContext.vaultSettings.maxRewardTradeSlippageLimitPercent
131:         );

Within the TwoTokenAuraRewardUtils._executeRewardTrades function, it will validate that the user-defined slippage does not exceed the designated threshold (strategyContext.vaultSettings.maxRewardTradeSlippageLimitPercent = 5%) in Line 60-61. The transaction will revert if it exceeds the threshold. Note that the slippageLimit is equal to maxRewardTradeSlippageLimitPercent which is 5%.

There is an edge case with the condition at Line 60-61. Consider the following cases:

• If params.primaryTrade.tradeParams.oracleSlippagePercent = 4% and slippageLimitPercent = 5%, the condition will evaluate as False and transaction will not revert.
• If params.primaryTrade.tradeParams.oracleSlippagePercent = 6% and slippageLimitPercent = 5%, the condition will evaluate as True and transaction will revert because it exceeds the designated threshold.
• If params.primaryTrade.tradeParams.oracleSlippagePercent = 0% and slippageLimitPercent = 5%, 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 (strategyContext.vaultSettings.maxRewardTradeSlippageLimitPercent = 5%), 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/reward/TwoTokenAuraRewardUtils.sol#L48

File: TwoTokenAuraRewardUtils.sol
48:     function _executeRewardTrades(
49:         TwoTokenPoolContext calldata poolContext,
50:         AuraStakingContext calldata stakingContext,
51:         ITradingModule tradingModule,
52:         bytes calldata data,
53:         uint256 slippageLimit
54:     ) internal returns (address rewardToken, uint256 primaryAmount, uint256 secondaryAmount) {
55:         Balanced2TokenRewardTradeParams memory params = abi.decode(
56:             data,
57:             (Balanced2TokenRewardTradeParams)
58:         );
59: 
60:         require(params.primaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);
61:         require(params.secondaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);

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 6% (6e6), the transaction will be reverted and fails the test. This is working as intended because the slippage (6%) exceeded the threshold (maxRewardTradeSlippageLimitPercent = 5%).

def test_reinvest_rewards_success(StratBoostedPoolDAIPrimary):
    (env, vault) = StratBoostedPoolDAIPrimary
    rewardAmount = Wei(50e18)
    env.tokens["BAL"].transfer(vault.address, rewardAmount, {"from": env.whales["BAL"]})

    dynamicTradeParams = "(uint16,uint8,uint32,bool,bytes)"
    singleSidedRewardTradeParams = "(address,address,uint256,{})".format(dynamicTradeParams)
    assert vault.getStrategyContext()["baseStrategy"]["totalBPTHeld"] == 0
    vault.reinvestReward([eth_abi.encode_abi(
        [singleSidedRewardTradeParams],
        [[
            env.tokens["BAL"].address,
            env.tokens["DAI"].address,
            rewardAmount,
            [
                DEX_ID["UNISWAP_V3"],
                TRADE_TYPE["EXACT_IN_BATCH"],
                Wei(6e6),
                False,
                get_univ3_batch_data([
                    env.tokens["BAL"].address, 3000, env.tokens["WETH"].address, 500, env.tokens["DAI"].address
                ])
            ]
        ]]
    ), 0],
        {"from": accounts[1]}
    )
    assert pytest.approx(vault.getStrategyContext()["baseStrategy"]["totalBPTHeld"], rel=1e-2) == 333814721786840411586

❯ brownie test tests/balancer/rewards/test_rewards_boosted_dai.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/rewards/test_rewards_boosted_dai.py F                                         [100%]

============================================= FAILURES =============================================
__________________________________ test_reinvest_rewards_success ___________________________________

StratBoostedPoolDAIPrimary = (<scripts.BalancerEnvironment.BalancerEnvironment object at 0x7f099c3aabe0>, <Balancer Boosted Pool Strategy Contract '0xD7c3Dc1C36d19cF4e8cea4eA143a2f4458Dd1937'>)

    def test_reinvest_rewards_success(StratBoostedPoolDAIPrimary):
        (env, vault) = StratBoostedPoolDAIPrimary
        rewardAmount = Wei(50e18)
        env.tokens["BAL"].transfer(vault.address, rewardAmount, {"from": env.whales["BAL"]})

        dynamicTradeParams = "(uint16,uint8,uint32,bool,bytes)"
        singleSidedRewardTradeParams = "(address,address,uint256,{})".format(dynamicTradeParams)
        assert vault.getStrategyContext()["baseStrategy"]["totalBPTHeld"] == 0
>       vault.reinvestReward([eth_abi.encode_abi(
            [singleSidedRewardTradeParams],
            [[
                env.tokens["BAL"].address,
                env.tokens["DAI"].address,
                rewardAmount,
                [
                    DEX_ID["UNISWAP_V3"],
                    TRADE_TYPE["EXACT_IN_BATCH"],
                    Wei(6e6),
                    False,
                    get_univ3_batch_data([
                        env.tokens["BAL"].address, 3000, env.tokens["WETH"].address, 500, env.tokens["DAI"].address
                    ])
                ]
            ]]
        ), 0],
            {"from": accounts[1]}
        )
E       brownie.exceptions.VirtualMachineError: Transaction reverted without a reason string: Transaction reverted without a reason string
E       Trace step 6841, program counter 271:
E         File "/.brownie/packages/OpenZeppelin/openzeppelin-contracts@4.6.0/contracts/proxy/Proxy.sol", line 39, in Proxy._delegate:    
E           case 0 {
E               revert(0, returndatasize())
E           }

tests/balancer/rewards/test_rewards_boosted_dai.py:49: VirtualMachineError
-------------------------------------- Captured stdout setup ---------------------------------------
0x407acf02000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000001e0000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000021e19e0c9bab2400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004c4b400000000000000000000000000000000000000000000000000000000000989680000000000000000000000000000000000000000000000000000000000098968000000000000000000000000000000000000000000000000000000000004c4b4000000000000000000000000000000000000000000000000000000000000007d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000016800000000000000000000000000000000000000000000000000000000000001680000000000000000000000000000000000000000000000000000000000000064000000000000000000000000000000000000000000000000000000000000003200000000000000000000000000000000000000000000000000000000000026ac000000000000000000000000000000000000000000000000000000000000001e42616c616e63657220426f6f7374656420506f6f6c2053747261746567790000
--------------------------------------- Captured stdout call ---------------------------------------
Transaction sent: 0x88a7d3043fcc7b5ee532b37fd8232c5bd54ede5fb4de0403b32b22b2074c61e8
===================================== short test summary info ======================================
FAILED tests/balancer/rewards/test_rewards_boosted_dai.py::test_reinvest_rewards_success - browni...
======================================== 1 failed in 6.75s =========================================
Terminating local RPC client...

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 (maxRewardTradeSlippageLimitPercent = 5%).

def test_reinvest_rewards_success(StratBoostedPoolDAIPrimary):
    (env, vault) = StratBoostedPoolDAIPrimary
    rewardAmount = Wei(50e18)
    env.tokens["BAL"].transfer(vault.address, rewardAmount, {"from": env.whales["BAL"]})

    dynamicTradeParams = "(uint16,uint8,uint32,bool,bytes)"
    singleSidedRewardTradeParams = "(address,address,uint256,{})".format(dynamicTradeParams)
    assert vault.getStrategyContext()["baseStrategy"]["totalBPTHeld"] == 0
    vault.reinvestReward([eth_abi.encode_abi(
        [singleSidedRewardTradeParams],
        [[
            env.tokens["BAL"].address,
            env.tokens["DAI"].address,
            rewardAmount,
            [
                DEX_ID["UNISWAP_V3"],
                TRADE_TYPE["EXACT_IN_BATCH"],
                Wei(0),
                False,
                get_univ3_batch_data([
                    env.tokens["BAL"].address, 3000, env.tokens["WETH"].address, 500, env.tokens["DAI"].address
                ])
            ]
        ]]
    ), 0],
        {"from": accounts[1]}
    )
    assert pytest.approx(vault.getStrategyContext()["baseStrategy"]["totalBPTHeld"], rel=1e-2) == 333814721786840411586

❯ brownie test tests/balancer/rewards/test_rewards_boosted_dai.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/rewards/test_rewards_boosted_dai.py .                                         [100%]

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

Impact

Malicious users can trigger the permissionless reinvestReward 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#L45 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/MetaStable2TokenAuraVault.sol#L164 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/external/MetaStable2TokenAuraHelper.sol#L114 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/reward/TwoTokenAuraRewardUtils.sol#L48 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 TwoTokenAuraRewardUtils._executeRewardTrades and Boosted3TokenAuraRewardUtils._executeRewardTrades functions to revert if the slippage is set to zero.

    function _executeRewardTrades(
        TwoTokenPoolContext calldata poolContext,
        AuraStakingContext calldata stakingContext,
        ITradingModule tradingModule,
        bytes calldata data,
        uint256 slippageLimit
    ) internal returns (address rewardToken, uint256 primaryAmount, uint256 secondaryAmount) {
        Balanced2TokenRewardTradeParams memory params = abi.decode(
            data,
            (Balanced2TokenRewardTradeParams)
        );

-        require(params.primaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);
-        require(params.secondaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);
+        require(0 < params.primaryTrade.tradeParams.oracleSlippagePercent && params.primaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);
+        require(0 < params.secondaryTrade.tradeParams.oracleSlippagePercent && params.secondaryTrade.tradeParams.oracleSlippagePercent <= slippageLimit);
        ..SNIP..

Duplicate of #73

[jeffywu]

Duplicate of #73

[jeffywu]

I marked this as a duplicate of #73 because the underlying condition (a zero value turns off the slippage threshold in the TradingModule) is the fundamental problem in both reported issues. Although the initial affected code is in different locations, the core of the issue is in the same location.

Both issues were reported by the same auditor and I do feel that given their understanding of the issue they could have reported them both as a single issue rather than two.