xiaoming90 - `BPT_IN_FOR_EXACT_TOKENS_OUT` should be used instead of `EXACT_BPT_IN_FOR_ALL_TOKENS_OUT` for `removeLiquidity` function

[sherlock-admin2]

xiaoming90

high

BPT_IN_FOR_EXACT_TOKENS_OUT should be used instead of EXACT_BPT_IN_FOR_ALL_TOKENS_OUT for removeLiquidity function

Summary

The incorrect type of exit is performed when the affected removeLiquidity function is executed, leading to a revert or an error while withdrawing liquidity from Balancer.

Vulnerability Detail

For the BalancerBeethovenAdapter.removeLiquidity function, it was understood from the sponsor that its purpose is to perform a "variable BPT in, exact tokens out" for the Balancer pools. Thus, this function accepts the exact tokens expected to receive and the max acceptable number of LP tokens to be burned.

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/destinations/adapters/BalancerBeethovenAdapter.sol#L151

File: BalancerBeethovenAdapter.sol
151:     function removeLiquidity(
152:         IVault vault,
153:         address pool,
154:         address[] calldata tokens,
155:         uint256[] calldata exactAmountsOut,
156:         uint256 maxLpBurnAmount
157:     ) public returns (uint256[] memory actualAmounts) {
158:         bytes memory userData;
159:         if (BalancerUtilities.isComposablePool(pool)) {
160:             userData = abi.encode(ExitKindComposable.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT, maxLpBurnAmount);
161:         } else {
162:             userData = abi.encode(ExitKind.BPT_IN_FOR_EXACT_TOKENS_OUT, exactAmountsOut, maxLpBurnAmount);
163:         }

However, "exact BPT in, variable tokens out" is performed instead for Balancer's Composable pools due to the wrong ExitKindComposable.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT flag being used.

ExitKindComposable.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT used in Line 160 above is wrong because it performs an _exitExactBPTInForTokensOut function as per the Balancer's source code.

https://github.com/balancer/balancer-v2-monorepo/blob/227683919a7031615c0bc7f144666cdf3883d212/pkg/pool-stable/contracts/ComposableStablePool.sol#L843

File: ComposableStablePool.sol
843:     function _doExit(
844:         uint256[] memory balances,
845:         uint256 currentAmp,
846:         uint256 preJoinExitSupply,
847:         uint256 preJoinExitInvariant,
848:         uint256[] memory scalingFactors,
849:         bytes memory userData
850:     ) internal view returns (uint256, uint256[] memory) {
851:         StablePoolUserData.ExitKind kind = userData.exitKind();
852:         if (kind == StablePoolUserData.ExitKind.BPT_IN_FOR_EXACT_TOKENS_OUT) {
853:             return
854:                 _exitBPTInForExactTokensOut(
855:                     preJoinExitSupply,
856:                     preJoinExitInvariant,
857:                     currentAmp,
858:                     balances,
859:                     scalingFactors,
860:                     userData
861:                 );
862:         } else if (kind == StablePoolUserData.ExitKind.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT) {
863:             return _exitExactBPTInForTokensOut(preJoinExitSupply, balances, userData);
864:         } else if (kind == StablePoolUserData.ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT) {
865:             return _exitExactBPTInForTokenOut(preJoinExitSupply, preJoinExitInvariant, currentAmp, balances, userData);
866:         } else {
867:             _revert(Errors.UNHANDLED_EXIT_KIND);
868:         }
869:     }

Impact

The incorrect type of exit is performed when the affected removeLiquidity function is executed, leading to a revert or an error while withdrawing liquidity from Balancer.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/destinations/adapters/BalancerBeethovenAdapter.sol#L151

Tool used

Manual Review

Recommendation

Update the affected function to use the ExitKindComposable.BPT_IN_FOR_EXACT_TOKENS_OUT flag instead of the ExitKindComposable.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT flag as shown below.

function removeLiquidity(
    IVault vault,
    address pool,
    address[] calldata tokens,
    uint256[] calldata exactAmountsOut,
    uint256 maxLpBurnAmount
) public returns (uint256[] memory actualAmounts) {
    bytes memory userData;
    if (BalancerUtilities.isComposablePool(pool)) {
-        userData = abi.encode(ExitKindComposable.EXACT_BPT_IN_FOR_ALL_TOKENS_OUT, maxLpBurnAmount);
+        userData = abi.encode(ExitKindComposable.BPT_IN_FOR_EXACT_TOKENS_OUT, maxLpBurnAmount);
    } else {
        userData = abi.encode(ExitKind.BPT_IN_FOR_EXACT_TOKENS_OUT, exactAmountsOut, maxLpBurnAmount);
    }

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, contract BalancerBeethovenAdapter is used by BalancerAuraDestinationVault, but there is no function call from BalancerAuraDestinationVault to BalancerBeethovenAdapter.removeLiquidity(). So no impact here

[xiaoming9090]

Escalate

This is an error in the code. The protocol team should review it and determine if it indeed causes no impact on other contracts (internal or external) or off-chain componentsand fix them if necessary.

Another point that I would like to highlight is that based on the judging comment, if this issue indeed has no impact after the protocol has reviewed, it should be marked as Low/Informational instead of Invalid. If an issue is technically correct, it should not be marked as invalid due to the following reasons:

1) The protocol team might miss out on valid low/informational issues , which they might be interested in fixing if they have seen them in the first place if they are tagged correctly. During fixing review, some project teams I have worked with in Sherlock are also interested in fixing Low/Information issues. 2) Watson's ratio might unnecessarily be penalized for an issue that is valid in the first place.

[sherlock-admin2]

Escalate

This is an error in the code. The protocol team should review it and determine if it indeed causes no impact on other contracts (internal or external) or off-chain componentsand fix them if necessary.

Another point that I would like to highlight is that based on the judging comment, if this issue indeed has no impact after the protocol has reviewed, it should be marked as Low/Informational instead of Invalid. If an issue is technically correct, it should not be marked as invalid due to the following reasons:

1) The protocol team might miss out on valid low/informational issues , which they might be interested in fixing if they have seen them in the first place if they are tagged correctly. During fixing review, some project teams I have worked with in Sherlock are also interested in fixing Low/Information issues. 2) Watson's ratio might unnecessarily be penalized for an issue that is valid in the first place.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

Hello @xiaoming9090, thank you for your feedback on my judging. I appreciate your input, and I will take more care when evaluating issues as low or invalid in the future.

Regarding this specific issue, I believe it should be categorized as low. Here's my reasoning: In my view, if a function is actually used in a larger codebase within Tokemak and that codebase is not currently within the scope of the audit, the issue should still be marked as low. This approach is similar to the treatment of view functions. We cannot determine whether a view function can be used in a larger codebase or not. Therefore, it would be unfair to competitors who submit view-related issues and do not receive any rewards. That's why I have categorized this report as low.

[Evert0x]

Planning to reject escalation and keep issue state.

It's unclear to me why this has high severity impact. It's unclear how a potential loss can occur.

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: rejected